Re: [PATCH net 4/6] xsk: validate launch-time metadata size

On 07/28, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Time-of-Check to Time-of-Use (TOCTOU) race condition in zero-copy TX metadata validation.
> 
> Pre-existing issues:
> - [Critical] struct xsk_tx_metadata has a layout mismatch between 32-bit and 64-bit architectures, breaking 32-bit userspace compatibility.
> - [High] Calling `skb_checksum_help()` on an empty or fragmented SKB in AF_XDP zero-copy path drops packets.
> --
> 
> --- Patch [4]: [PATCH net 4/6] xsk: validate launch-time metadata size ---
> commit 812dce9383d33c01b8fe81b1fbbcc8371998ca62
> Author: Stanislav Fomichev <[email protected]>
> 
> xsk: validate launch-time metadata size
> 
> This commit validates the launch-time metadata size by rejecting requests
> when the registered metadata area does not contain the complete field. It
> also snapshots the validated flags for the generic transmit path to avoid
> inconsistent decisions if user space changes the flags concurrently.
> 
> > diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
> > index 46797645a0c24..68a7876013572 100644
> > --- a/include/net/xdp_sock_drv.h
> > +++ b/include/net/xdp_sock_drv.h
> [ ... ]
> > @@ -260,21 +260,28 @@ xsk_buff_raw_get_ctx(const struct xsk_buff_pool *pool, u64 addr)
> >  	0)
> >  
> >  static inline bool
> > -xsk_buff_valid_tx_metadata(const struct xsk_tx_metadata *meta)
> > +xsk_buff_valid_tx_metadata(const struct xsk_buff_pool *pool,
> > +			   const struct xsk_tx_metadata *meta, u64 *flags)
> >  {
> > -	return !(meta->flags & ~XDP_TXMD_FLAGS_VALID);
> > +	*flags = READ_ONCE(meta->flags);
> > +	if (*flags & XDP_TXMD_FLAGS_LAUNCH_TIME)
> > +		if (pool->tx_metadata_len <
> > +		    offsetofend(struct xsk_tx_metadata, request.launch_time))
> > +			return false;
> 
> [Severity: Critical]
> This is a pre-existing issue, but the UAPI struct definition in
> include/uapi/linux/if_xdp.h has two __u16 fields followed by a __u64 field
> without explicit padding.
> 
> 64-bit kernels will read launch_time from offset 16, while 32-bit userspace
> writes it to offset 12.
> 
> This newly added validation explicitly expects the 64-bit structure size,
> which solidifies the layout incompatibility.
> 
> Does struct xsk_tx_metadata have a layout mismatch between 32-bit and 64-bit
> architectures that prevents 32-bit userspace compatibility?

Same as the other email, yes, adding padding here is probably a good idea,
will send separately.

> > +	return !(*flags & ~XDP_TXMD_FLAGS_VALID);
> >  }
> >  
> >  static inline struct xsk_tx_metadata *
> >  __xsk_buff_get_metadata(const struct xsk_buff_pool *pool, void *data)
> >  {
> >  	struct xsk_tx_metadata *meta;
> > +	u64 flags;
> >  
> >  	if (!pool->tx_metadata_len)
> >  		return NULL;
> >  
> >  	meta = data - pool->tx_metadata_len;
> > -	if (unlikely(!xsk_buff_valid_tx_metadata(meta)))
> > +	if (unlikely(!xsk_buff_valid_tx_metadata(pool, meta, &flags)))
> >  		return NULL; /* no way to signal the error to the user */
> >  
> >  	return meta;
> 
> [Severity: Medium]
> The snapshotted flags are validated for size compliance in
> xsk_buff_valid_tx_metadata() but then discarded, returning the
> un-snapshotted user memory pointer (meta) to the driver.
> 
> Later in the zero-copy driver path, xsk_tx_metadata_request() re-reads
> meta->flags directly from user memory:
> 
> include/net/xdp_sock.h:xsk_tx_metadata_request() {
>     ...
>     if (meta->flags & XDP_TXMD_FLAGS_LAUNCH_TIME)
>         ops->tmo_request_launch_time(meta->request.launch_time, priv);
>     ...
> }
> 
> Does this create a Time-of-Check to Time-of-Use (TOCTOU) race condition in
> the zero-copy TX metadata validation where userspace can concurrently enable
> launch time after the size validation?

It is explained in the commit message:

  Note that only xsk_skb_metadata is properly using the flags,
  __xsk_buff_get_metadata ignores them. Next commits address that.

> >  }
> [ ... ]
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index fcc6f17f35760..7855ee09c4b64 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -967,15 +967,16 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
> >  {
> >  	struct xsk_tx_metadata *meta = NULL;
> >  	u16 csum_start, csum_offset;
> > +	u64 flags;
> >  
> >  	if (unlikely(pool->tx_metadata_len == 0))
> >  		return -EINVAL;
> >  
> >  	meta = buffer - pool->tx_metadata_len;
> > -	if (unlikely(!xsk_buff_valid_tx_metadata(meta)))
> > +	if (unlikely(!xsk_buff_valid_tx_metadata(pool, meta, &flags)))
> >  		return -EINVAL;
> >  
> > -	if (meta->flags & XDP_TXMD_FLAGS_CHECKSUM) {
> > +	if (flags & XDP_TXMD_FLAGS_CHECKSUM) {
> >  		csum_start = READ_ONCE(meta->request.csum_start);
> >  		csum_offset = READ_ONCE(meta->request.csum_offset);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but for zero-copy SKB
> construction (xsk_build_skb_zerocopy), the payload is populated strictly in
> fragments, leaving skb_headlen(skb) == 0.
> 
> When a user requests XDP TX metadata with checksum offload on a device
> that uses software checksum fallback (pool->tx_sw_csum),
> skb_checksum_help() fails immediately on such fragmented SKBs by returning
> -EINVAL, which drops the packet.
> 
> Does calling skb_checksum_help() later in this checksum block incorrectly
> assume the SKB has a linearized payload?

We treat skb path as a mostly test-fallback, so I think it is ok.
I'll add a headlen check just to give the users better signals..