Re: [PATCH net v3 4/4] net: mvpp2: build skb from XDP-adjusted data on XDP_PASS

[sashiko-bot@kernel.org]

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

New issues:
- [High] Overwriting `rx_bytes` with the XDP-modified packet length causes a partial DMA sync on the SKB allocation error path, potentially corrupting subsequent packets.

Pre-existing issues:
- [High] Hardware checksum offload is unconditionally applied even after XDP program modifications, potentially bypassing network stack checksum validation for corrupted packets.
--

commit 92e9744c23c0c27c625a826b0e44e159927ea85a
Author: Til Kaiser <mail@tk154.de>

net: mvpp2: build skb from XDP-adjusted data on XDP_PASS

When an XDP program modifies packet geometry and returns XDP_PASS, mvpp2
currently discards these adjustments and builds the skb using fixed
offsets. This commit updates the offset and length variables to reflect
the XDP-adjusted data, ensuring the skb correctly represents the modified
packet.

> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 481daafdc1cbc..dd30674148aa4 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c

[ ... ]

> @@ -3999,6 +4000,10 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
>  				continue;
>  			}
>  
> +			/* Update offset and length to reflect any XDP adjustments. */
> +			rx_offset = xdp.data     - data;
> +			rx_bytes  = xdp.data_end - xdp.data;
> +

[Severity: High]
If rx_bytes is updated to reflect the XDP-modified packet length here, what
happens if skb allocation fails later in mvpp2_rx()?

Looking at the error path:

mvpp2_rx() {
    ...
    if (!skb) {
        if (pp) {
            page_pool_put_page(pp, virt_to_head_page(data),
                               rx_bytes + MVPP2_MH_SIZE,
                               true);
    ...
}

If the XDP program truncated the packet, rx_bytes is smaller than the
original size synced to the CPU. Does this cause an incomplete DMA sync
back to the device, leaving dirty CPU cache lines in the remainder of the
buffer? Could this corrupt subsequent packets when the hardware reuses the
buffer?

>  			metasize = xdp.data - xdp.data_meta;
>  		}
>  
> @@ -4041,7 +4046,7 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
>  		ps.rx_packets++;
>  		ps.rx_bytes += rx_bytes;
>  
> -		skb_reserve(skb, MVPP2_MH_SIZE + MVPP2_SKB_HEADROOM);
> +		skb_reserve(skb, rx_offset);
>  		skb_put(skb, rx_bytes);
>  		if (metasize)
>  			skb_metadata_set(skb, metasize);

[Severity: High]
This isn't a bug introduced by this patch, but does unconditionally
applying the hardware checksum status to the skb here bypass software
validation for packets modified by XDP?

Looking at the code immediately following this block in mvpp2_rx():

    skb->ip_summed = mvpp2_rx_csum(port, rx_status);
    
Since XDP programs can modify packet headers or payloads, the
hardware-validated checksum might no longer be correct. If the hardware
reports CHECKSUM_UNNECESSARY, could this allow corrupted or improperly
modified packets to be incorrectly accepted by the network stack? Should
the hardware checksum be invalidated (e.g., set to CHECKSUM_NONE) if an
XDP program has been executed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605121332.147397-1-mail@tk154.de?part=4