Re: [PATCH net 3/6] xsk: clear metadata pointer when no timestamp is requested

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Integer overflow in `xsk_skb_metadata` when calculating `skb->csum_start`, leading to a potential kernel warning and panic (DoS).
--

--- Patch [3]: [PATCH net 3/6] xsk: clear metadata pointer when no timestamp is requested ---
commit 814027e56069dbb5716ac5b9f7f38aebab332dd6
Author: Stanislav Fomichev <sdf.kernel@gmail.com>

xsk: clear metadata pointer when no timestamp is requested

User space can change metadata flags after request processing. Rereading
them during completion can therefore make the kernel write a timestamp
that was not requested when the packet was submitted. Clear the metadata
pointer during request processing unless timestamp completion is requested.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c

[ ... ]

> @@ -998,6 +998,8 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
>  
>  	if (meta->flags & XDP_TXMD_FLAGS_LAUNCH_TIME)
>  		skb->skb_mstamp_ns = meta->request.launch_time;

[Severity: High]
This is a pre-existing issue, but can the checksum offset calculation
earlier in this function cause an integer overflow?

When handling XDP_TXMD_FLAGS_CHECKSUM, the code does:

    skb->csum_start = hr + csum_start;

On architectures with 64KB pages, csum_start can be large enough that
adding hr wraps the u16 skb->csum_start. This would result in
skb->csum_start being smaller than the headroom.

If pool->tx_sw_csum is true, this then calls skb_checksum_help(). Inside
skb_checksum_help(), the offset is calculated by subtracting headroom from
skb->csum_start, which yields a negative integer due to the wrap-around.

When this negative offset is compared against skb_headlen() (which is
unsigned), it is implicitly cast to a huge unsigned value:

net/core/dev.c:skb_checksum_help() {
    ...
    offset = skb_checksum_start_offset(skb);
    ...
    if (unlikely(offset >= skb_headlen(skb))) {
        DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false);
        WARN_ONCE(true, "offset (%d) >= skb_headlen() (%u)\n", ...);
    ...
}

Could this allow an unprivileged user to trigger a kernel warning and
potentially panic the system if panic_on_warn is set?

> +	if (!(meta->flags & XDP_TXMD_FLAGS_TIMESTAMP))
> +		meta = NULL;
>  	xsk_tx_metadata_to_compl(meta, &skb_shinfo(skb)->xsk_meta);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727161959.885642-1-sdf@fomichev.me?part=3