Re: [PATCH net 1/6] xsk: require at least 16 bytes of TX metadata

On 07/28, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] Incomplete Fix / Out-of-Bounds Memory Read in TX Metadata
> - [High] UAPI ABI Incompatibility between 32-bit and 64-bit architectures
> --
> 
> --- Patch [1]: [PATCH net 1/6] xsk: require at least 16 bytes of TX metadata ---
> commit 8f587b21f2b30dd32c9e63e0817476297538dfb0
> Author: Stanislav Fomichev <[email protected]>
> xsk: require at least 16 bytes of TX metadata
> 
> AF_XDP accepts a TX metadata length as small as eight bytes. This commit
> requires a minimum of 16 bytes of TX metadata to preserve compatibility with
> applications that do not use launch-time metadata while addressing
> out-of-bounds reads.
> 
> Link: https://lore.kernel.org/netdev/[email protected]/
> 
> > diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> > index 58da2f4f4397a..cd8643360eb3a 100644
> > --- a/net/xdp/xdp_umem.c
> > +++ b/net/xdp/xdp_umem.c
> > @@ -210,6 +210,8 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
> >  	if (mr->flags & XDP_UMEM_TX_METADATA_LEN) {
> >  		if (mr->tx_metadata_len >= 256 || mr->tx_metadata_len % 8)
> >  			return -EINVAL;
> > +		if (mr->tx_metadata_len < 16)
> > +			return -EINVAL;
> 
> [Severity: Critical]
> This is a pre-existing issue, but is a 16-byte minimum sufficient when the
> launch_time flag is requested?
> 
> If a userspace application sets XDP_TXMD_FLAGS_LAUNCH_TIME in the metadata
> flags and provides a tx_metadata_len of 16, it appears we can still read out
> of bounds. In include/net/xdp_sock.h:xsk_tx_metadata_request(), the code
> reads the 8-byte launch_time field at offset 16 when the flag is set:
> 
> include/net/xdp_sock.h:xsk_tx_metadata_request() {
>     ...
>     ops->tmo_request_launch_time(meta->request.launch_time, priv);
>     ...
> }
> 
> If the descriptor addr is placed at the very end of the registered umem (for
> example, desc->addr = umem_size - 1 with len = 1), reading 8 bytes at offset
> 16 extends beyond the umem bounds. Since xp_aligned_validate_desc() validates
> based on the chunk, does this allow reading into potentially unmapped memory
> if the launch_time flag is set?

Lanuch time is handled separately later in the series:

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

> [Severity: High]
> This isn't a bug introduced by this patch, but does this tx_metadata_len
> validation correctly handle 32-bit applications running on a 64-bit kernel?
> 
> Looking at struct xsk_tx_metadata in include/uapi/linux/if_xdp.h, there is no
> explicit padding between csum_offset and launch_time:
> 
> include/uapi/linux/if_xdp.h:struct xsk_tx_metadata {
>     __u64 flags;
>     union {
>         struct {
>             __u16 csum_start;
>             __u16 csum_offset;
>             __u64 launch_time;
>         } request;
>         ...
>     };
> };
> 
> On 32-bit x86, __u64 has a 4-byte alignment, which places launch_time at
> offset 12 and makes the total structure size 20 bytes. On a 64-bit kernel,
> __u64 is 8-byte aligned, placing launch_time at offset 16 with a 24-byte
> structure size.
> 
> If a 32-bit application registers a 20-byte length, it would be rejected by
> the tx_metadata_len % 8 check above. If it registers 24 bytes but populates
> launch_time at its native offset 12, would the 64-bit kernel read unexpected
> data from offset 16?

Noted, I don't think we test 32 bit arches, the sensible thing is to
probably add some padding (potentially breaking existing, if any, 32
bit apps). Will put it on the todo.