Re: [PATCH net 6/6] xsk: validate metadata when processing requests

On 07/28, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Zero-copy drivers unconditionally parse XDP TX metadata ignoring the XDP_TX_METADATA option flag.
> - [High] The stmmac driver drops AF_XDP TX frames without generating completions when the frame length exceeds max_sdu, causing the AF_XDP completion queue (CQ) to permanently desynchronize.
> --
> 
> --- Patch [6]: [PATCH net 6/6] xsk: validate metadata when processing requests ---
> commit 8967b4032038ba439c0a13ce2dc4cab303b87c2e
> Author: Stanislav Fomichev <[email protected]>
> 
> xsk: validate metadata when processing requests
>     
> The zero-copy path validates TX metadata while obtaining the descriptor
> context, then reads it again later when preparing the hardware request.
> Validate the metadata in xsk_tx_metadata_request() and use the resulting
> flags snapshot for every feature check.
> 
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> > index 5a18d00deb4f4..7b6cdc037c00f 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -3082,7 +3082,7 @@ static void igc_xdp_xmit_zc(struct igc_ring *ring)
> >  		meta_req.tx_buffer = bi;
> >  		meta_req.meta = meta;
> >  		meta_req.used_desc = 0;
> > -		xsk_tx_metadata_request(&meta, &igc_xsk_tx_metadata_ops,
> > +		xsk_tx_metadata_request(pool, &meta, &igc_xsk_tx_metadata_ops,
> >  					&meta_req);
> 
> [Severity: High]
> This is a pre-existing issue, but do the zero-copy drivers unconditionally
> parse XDP TX metadata while ignoring the XDP_TX_METADATA option flag?
> 
> Looking at igc_xdp_xmit_zc() (and similarly in mlx5e_xsk_tx() and
> stmmac_xdp_xmit_zc()), the zero-copy path reads metadata directly without
> checking xdp_desc.options:
> 
>     meta = xsk_buff_get_metadata(pool, xdp_desc.addr);
> 
> If an application sends a standard AF_XDP packet without requesting metadata
> (by clearing the XDP_TX_METADATA flag), the driver ignores desc->options and
> unconditionally reads the metadata memory. Since the memory might contain
> stale data, meta->flags could contain valid but bogus bits.
> 
> Could this trigger unintended hardware offloads, causing data corruption
> (bogus checksum offsets) or TX queue stalls (bogus launch times)?

That seems like a valid pre-existing issue, will address separately.
 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 3f5111969c414..eb666b0bcdd22 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -2747,8 +2747,8 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
> >  		meta_req.set_ic = &set_ic;
> >  		meta_req.tbs = tx_q->tbs;
> >  		meta_req.edesc = &tx_q->dma_entx[entry];
> > -		xsk_tx_metadata_request(&meta, &stmmac_xsk_tx_metadata_ops,
> > -					&meta_req);
> > +		xsk_tx_metadata_request(pool, &meta,
> > +					&stmmac_xsk_tx_metadata_ops, &meta_req);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does stmmac_xdp_xmit_zc()
> permanently desynchronize the AF_XDP completion queue when dropping oversized
> frames?
> 
> Earlier in stmmac_xdp_xmit_zc(), if the frame length exceeds max_sdu, the
> descriptor is dropped via continue without issuing an XSK completion or
> posting a dummy hardware descriptor:
> 
>     if (priv->est && priv->est->enable &&
>         priv->est->max_sdu[queue] &&
>         xdp_desc.len > priv->est->max_sdu[queue]) {
>         priv->xstats.max_sdu_txq_drop[queue]++;
>         continue;
>     }
> 
> Since xsk_tx_peek_desc() reserves a slot in the AF_XDP completion queue (CQ),
> dropping the frame skips the hardware descriptor setup, and no completion
> interrupt is generated for this packet.
> 
> When a subsequent packet completes, xsk_tx_completed() will advance the CQ
> producer. Because CQ entries are positional, would user space receive the
> address of the dropped packet as a completion instead of the actually
> completed packet, permanently leaking descriptors?

Hmm, good point, will send a patch separately to reorder xsk_tx_peek_desc
and max_sdu check.