Re: [PATCH net-next 01/15] net: skbuff: make skb_copy_and_csum_bits() report unreadable frags

[netdev-bot+sashiko@xxxxxxxxxx]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Missing Fixes: and Cc: stable tags on a data-integrity fix.
- [Low] The new comment in skb_copy_and_csum_dev() reads: "On failure the
  rest of @to is zeroed; leave the checksum alone so the frame goes out…

--

net: skbuff: make skb_copy_and_csum_bits() report unreadable frags

skb_copy_and_csum_bits() now returns 0 or -EFAULT, hands the checksum
back through a pointer, and is marked __must_check. On error, the callers
in skb_segment(), the ICMP code, the IP append paths, sunvnet and sunrpc
take the failure paths they already have. skb_copy_and_csum_dev() returns
early without storing a checksum. With this change, software GSO of a
devmem TCP skb fails instead of silently replacing the payload with
zeroes.

Link: 
https://lore.kernel.org/all/179148235783.434549.14322374227477832817@xxxxxxxxxx/

> Link: 
> https://lore.kernel.org/all/179148235783.434549.14322374227477832817@xxxxxxxxxx/
> Suggested-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> Reviewed-by: Mina Almasry <almasrymina@xxxxxxxxxx>
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>

[Severity: Medium]
This fixes the silent payload replacement in the !sg && !csum branch of
skb_segment(), but there is no Fixes: or Cc: stable tag.

The commit message says commit ab9414ed70bd ("net: skbuff: don't leave
stale bytes in skb_copy_and_csum_bits()") is what made the replacement
silent. That commit carries:

    Fixes: 65249feb6b3d ("net: add support for skbs with unreadable frags")
    Cc: stable@xxxxxxxxxxxxxxx

Could stable trees take ab9414ed70bd without this patch? Devmem TX would
then send a zeroed payload under a valid TCP checksum, and the peer would
accept it.

Other patches in this series carry Fixes: tags, for example "net: skbuff:
reject a negative length in skb_copy_and_csum_bits()" and "net: skbuff:
check csum_start against len in skb_copy_and_csum_dev()".

Should this one also name ab9414ed70bd (or 65249feb6b3d) in a Fixes: tag
and Cc stable, so that both changes are backported together?

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 93c2f440b9ec..a2b52a5a61a9 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c

[ ... ]

> @@ -4006,9 +4025,13 @@ void skb_copy_and_csum_dev(const struct sk_buff *skb, 
> u8 *to)
>       skb_copy_from_linear_data(skb, to, csstart);
>  
>       csum = 0;
> -     if (csstart != skb->len)
> -             csum = skb_copy_and_csum_bits(skb, csstart, to + csstart,
> -                                           skb->len - csstart);
> +     /* On failure the rest of @to is zeroed; leave the checksum alone so
> +      * the frame goes out with a bad one.
> +      */
> +     if (csstart != skb->len &&
> +         skb_copy_and_csum_bits(skb, csstart, to + csstart,
> +                                skb->len - csstart, &csum))
> +             return;

[Severity: Low]
Does this early return actually make the frame go out with a bad
checksum?

It skips the csum_fold() store, but nothing marks the checksum as invalid.
The sanity check earlier in skb_copy_and_csum_dev() only bounds csstart
by skb_headlen():

        if (unlikely(csstart < 0 || csstart > skb_headlen(skb) ||
                     (skb->ip_summed == CHECKSUM_PARTIAL &&
                      csstart + skb->csum_offset + sizeof(__sum16) >
                      skb->len))) {

For CHECKSUM_PARTIAL, the field at csstart + csum_offset can therefore sit
past the linear area. That puts it inside the region that
skb_copy_and_csum_bits() zero-fills on failure:

        if (!skb_frags_readable(skb)) {
                /* Don't hand the caller a buffer with stale bytes in it. */
                if (len > 0)
                        memset(to, 0, len);
                return -EFAULT;
        }

For UDP over IPv4, a checksum of 0x0000 means "no checksum". Wouldn't the
receiver then accept the zeroed payload?

The same applies to a CHECKSUM_NONE UDP skb sent without a checksum. The
headers are copied intact, only the payload is zeroed, and nothing marks
the frame as bad.

The commit message makes the same claim:

    skb_copy_and_csum_dev() can't return an error, so it leaves the
    checksum alone and the frame goes out with a bad one.

None of the current callers (ep93xx_eth, ec_bhf, ksz884x, 8139too,
sc92031, via-rhine) sets netmem_tx. validate_xmit_unreadable_skb()
therefore drops unreadable skbs before they reach these drivers, and the
remaining failure case needs an skb whose lengths are inconsistent.

Should the comment and the commit message be narrowed? Alternatively,
this path could store a non-zero value at csstuff that is known not to
match, for CHECKSUM_PARTIAL. The code is unchanged at the end of the
series.

>  
>       if (skb->ip_summed == CHECKSUM_PARTIAL) {
>               long csstuff = csstart + skb->csum_offset;

[ ... ]

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