Re: [PATCH net 4/4] NTB: ntb_transport: Reject oversized TX buffers

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Unvalidated `transport_mtu` module parameter allows division by zero and integer underflows leading to out-of-bounds memory writes.
- [High] Queue entries are leaked in `ntb_send_link_down()` when the transmission ring is full.
- [High] Queue teardown permanently leaks `ntb_queue_entry` objects and associated network payload buffers.
--

--- Patch [4]: [PATCH net 4/4] NTB: ntb_transport: Reject oversized TX buffers ---
commit 0674c1f1ad07dc65b6ed7cc1cb352ca787323a36
Author: Koichiro Den <[email protected]>
Subject: NTB: ntb_transport: Reject oversized TX buffers

This commit moves the oversized buffer check from ntb_process_tx() to 
ntb_transport_tx_enqueue(). It returns -EMSGSIZE before acquiring a queue
entry, allowing the caller to retain ownership and free the buffer.

Link: https://lore.kernel.org/r/[email protected]/

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 4bdd81edcb87a..f9caa1a653c5b 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1955,15 +1955,6 @@ static int ntb_process_tx(struct ntb_transport_qp *qp,
>  		return -EAGAIN;
>  	}
>  
> -	if (entry->len > qp->tx_max_frame - sizeof(struct ntb_payload_header)) {
> -		if (qp->tx_handler)
> -			qp->tx_handler(qp, qp->cb_data, NULL, -EIO);
> -
> -		ntb_list_add(&qp->ntb_tx_free_q_lock, &entry->entry,
> -			     &qp->tx_free_q);
> -		return 0;
> -	}
> -
>  	ntb_async_tx(qp, entry);

[Severity: High]
This is a pre-existing issue, but does ntb_send_link_down() leak queue entries
when ntb_process_tx() fails?

When ntb_process_tx() returns an error like -EAGAIN,
ntb_transport_tx_enqueue() correctly adds the entry back to tx_free_q.
However, ntb_send_link_down() doesn't seem to return the entry to the
queue or free it:

drivers/ntb/ntb_transport.c:ntb_send_link_down() {
    ...
    rc = ntb_process_tx(qp, entry);
    if (rc)
        dev_err(&pdev->dev, "ntb: QP%d unable to send linkdown msg\n",
                qp->qp_num);
    ...
}

Could repeated link flaps permanently exhaust the queue?

[ ... ]

> @@ -2356,6 +2347,9 @@ int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
>  	if (!qp->link_is_up)
>  		return -ENOLINK;
>  
> +	if (len > qp->tx_max_frame - sizeof(struct ntb_payload_header))
> +		return -EMSGSIZE;

[Severity: High]
This is a pre-existing issue, but if a user sets the transport_mtu module
parameter to a small value (like 1 to 11), could this length check underflow?

If qp->tx_max_frame evaluates to less than sizeof(struct ntb_payload_header),
the subtraction will underflow to a large positive integer, bypassing the
bounds check. This would then lead to out-of-bounds memory writes during
pointer arithmetic in ntb_async_tx():

drivers/ntb/ntb_transport.c:ntb_async_tx() {
    ...
    hdr = offset + qp->tx_max_frame - sizeof(struct ntb_payload_header);
    ...
}

Also, if transport_mtu is set to 0, it appears to cause a division by zero in
ntb_transport_init_queue():

drivers/ntb/ntb_transport.c:ntb_transport_init_queue() {
    ...
    qp->tx_max_entry = tx_size / qp->tx_max_frame;
    ...
}

Should the module parameter be validated to prevent this?

> +
>  	entry = ntb_list_rm(&qp->ntb_tx_free_q_lock, &qp->tx_free_q);
>  	if (!entry) {
>  		qp->tx_err_no_buf++;

[Severity: High]
This is a pre-existing issue, but does queue teardown permanently leak
ntb_queue_entry objects and their associated network buffers?

In ntb_transport_free_queue(), entries are freed without invoking their
completion handlers (like tx_handler) to release the attached network
buffers:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
    while ((entry = ntb_list_rm(&qp->ntb_tx_offl_q_lock, &qp->tx_offl_q)))
        kfree(entry);
    ...
}

Additionally, it seems DMA operations are terminated without callbacks:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
    dmaengine_terminate_all(chan);
    ...
}

Because ntb_async_tx_submit() doesn't populate qp->last_cookie with the
TX cookie, does the preceding dma_sync_wait() incorrectly wait on the RX
cookie, allowing engine termination to orphan all active TX entries?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4