Re: [PATCH net 2/6] xsk: pass TX metadata pointer by reference

On 07/28, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] `xsk_tx_metadata_to_compl()` returns early when `meta == NULL` without clearing the `compl` output structure, leading to arbitrary kernel memory writes during TX completion.
> --
> 
> --- Patch [2]: [PATCH net 2/6] xsk: pass TX metadata pointer by reference ---
> commit 83fcec7a0c0075ec991849b8d99f6d6cd083c335
> Author: Stanislav Fomichev <[email protected]>
> 
> xsk: pass TX metadata pointer by reference
> 
> Completion handling needs to know whether a timestamp was requested when
> the metadata was processed. Let xsk_tx_metadata_request() update the
> caller's metadata pointer so that decision can be carried forward without
> rereading user-controlled flags.
> 
> [ ... ]
> 
> > diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> > index 8b51876efbed1..06c081feff42d 100644
> > --- a/include/net/xdp_sock.h
> > +++ b/include/net/xdp_sock.h
> > @@ -153,17 +153,19 @@ static inline void xsk_tx_metadata_to_compl(struct xsk_tx_metadata *meta,
> 
> [Severity: Critical]
> This isn't a bug introduced by this patch, but I noticed that 
> xsk_tx_metadata_to_compl() returns early without explicitly clearing the 
> compl structure when meta is NULL:
> 
> static inline void xsk_tx_metadata_to_compl(struct xsk_tx_metadata *meta,
>                                             struct xsk_tx_metadata_compl *compl)
> {
>         if (!meta)
>                 return;
> 
> In drivers like mlx5e_xsk_tx(), compl is declared on the stack and passed in 
> uninitialized:
> 
> mlx5e_xsk_tx() {
>         if (xp_tx_metadata_enabled(sq->xsk_pool)) {
>                 struct xsk_tx_metadata_compl compl;
> 
>                 xsk_tx_metadata_to_compl(meta, &compl);
>                 ...
>                 mlx5e_xdpi_fifo_push(..., { .xsk_meta = compl });
>         }
> }
> 
> If an untrusted user sends a packet with invalid flags, causing meta to be 
> NULL, could the uninitialized stack garbage in compl be pushed to the NAPI TX 
> completion FIFO? Would this cause the completion path to dereference a garbage 
> pointer and write a hardware timestamp to arbitrary kernel memory?
> 
> Similarly, in stmmac_xdp_xmit_zc():
> 
> stmmac_xdp_xmit_zc() {
>         ...
>         xsk_tx_metadata_to_compl(meta,
>                                  &tx_q->tx_skbuff_dma[entry].xsk_meta);
> }
> 
> Could this early return leave a stale pointer from a previous packet in the 
> descriptor ring, leading to an accidental overwrite of userspace UMEM?

I think this is now addressed by:
- [PATCH net 3/6] xsk: clear metadata pointer when no timestamp is requested