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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] net: mpnic: trusting DMA parameters from device-accessible ring
- [High] net: mpnic: missing bounds check on hardware completion head causes NULL deref
- [High] net: mpnic: integer underflow in desc_cnt causes massive out-of-bounds unmaps
- [High] net: mpnic: missing memory barrier before completion queue doorbell
--

commit 92d64496d6b2ec052301fad8882b705af9c36a85
Author: Daniel Zahka <daniel.zahka@gmail.com>

    eth: mpnic: implement Tx queue allocation and cleanup

This commit introduces the allocation and cleanup of the Tx queues for the mpnic driver. It sets up triads of work and completion queues per NAPI vector, along with their descriptor memory, interrupts, and completion processing.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index 0000000000000..853976e11b032
> --- /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);
> +}

[Severity: Critical]
Does this code rely on untrusted DMA parameters?

The descriptor ring memory is device-accessible. If a malicious or buggy
PCIe device overwrites the address and length fields after submission, it
could trick the host into unmapping arbitrary kernel or IOMMU memory. Should
the driver store DMA mapping state in host-only software memory instead?

> +
> +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]
Similar to mpnic_unmap_single_twd(), does this code safely handle potentially
spoofed DMA parameters from the hardware 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 hw_head to be spoofed by the device, causing an
out-of-bounds access?

If the completion queue reports an out-of-bounds hw_head, clean_desc might
cover unpopulated slots in the ring buffer. This would result in accessing
ring->tx_buf[head] which is NULL, leading to a NULL pointer dereference when
accessing MPNIC_XMIT_CB(skb)->desc_count. Should the number of completed
descriptors be validated against the number of pending descriptors?

> +		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--) {

[Severity: High]
Can desc_cnt underflow here if desc_cnt is less than 2?

If the completion head is advanced by a malicious device to cover a packet
currently being built (where desc_cnt might be 0), the unconditional double
decrement will underflow desc_cnt to a massive unsigned value. This would
cause the subsequent loop to execute millions of times with out-of-bounds
ring indices and out-of-bounds DMA unmaps.

> +			mpnic_unmap_page_twd(nv->dev, &ring->desc[head]);
> +			head++;
> +			head &= ring->size_mask;
> +		}
> +
> +		total_bytes += MPNIC_XMIT_CB(skb)->bytecount;
> +		total_packets++;
> +
> +		napi_consume_skb(skb, napi_budget);
> +	}
[ ... ]
> +static void mpnic_commit_cq_head(struct mpnic_ring *cmpl)
> +{
> +	u32 head = cmpl->head;
> +
> +	/* The tail shadows the last value written to the doorbell, so a
> +	 * completion queue which has not moved costs no MMIO write.
> +	 */
> +	if (cmpl->tail != head) {
> +		cmpl->tail = head;
> +		writeq(head & cmpl->size_mask, cmpl->doorbell);
> +	}
> +}

[Severity: High]
Does this MMIO write to the doorbell require a memory barrier to guarantee
prior descriptor reads are complete?

On weakly ordered architectures like ARM64, writeq() only provides a
Store-Store barrier (wmb()). Without a read-store barrier (like rmb() or
dma_rmb()), prior memory loads could be reordered after the doorbell write,
potentially allowing the hardware to overwrite descriptors while the CPU is
still reading them.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-linux-mpnic-v2-0-4badc9b58b9e@gmail.com?part=4