Re: [PATCH net-next 4/8] eth: mpnic: implement Tx queue allocation and cleanup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] eth: mpnic: TOCTOU IOMMU bypass when unmapping Tx descriptors
- [High] eth: mpnic: missing bounds check on hardware completion head
--

commit 154e61c52ca2706a8f945c1575f31b7f328f5529
Author: Daniel Zahka <daniel.zahka@gmail.com>

eth: mpnic: implement Tx queue allocation and cleanup

Queues come in triads of two work queues and one completion queue. The
Tx triad only uses the first work queue for now, the second one will be
used for the XDP ring later.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index 0000000000000..fe360a26a27be 100644
> --- /dev/null
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
[ ... ]
> +static void mpnic_unmap_single_twd(struct device *dev, __le64 *twd)
> +{
> +	u64 raw_twd = le64_to_cpu(*twd);
> +
> +	dma_unmap_single(dev, FIELD_GET(MPNIC_TWD_ADDR, raw_twd),
> +			 FIELD_GET(MPNIC_TWD_LEN, raw_twd), DMA_TO_DEVICE);
> +}
> +
> +static void mpnic_unmap_page_twd(struct device *dev, __le64 *twd)
> +{
> +	u64 raw_twd = le64_to_cpu(*twd);
> +
> +	dma_unmap_page(dev, FIELD_GET(MPNIC_TWD_ADDR, raw_twd),
> +		       FIELD_GET(MPNIC_TWD_LEN, raw_twd), DMA_TO_DEVICE);
> +}

[Severity: Critical]
Can a malicious PCIe device corrupt host memory by rewriting these descriptors?

During Tx completion, the DMA address and length are read directly from the
device-accessible descriptor ring in mpnic_clean_twq0() and passed to
dma_unmap_single() and dma_unmap_page() here. Because the descriptor memory
is coherent DMA memory, the device could change the addresses between
submission and completion, bypassing IOMMU protections.

Could we store the DMA metadata in host-private memory (like the tx_buf
array) instead of reading it back from the ring?

> +
> +static void mpnic_clean_twq0(struct mpnic_napi_vector *nv, int napi_budget,
> +			     struct mpnic_ring *ring, bool discard,
> +			     unsigned int hw_head)
> +{
> +	u64 total_bytes = 0, total_packets = 0;
> +	unsigned int head = ring->head;
> +	struct netdev_queue *txq;
> +	unsigned int clean_desc;
> +
> +	clean_desc = (hw_head - head) & ring->size_mask;
> +
> +	while (clean_desc) {
> +		struct sk_buff *skb = ring->tx_buf[head];
> +		unsigned int desc_cnt;
> +
> +		desc_cnt = MPNIC_XMIT_CB(skb)->desc_count;

[Severity: High]
Is it possible for a malfunctioning or malicious device to cause a NULL pointer
dereference here?

The loop boundary clean_desc is calculated from the untrusted hw_head without
validating it against the software-maintained tail that tracks actually
submitted descriptors. If the hardware reports an out-of-bounds hw_head, the
loop will access unpopulated slots in ring->tx_buf, returning a NULL skb that
gets immediately dereferenced.

> +		if (desc_cnt > clean_desc)
> +			break;
> +
> +		ring->tx_buf[head] = NULL;
> +
> +		clean_desc -= desc_cnt;
> +
> +		/* Step over the metadata descriptor */
> +		head++;
> +		head &= ring->size_mask;
> +		desc_cnt--;
> +
> +		mpnic_unmap_single_twd(nv->dev, &ring->desc[head]);
> +		head++;
> +		head &= ring->size_mask;
> +		desc_cnt--;
> +
> +		while (desc_cnt--) {
> +			mpnic_unmap_page_twd(nv->dev, &ring->desc[head]);
> +			head++;
> +			head &= ring->size_mask;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922-linux-mpnic-v1-0-236844f53072@gmail.com?part=4