[PATCH net-next v3 10/15] net: skbuff: reject a negative length in skb_copy_and_csum_bits()

[Josef Bacik <josef@xxxxxxxxxxxxxx>]

The head, frag and frag_list copies in skb_copy_and_csum_bits() each
clamp their length to @len.  A negative @len from a broken caller
therefore becomes the copy length, and the copy takes it as a huge
unsigned value.  The two len > 0 tests around the zero-fills only
protect the zero-fills.

Reject a negative @len on entry with -EINVAL and drop the two tests.
@to isn't touched in that case.  Every caller already handles an error,
and skb_copy_and_csum_dev() zeroes its whole frame on one.  No caller is
known to pass a negative length.

Found by the Sashiko AI review of the BUG_ON() series.

Link: 
https://lore.kernel.org/all/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b@xxxxxxxxxxxxxx/
Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
---
 net/core/skbuff.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b74d8a6ef0f9..e3fc6291d223 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3731,8 +3731,7 @@ static noinline int skb_copy_and_csum_frag_list(const 
struct sk_buff *skb,
        if (unlikely(len)) {
                DEBUG_NET_WARN_ON_ONCE(1);
                /* Don't hand the caller a buffer with stale bytes in it. */
-               if (len > 0)
-                       memset(to, 0, len);
+               memset(to, 0, len);
                return -EFAULT;
        }
 out:
@@ -3748,9 +3747,10 @@ static noinline int skb_copy_and_csum_frag_list(const 
struct sk_buff *skb,
  *     @len: number of bytes to copy
  *     @csump: where to store the checksum of the copied bytes
  *
- *     Return: 0 on success, or -EFAULT if @skb has unreadable frags or runs
- *     out before @len does.  On failure *@csump is not set and the part of
- *     @to that couldn't be filled is zeroed.
+ *     Return: 0 on success, -EINVAL if @len is negative, or -EFAULT if @skb
+ *     has unreadable frags or runs out before @len does.  On failure *@csump
+ *     is not set.  On -EFAULT the part of @to that couldn't be filled is
+ *     zeroed, and on -EINVAL @to is not touched.
  */
 int skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
                           u8 *to, int len, __wsum *csump)
@@ -3760,6 +3760,14 @@ int skb_copy_and_csum_bits(const struct sk_buff *skb, 
int offset,
        int pos = 0;
        __wsum csum = 0;
 
+       /* Everything below clamps its copy to @len, so a negative one would
+        * turn into a huge copy.
+        */
+       if (unlikely(len < 0)) {
+               DEBUG_NET_WARN_ON_ONCE(1);
+               return -EINVAL;
+       }
+
        /* Copy header. */
        if (copy > 0) {
                if (copy > len)
@@ -3775,8 +3783,7 @@ int skb_copy_and_csum_bits(const struct sk_buff *skb, int 
offset,
 
        if (!skb_frags_readable(skb)) {
                /* Don't hand the caller a buffer with stale bytes in it. */
-               if (len > 0)
-                       memset(to, 0, len);
+               memset(to, 0, len);
                return -EFAULT;
        }
 

-- 
2.55.0