Re: [PATCH net-next 08/15] net: skbuff: make skb_try_coalesce()'s len unsigned

[netdev-bot+sashiko@xxxxxxxxxx]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message gets the bug's history wrong, so the single
  Fixes: tag (279877d86c82) is incomplete.

--

net: skbuff: make skb_try_coalesce()'s len unsigned

This changes the local len in skb_try_coalesce() from int to unsigned int,
so a from->len above INT_MAX can no longer pass the skb_tailroom() check.
It also adds an explicit delta < 0 test to the truesize WARN_ON_ONCE(),
since that comparison is now unsigned.

Link: 
https://lore.kernel.org/all/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b@xxxxxxxxxxxxxx/

> Before that commit skb_put() ran first,
> and its overflow check caught such a length before anything was
> copied.

[Severity: Low]

Is this accurate on all configurations? Before 279877d86c82,
skb_try_coalesce() looked like this:

        if (len <= skb_tailroom(to) && skb_frags_readable(from)) {
                if (len)
                        BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));

len was already a signed int at that point. A from->len above INT_MAX
passed the tailroom check then, the same way it does now.

skb_put() then does:

        skb->tail += len;
        skb->len  += len;
        if (unlikely(skb->tail > skb->end))
                skb_over_panic(skb, len, __builtin_return_address(0));

On 32-bit, sk_buff_data_t is unsigned char *. A lowmem tail at or above
0x80000000 plus a len at or above 0x80000000 wraps below end, so
skb_over_panic() is never called. skb_put() returns the old tail, and
skb_copy_bits() runs with the negative len. That is the same large write
past @to that the commit message attributes to 279877d86c82.

On 64-bit, tail and end are u32 offsets. A from->len in the range
[2^32 - tail, 2^32) also wraps tail below end and gets past the check.
279877d86c82 seems to have widened this window on 64-bit, but it did not
open it.

git blame shows that the signed len goes back to the introduction of the
function:

  bad43ca8325f4 ("net: introduce skb_try_coalesce()")

Could the commit message be corrected? Should a Fixes: tag for
bad43ca8325f4 also be added? Without it, stable trees that don't contain
279877d86c82, including their 32-bit builds, may not get this fix.

> Fixes: 279877d86c82 ("net: skbuff: don't BUG() when skb_copy_bits() fails in 
> copy helpers")

-- 
Sashiko AI review · 
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-b4-skbuff-bug-on-followups-v1-0-41dbef50b9f7%40toxicpanda.com