Re: [PATCH net 3/6] gve: fix XSK buffer leak on error descriptor

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

Pre-existing issues:
- [Critical] Fragment desynchronization resulting in truncated or malformed packets being incorrectly pushed to the network stack upon intermediate descriptor errors.
- [High] Massive unmitigated leak of AF_XDP zero-copy buffers (`xsk_buff`) and XDP frames (`xdpf`) on XDP_TX and XDP_REDIRECT paths.
- [High] `gve_rx_xsk_dqo` unconditionally overwrites `rx->ctx.skb_head` on multi-fragment XDP sequences, causing leaks.
- [High] The driver inherently trusts the `packet_len` provided by the device DMA completion descriptor without verifying it against the allocated buffer size.
- [High] Missing bounds check on `compl_desc->header_len` allows out-of-bounds reads from the header buffer array.
--

--- Patch [3]: [PATCH net 3/6] gve: fix XSK buffer leak on error descriptor ---
commit 92aa7b2aab7b9d9aa0aed8717f991d9e6e5c13b2
Author: Joshua Washington <[email protected]>

gve: fix XSK buffer leak on error descriptor

When the error bit is set in the RX completion descriptor, the buf_state
and its attached buffer should be freed. In the case of AF_XDP ZC, the
XSK buffer was not freed, leading to a leak.

Fixes: c1fffc5d66a7 ("gve: implement DQO RX datapath and control path for AF_XDP zero-copy")
Cc: [email protected]

> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index 2c1ce23d15508..811db42d05c67 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> @@ -845,7 +845,12 @@ static int gve_rx_dqo(struct napi_struct *napi, struct gve_rx_ring *rx,
>  	}
>  
>  	if (unlikely(compl_desc->rx_error)) {
> -		gve_free_buffer(rx, buf_state);
> +		if (buf_state->xsk_buff) {
> +			xsk_buff_free(buf_state->xsk_buff);
> +			gve_free_buf_state(rx, buf_state);
> +		} else {
> +			gve_free_buffer(rx, buf_state);
> +		}
>  		return -EINVAL;
>  	}

[Severity: High]
This is a pre-existing issue, but does the driver verify the length fields
provided by the device DMA completion descriptor?

In gve_rx_dqo(), the driver reads packet_len and header_len directly from
the untrusted descriptor:

        buf_len = compl_desc->packet_len;
        hdr_len = compl_desc->header_len;

It then uses hdr_len to copy data:

        rx->ctx.skb_head =
                gve_rx_copy_data(priv->dev, napi,
                                 rx->dqo.hdr_bufs.data + offset,
                                 hdr_len);

Could a buggy or malicious device supply a large header length (up to 1023
bytes), causing out-of-bounds memory reads from the header buffer array?
Similarly, could an unchecked packet_len result in out-of-bounds memory
reads being copied into SKBs and pushed to the network stack?

[Severity: High]
This isn't a bug introduced by this patch, but are there other paths where
AF_XDP zero-copy buffers or XDP frames can leak?

In drivers/net/ethernet/google/gve/gve_rx_dqo.c:gve_xsk_done_dqo(), the
XDP_REDIRECT failure path appears to fall through without freeing the
original xsk_buff:

        case XDP_REDIRECT:
                if (unlikely(xdp_do_redirect(priv->dev, xdp, xprog)))
                        goto err;

Also, in drivers/net/ethernet/google/gve/gve_tx_dqo.c:gve_xdp_xmit_one_dqo(),
the error path nullifies the packet frame without calling xdp_return_frame():

err:
        pkt->xdpf = NULL;
        pkt->num_bufs = 0;
        gve_free_pending_packet(tx, pkt);
        return err;

Does this lead to resource exhaustion if XDP_TX or XDP_REDIRECT actions fail?

[Severity: Critical]
This is a pre-existing issue, but does the RX poll loop correctly track
dropped packets across multiple descriptors?

In gve_rx_poll_dqo(), an error causes the skb to be freed:

        if (err < 0) {
                gve_rx_free_skb(napi, rx);

However, if this was the first or intermediate descriptor of a multi-fragment
packet, there doesn't seem to be a flag (like rx->ctx.drop_pkt) to ignore
the remaining descriptors. As a result, when the next fragment is processed in
gve_rx_dqo(), a new skb is allocated:

        rx->ctx.skb_head = napi_get_frags(napi);

Could this fragment desynchronization allow subsequent fragments of the same
packet to be parsed as the start of an entirely new packet?

[Severity: High]
This isn't a bug introduced by this patch, but does gve_rx_xsk_dqo() leak
skbs on multi-fragment XDP sequences?

If a multi-fragment packet arrives while AF_XDP zero-copy is active,
gve_rx_xsk_dqo() unconditionally overwrites rx->ctx.skb_head:

        /* Copy the data to skb */
        rx->ctx.skb_head = gve_rx_copy_data(priv->dev, napi,
                                            xdp->data, buf_len);

Could this unconditionally overwrite the globally stored skb_head and
skb_tail without appending to or freeing the existing partial SKB,
causing a permanent leak of SKBs allocated for prior fragments?

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