Re: [PATCH net-next v2 1/4] NTB: ntb_transport: Order RX descriptor reads after completion

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

Pre-existing issues:
- [Critical] Double-fetch vulnerability (TOCTOU) on `hdr->len` allowing kernel heap buffer overflow.
- [Critical] Use-After-Free of memory windows during link teardown due to missing tasklet and DMA synchronization.
- [Critical] Invalid use of `virt_to_phys()` on DMA coherent memory corrupts DMA mappings.
- [High] Missing endianness conversion on memory-mapped payload headers breaks big-endian systems.
- [High] RX DMA abort fallback copies data from the wrong ring index, causing data corruption.
- [High] Concurrent modification of `qp->rx_index` causes ring state corruption.
- [High] Missing bounds validation of `hdr->len` against frame size causes out-of-bounds read and information leak.
--

--- Patch [1]: [PATCH net-next v2 1/4] NTB: ntb_transport: Order RX descriptor reads after completion ---
(Note: The format of this report has been altered due to recitation
restrictions. The original patch diff is not quoted, and findings are
provided as a free-form summary.)

Commit: 27d90886c93f5cd8fe8f9c93d5acf5cd83c90d55
Author: Koichiro Den <[email protected]>
Subject: NTB: ntb_transport: Order RX descriptor reads after completion

This commit addresses an ordering issue on weakly ordered CPUs where the
descriptor and payload could be consumed before DESC_DONE_FLAG is fully
observed. It ensures the completion word is read once, followed by a
dma_rmb(), and uses the saved word for subsequent checks.

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

[Severity: Critical]
This is a pre-existing issue, but in ntb_process_rxc(), could the double
fetch of hdr->len lead to a TOCTOU vulnerability?

The code reads hdr->len to validate it against entry->len. After this check,
operations like qp->rx_bytes += hdr->len and entry->len = hdr->len read the
length directly from DMA-coherent memory again.

Could a malicious peer concurrently swap a valid small length with a huge
value after the bounds check passes, causing the fallback CPU memcpy to
copy out-of-bounds into the local kernel buffer?

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of endianness
conversion on memory-mapped payload headers break big-endian systems?

The TX side writes header fields using iowrite32(), which implicitly converts
to little-endian. However, the RX side in ntb_process_rxc() reads the
memory directly via READ_ONCE(hdr->flags) and evaluates flags against
DESC_DONE_FLAG without using le32_to_cpu().

Would this cause the receiver on big-endian hosts to misinterpret the bytes
and drop all received packets?

[Severity: Critical]
This is a pre-existing issue, but does ntb_transport_link_cleanup() safely
free memory windows if tasklets or DMA engines are still active?

When the link drops, it calls ntb_free_mw(), invoking dma_free_attrs().
However, it does not call tasklet_kill(&qp->rxc_db_work) or
dmaengine_terminate_all().

If the RX tasklet is actively processing doorbells or the DMA engine is
transferring data, will this result in a use-after-free or IOMMU DMA fault?

[Severity: High]
This isn't a bug introduced by this patch, but does ntb_rx_copy_callback()
use the correct index when falling back to a CPU copy?

If a DMA transaction aborts, it calculates the offset using the shared
qp->rx_index via qp->rx_buff + qp->rx_max_frame * qp->rx_index. Because the
callback is asynchronous, the tasklet may have already advanced
qp->rx_index.

Should it use the saved entry->rx_index instead to avoid copying data from
the wrong ring slot and corrupting the stream?

[Severity: High]
This is a pre-existing issue, but can ntb_qp_link_down_reset() safely reset
indices while traffic is flowing?

It forcefully zeroes qp->rx_index and qp->tx_index from a workqueue without
synchronizing with the rxc_db_work tasklet.

If the tasklet is running concurrently in ntb_process_rxc(), could the
unsynchronized writes corrupt the ring buffer state?

[Severity: High]
This isn't a bug introduced by this patch, but does ntb_process_rxc()
adequately validate hdr->len against the physical frame size?

The code verifies hdr->len against entry->len (the client buffer size) but
never against qp->rx_max_frame (the hardware memory window slice size).

If a client configures a large buffer, could a malicious remote peer set
hdr->len to a value exceeding the frame size, allowing an out-of-bounds
read that leaks adjacent kernel memory?

[Severity: Critical]
This is a pre-existing issue, but is it valid to use virt_to_phys() on
DMA coherent memory in ntb_async_rx_submit()?

The offset pointer is derived from qp->rx_buff, which is allocated via
dma_alloc_attrs(). On architectures where this returns a non-linear virtual
address in the vmalloc area, virt_to_phys() will return an incorrect
physical address.

Will this cause the DMA engine to write received data to arbitrary physical
memory locations, leading to silent memory corruption?

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