Re: [PATCH net-next 09/15] net: skbuff: check csum_start against len in skb_copy_and_csum_dev()

[netdev-bot+sashiko@xxxxxxxxxx]

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

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

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] net: skbuff: incomplete headlen underflow check in
  skb_copy_and_csum_dev()

--

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4103,7 +4103,11 @@ void skb_copy_and_csum_dev(const struct sk_buff *skb, 
> u8 *to)
>       else
>               csstart = skb_headlen(skb);
>  
> +     /* skb_headlen() underflows if data_len is bigger than len, so check
> +      * csstart against len as well.
> +      */
>       if (unlikely(csstart < 0 || csstart > skb_headlen(skb) ||
> +                  csstart > skb->len ||
>                    (skb->ip_summed == CHECKSUM_PARTIAL &&
>                     csstart + skb->csum_offset + sizeof(__sum16) >
>                     skb->len))) {
>

[Severity: Medium]
This only closes the hole for the non-CHECKSUM_PARTIAL case. If
skb->data_len > skb->len and ip_summed is CHECKSUM_PARTIAL, csstart
comes from skb_checksum_start_offset() instead of skb_headlen(), so it
can be anywhere up to skb->len while skb_headlen() has wrapped to
about 4GB. Both the headlen comparison and the new skb->len comparison
then pass, the csum_offset check passes too, and we go straight on to
skb_copy_from_linear_data(skb, to, csstart) with nothing bounding
csstart by the real linear area.

skb_copy_and_csum_bits() then starts from int start =
skb_headlen(skb), which is either negative, so the linear part is
skipped and the frag reads are shifted by data_len - len bytes, or,
once the difference exceeds INT_MAX, a bogus positive value that lets
it copy from skb->data past the end of the linear data.

The fallback isn't safe either. skb_copy_bits() also begins with
int start = skb_headlen(skb), so sending such an skb down the existing
fallback, as the changelog describes, bounds the writes to @to by
skb->len but not the reads from the skb.

Since the condition you really want to catch is data_len > len,
would it be simpler to test that directly before computing csstart,
and memset() @to rather than calling skb_copy_bits() in that case? Or
should skb_copy_bits() and skb_copy_and_csum_bits() reject
data_len > len themselves?

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