Re: [PATCH net-next 14/15] net/tls: don't BUG() on leftover length in __skb_nsg()

[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] In net/tls/tls_sw.c, the comment above skb_nsg() is now out of
  date.

--

net/tls: don't BUG() on leftover length in __skb_nsg()

This patch replaces the BUG_ON(len) in __skb_nsg() with a
DEBUG_NET_WARN_ON_ONCE() and an -EINVAL return. A range that runs past
the end of the skb now fails the record in tls_decrypt_sg() instead of
crashing the kernel.

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 312e51270f29..1c578ea47a1b 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -142,7 +142,10 @@ static int __skb_nsg(struct sk_buff *skb, int offset, 
> int len,
>                          start = end;
>                  }
>          }
> -        BUG_ON(len);
> +     if (unlikely(len)) {
> +             DEBUG_NET_WARN_ON_ONCE(1);
> +             return -EINVAL;
> +     }
>          return elt;
>  }
>  

[Severity: Low]
This isn't a bug, but should the comment above skb_nsg() be updated to
cover the new return value? The comment directly below this hunk still
says:

net/tls/tls_sw.c:
/* Return the number of scatterlist elements required to completely map the
 * skb, or -EMSGSIZE if the recursion depth is exceeded.
 */
static int skb_nsg(struct sk_buff *skb, int offset, int len)
{
        return __skb_nsg(skb, offset, len, 0);
}

skb_nsg() returns whatever __skb_nsg() returns. The recursive frag_list
calls also pass negative values straight up:

    if (unlikely(ret < 0))
            return ret;

So skb_nsg() can now return -EINVAL when the skb runs out before the
requested range does.

The only caller, tls_decrypt_sg(), already handles any negative value:

    if (n_sgin < 1)
            return n_sgin ?: -EBADMSG;

So this doesn't change behavior at runtime.

The comment is also still unchanged at the end of the series, including
after the last patch, "net: update comments that still describe the skb
BUG_ON()s".

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