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

On 07/28, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

I guess.. We can do a similar size check before calling skb_checksum_help
to avoid WARN_ONCE.