__skb_checksum_complete() and __skb_gro_checksum_complete() verify a packet with skb_checksum(), which returns 0 when the payload is in unreadable frags or the skb's lengths don't add up. Both add that 0 to the pseudo-header sum, so they accept the packet whenever the pseudo-header sum folds to 0xffff. The sender controls that sum through the addresses and the length. Use skb_checksum_full() in both, and in __skb_checksum_complete_head(), and report an skb that can't be checksummed as having a bad checksum. Also take the skb off CHECKSUM_COMPLETE on that path. nf_ip_checksum() stores the pseudo-header sum in skb->csum before it calls __skb_checksum_complete(), and conntrack doesn't drop a packet with a bad checksum. If the skb stayed CHECKSUM_COMPLETE, __skb_checksum_validate_complete() would add the same pseudo-header sum to skb->csum and accept the packet under the same condition. A test module builds an skb that claims 50 more bytes than it holds, with a pseudo-header sum of 0xffff. Both functions accept that skb before this patch and reject it after. Found by the Sashiko AI review of the BUG_ON() series. Fixes: 20ddd0fd2fcd ("net: skbuff: don't BUG() on leftover length in skb_checksum() and friends") Fixes: 65249feb6b3d ("net: add support for skbs with unreadable frags") Link: https://lore.kernel.org/all/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b@toxicpanda.com/ Link: https://lore.kernel.org/all/179273111951.3134.3452028799437573017@kernel.org/ Signed-off-by: Josef Bacik --- include/linux/skbuff.h | 4 ++-- net/core/gro.c | 11 +++++++++-- net/core/skbuff.c | 27 +++++++++++++++++++++++---- 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 608c1491597e..5a5354a6b6ba 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -4892,7 +4892,7 @@ static inline void skb_checksum_complete_unset(struct sk_buff *skb) * 0: checksum is validated or try to in skb_checksum_complete. In the latter * case the ip_summed will not be CHECKSUM_UNNECESSARY and the pseudo * checksum is stored in skb->csum for use in __skb_checksum_complete - * non-zero: value of invalid checksum + * non-zero: value of invalid checksum, or 1 if the skb can't be checksummed * */ static inline __sum16 __skb_checksum_validate_complete(struct sk_buff *skb, @@ -4932,7 +4932,7 @@ static inline __wsum null_compute_pseudo(struct sk_buff *skb, int proto) * * Return values: * 0: checksum is validated or try to in skb_checksum_complete - * non-zero: value of invalid checksum + * non-zero: value of invalid checksum, or 1 if the skb can't be checksummed */ #define __skb_checksum_validate(skb, proto, complete, \ zero_okay, check, compute_pseudo) \ diff --git a/net/core/gro.c b/net/core/gro.c index 1b033855a20d..e2e847c95270 100644 --- a/net/core/gro.c +++ b/net/core/gro.c @@ -798,10 +798,17 @@ EXPORT_SYMBOL(napi_gro_frags); */ __sum16 __skb_gro_checksum_complete(struct sk_buff *skb) { - __wsum wsum; + __wsum wsum = 0; __sum16 sum; - wsum = skb_checksum(skb, skb_gro_offset(skb), skb_gro_len(skb), 0); + /* If the data can't be checksummed it can't be valid either, and + * NAPI_GRO_CB(skb)->csum only holds the pseudo checksum. + */ + if (skb_checksum_full(skb, skb_gro_offset(skb), skb_gro_len(skb), + &wsum)) { + NAPI_GRO_CB(skb)->csum_valid = 0; + return (__force __sum16)1; + } /* NAPI_GRO_CB(skb)->csum holds pseudo checksum */ sum = csum_fold(csum_add(NAPI_GRO_CB(skb)->csum, wsum)); diff --git a/net/core/skbuff.c b/net/core/skbuff.c index c5f8aca89221..0d3f0f09c7d1 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -3885,11 +3885,28 @@ u32 skb_crc32c(const struct sk_buff *skb, int offset, int len, u32 crc) EXPORT_SYMBOL(skb_crc32c); #endif /* CONFIG_NET_CRC32C */ +/* The data can't be checksummed, so it can't be valid either. The caller has + * put the pseudo header's checksum in skb->csum, so don't leave the skb + * CHECKSUM_COMPLETE for a later validator to take that for the packet's. + */ +static __sum16 skb_checksum_complete_failed(struct sk_buff *skb) +{ + if (!skb_shared(skb)) { + skb_checksum_complete_unset(skb); + skb->csum_valid = 0; + } + return (__force __sum16)1; +} + __sum16 __skb_checksum_complete_head(struct sk_buff *skb, int len) { + __wsum csum = skb->csum; __sum16 sum; - sum = csum_fold(skb_checksum(skb, 0, len, skb->csum)); + if (skb_checksum_full(skb, 0, len, &csum)) + return skb_checksum_complete_failed(skb); + + sum = csum_fold(csum); /* See comments in __skb_checksum_complete(). */ if (likely(!sum)) { if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) && @@ -3909,14 +3926,16 @@ EXPORT_SYMBOL(__skb_checksum_complete_head); * * It returns non-zero if the recomputed checksum is still invalid, otherwise * zero. The new checksum is stored back into skb->csum unless the skb is - * shared. + * shared. If the skb can't be checksummed it returns non-zero, skb->csum is + * left alone, and the skb is no longer CHECKSUM_COMPLETE unless it's shared. */ __sum16 __skb_checksum_complete(struct sk_buff *skb) { - __wsum csum; + __wsum csum = 0; __sum16 sum; - csum = skb_checksum(skb, 0, skb->len, 0); + if (skb_checksum_full(skb, 0, skb->len, &csum)) + return skb_checksum_complete_failed(skb); sum = csum_fold(csum_add(skb->csum, csum)); /* This check is inverted, because we already knew the hardware -- 2.55.0