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@toxicpanda.com/ (local)
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
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