mirror of
https://github.com/torvalds/linux.git
synced 2026-11-07 06:17:02 +01:00
Merge branch 'net-skbuff-replace-most-bug_on-s-with-error-returns'
Josef Bacik says: ==================== net: skbuff: replace most BUG_ON()s with error returns I'm going through and reducing BUG_ON() usage in areas that have created the most problems for us. 89 commits in the tree quote "kernel BUG at net/core/skbuff.c", 31 of them since 2024, and some of those could be triggered from inside a user namespace. The first patch is a fix: skb_copy_and_csum_bits() leaves stale bytes in a buffer headed for the wire when it hits unreadable frags. The BUG_ON() conversion of the same function needs the same handling, so it's here rather than sent separately. The rest of the series converts 17 of the 19 BUG_ON()s in skbuff.c. Each one becomes if (WARN_ON_ONCE(cond)) <error path>; where the error path is a failure return the function already has and its callers already handle: -EINVAL from pskb_expand_head() and skb_segment(), NULL from skb_copy(), 0 from skb_shift(), and so on. Each patch says what its error path is and why it's safe. Anybody who wants the old behaviour, syzbot included, gets it with panic_on_warn. A few don't have an obvious error return: - skb_copy_and_csum_bits() zeroes the part of the caller's buffer it couldn't fill instead of leaving stale bytes in it. - skb_copy_and_csum_dev() copies the frame without a checksum. It also now catches a csum_start before the head and a csum_offset past the end of the frame, which the BUG_ON() missed. - skb_shift()'s second check ran after the shift had been committed. It moves up to just before the commit, where nothing has changed yet, and returns 0 there. Two BUG_ON()s are left on purpose. __pskb_pull_tail() and skb_pull_rcsum() have callers that can't otherwise fail, so they don't check the return. Some of them would carry on and BUG() somewhere else, or push back a pull that never happened. Those need their callers fixed first and will come as separate series. skb_over_panic() and skb_under_panic() keep their BUG() as well; that's overflow hardening and should stay fatal. skbuff.o text on x86_64 defconfig grows by 114 bytes. The fast path takes the same branch it does today; the extra bytes are the error paths that BUG() used to replace. Testing: x86_64 defconfig with CONFIG_WERROR boots, and every patch builds net/core/skbuff.o on its own with allmodconfig, W=1 and CONFIG_DEBUG_NET. A test module drives 12 of the 17 converted BUG_ON()s with a bad argument or a malformed skb, skb_shift() through a test-only export. Each one warns once and returns its documented error, and the skbs are left alone. The same module covers the unreadable-frags fix. test_bpf's skb_segment tests pass. The other five were only reviewed: skb_crc32c() isn't built in defconfig, and the four skb_segment() layout checks need a crafted frag_list. ==================== Link: https://patch.msgid.link/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b@toxicpanda.com Signed-off-by: Jakub Kicinski <kuba@kernel.org>
This commit is contained in:
Jakub Kicinski
commit
ebe53e774d
|
|
@ -2201,7 +2201,12 @@ struct sk_buff *skb_copy(const struct sk_buff *skb, gfp_t gfp_mask)
|
|||
/* Set the tail pointer and length */
|
||||
skb_put(n, skb->len);
|
||||
|
||||
BUG_ON(skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len));
|
||||
if (unlikely(skb_copy_bits(skb, -headerlen, n->head,
|
||||
headerlen + skb->len))) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
kfree_skb(n);
|
||||
return NULL;
|
||||
}
|
||||
|
||||
skb_copy_header(n, skb);
|
||||
return n;
|
||||
|
|
@ -2303,9 +2308,15 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
|
|||
u8 *data;
|
||||
int i;
|
||||
|
||||
BUG_ON(nhead < 0);
|
||||
if (unlikely(nhead < 0)) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
return -EINVAL;
|
||||
}
|
||||
|
||||
BUG_ON(skb_shared(skb));
|
||||
if (unlikely(skb_shared(skb))) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
return -EINVAL;
|
||||
}
|
||||
|
||||
skb_zcopy_downgrade_managed(skb);
|
||||
|
||||
|
|
@ -2539,8 +2550,13 @@ struct sk_buff *skb_copy_expand(const struct sk_buff *skb,
|
|||
head_copy_off = newheadroom - head_copy_len;
|
||||
|
||||
/* Copy the linear header and data. */
|
||||
BUG_ON(skb_copy_bits(skb, -head_copy_len, n->head + head_copy_off,
|
||||
skb->len + head_copy_len));
|
||||
if (unlikely(skb_copy_bits(skb, -head_copy_len,
|
||||
n->head + head_copy_off,
|
||||
skb->len + head_copy_len))) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
kfree_skb(n);
|
||||
return NULL;
|
||||
}
|
||||
|
||||
skb_copy_header(n, skb);
|
||||
|
||||
|
|
@ -3603,7 +3619,10 @@ __wsum skb_checksum(const struct sk_buff *skb, int offset, int len, __wsum csum)
|
|||
}
|
||||
start = end;
|
||||
}
|
||||
BUG_ON(len);
|
||||
if (unlikely(len)) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
return 0;
|
||||
}
|
||||
|
||||
return csum;
|
||||
}
|
||||
|
|
@ -3698,7 +3717,13 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
|
|||
}
|
||||
start = end;
|
||||
}
|
||||
BUG_ON(len);
|
||||
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);
|
||||
return 0;
|
||||
}
|
||||
return csum;
|
||||
}
|
||||
EXPORT_SYMBOL(skb_copy_and_csum_bits);
|
||||
|
|
@ -3768,7 +3793,10 @@ u32 skb_crc32c(const struct sk_buff *skb, int offset, int len, u32 crc)
|
|||
}
|
||||
start = end;
|
||||
}
|
||||
BUG_ON(len);
|
||||
if (unlikely(len)) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
return 0;
|
||||
}
|
||||
|
||||
return crc;
|
||||
}
|
||||
|
|
@ -3889,7 +3917,10 @@ skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
|
|||
struct page *page;
|
||||
unsigned int offset;
|
||||
|
||||
BUG_ON(!from->head_frag && !hlen);
|
||||
if (unlikely(!from->head_frag && !hlen)) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
return -EFAULT;
|
||||
}
|
||||
|
||||
/* dont bother with small payloads */
|
||||
if (len <= skb_tailroom(to))
|
||||
|
|
@ -3959,7 +3990,18 @@ void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to)
|
|||
else
|
||||
csstart = skb_headlen(skb);
|
||||
|
||||
BUG_ON(csstart > skb_headlen(skb));
|
||||
if (unlikely(csstart < 0 || csstart > skb_headlen(skb) ||
|
||||
(skb->ip_summed == CHECKSUM_PARTIAL &&
|
||||
csstart + skb->csum_offset + sizeof(__sum16) >
|
||||
skb->len))) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
/* Send the frame without the checksum filled in, or send
|
||||
* zeroes if we can't even copy it.
|
||||
*/
|
||||
if (skb_copy_bits(skb, 0, to, skb->len))
|
||||
memset(to, 0, skb->len);
|
||||
return;
|
||||
}
|
||||
|
||||
skb_copy_from_linear_data(skb, to, csstart);
|
||||
|
||||
|
|
@ -4293,7 +4335,10 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
|
|||
int from, to, merge, todo;
|
||||
skb_frag_t *fragfrom, *fragto;
|
||||
|
||||
BUG_ON(shiftlen > skb->len);
|
||||
if (unlikely(shiftlen > skb->len)) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
return 0;
|
||||
}
|
||||
|
||||
if (skb_headlen(skb))
|
||||
return 0;
|
||||
|
|
@ -4373,6 +4418,14 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
|
|||
}
|
||||
}
|
||||
|
||||
/* The frags ran out before shiftlen did. Nothing has been committed
|
||||
* yet, so back out.
|
||||
*/
|
||||
if (unlikely(todo > 0)) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
return 0;
|
||||
}
|
||||
|
||||
/* Ready to "commit" this state change to tgt */
|
||||
skb_shinfo(tgt)->nr_frags = to;
|
||||
|
||||
|
|
@ -4390,8 +4443,6 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
|
|||
skb_shinfo(skb)->frags[to++] = skb_shinfo(skb)->frags[from++];
|
||||
skb_shinfo(skb)->nr_frags = to;
|
||||
|
||||
BUG_ON(todo > 0 && !skb_shinfo(skb)->nr_frags);
|
||||
|
||||
onlymerged:
|
||||
/* Most likely the tgt won't ever need its checksum anymore, skb on
|
||||
* the other hand might need it if it needs to be resent
|
||||
|
|
@ -4918,7 +4969,11 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
|
|||
|
||||
if (hsize <= 0 && i >= nfrags && skb_headlen(list_skb) &&
|
||||
(skb_headlen(list_skb) == len || sg)) {
|
||||
BUG_ON(skb_headlen(list_skb) > len);
|
||||
if (unlikely(skb_headlen(list_skb) > len)) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
err = -EINVAL;
|
||||
goto err;
|
||||
}
|
||||
|
||||
nskb = skb_clone(list_skb, GFP_ATOMIC);
|
||||
if (unlikely(!nskb))
|
||||
|
|
@ -4931,7 +4986,12 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
|
|||
pos += skb_headlen(list_skb);
|
||||
|
||||
while (pos < offset + len) {
|
||||
BUG_ON(i >= nfrags);
|
||||
if (unlikely(i >= nfrags)) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
kfree_skb(nskb);
|
||||
err = -EINVAL;
|
||||
goto err;
|
||||
}
|
||||
|
||||
size = skb_frag_size(frag);
|
||||
if (pos + size > offset + len)
|
||||
|
|
@ -5038,9 +5098,17 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
|
|||
skb_shinfo(nskb)->flags |= skb_shinfo(frag_skb)->flags & SKBFL_SHARED_FRAG;
|
||||
|
||||
if (!skb_headlen(list_skb)) {
|
||||
BUG_ON(!nfrags);
|
||||
if (unlikely(!nfrags)) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
err = -EINVAL;
|
||||
goto err;
|
||||
}
|
||||
} else {
|
||||
BUG_ON(!list_skb->head_frag);
|
||||
if (unlikely(!list_skb->head_frag)) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
err = -EINVAL;
|
||||
goto err;
|
||||
}
|
||||
|
||||
/* to make room for head_frag. */
|
||||
i--;
|
||||
|
|
@ -5314,7 +5382,10 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len,
|
|||
}
|
||||
start = end;
|
||||
}
|
||||
BUG_ON(len);
|
||||
if (unlikely(len)) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
return -EINVAL;
|
||||
}
|
||||
return elt;
|
||||
}
|
||||
|
||||
|
|
@ -6222,8 +6293,15 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
|
|||
return false;
|
||||
|
||||
if (len <= skb_tailroom(to) && skb_frags_readable(from)) {
|
||||
if (len)
|
||||
BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
|
||||
if (len) {
|
||||
if (unlikely(skb_copy_bits(from, 0,
|
||||
skb_tail_pointer(to),
|
||||
len))) {
|
||||
DEBUG_NET_WARN_ON_ONCE(1);
|
||||
return false;
|
||||
}
|
||||
skb_put(to, len);
|
||||
}
|
||||
*delta_truesize = 0;
|
||||
return true;
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user