Re: [PATCH 1/2 net] ip_gre: add validation for csum_start
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2021-09-01 13:47:37
Also in:
lkml
On Wed, Sep 1, 2021 at 7:53 AM Ido Schimmel [off-list ref] wrote:
quoted hunk ↗ jump to hunk
On Sat, Aug 21, 2021 at 09:41:14AM -0400, Willem de Bruijn wrote:quoted
On Sat, Aug 21, 2021 at 3:14 AM Shreyansh Chouhan [off-list ref] wrote:quoted
Validate csum_start in gre_handle_offloads before we call _gre_xmit so that we do not crash later when the csum_start value is used in the lco_csum function call. This patch deals with ipv4 code. Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.") Reported-by: syzbot+ff8e1b9f2f36481e2efc@syzkaller.appspotmail.com Signed-off-by: Shreyansh Chouhan <redacted>Reviewed-by: Willem de Bruijn <willemb@google.com>Hi Shreyansh, Willem, I bisected packet drops with a GRE tunnel to this patch. With the following debug patch [1], I'm getting this output [2]. Tested with IPv4 underlay only, but I assume problem exists with ip6gre as well. Thanks [1]diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index 177d26d8fb9c..cf4e13db030b 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c@@ -473,8 +473,11 @@ static void __gre_xmit(struct sk_buff *skb, struct net_device *dev, static int gre_handle_offloads(struct sk_buff *skb, bool csum) { - if (csum && skb_checksum_start(skb) < skb->data) + if (csum && skb_checksum_start(skb) < skb->data) { + if (net_ratelimit()) + skb_dump(KERN_WARNING, skb, false); return -EINVAL; + } return iptunnel_handle_offloads(skb, csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE); }[2] skb len=84 headroom=78 headlen=84 tailroom=15902 mac=(78,0) net=(78,20) trans=98 shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0)) csum(0x0 ip_summed=0 complete_sw=0 valid=0 level=0) hash(0x0 sw=0 l4=0) proto=0x0800 pkttype=0 iif=32 dev name=g1a feat=0x0x00000006401d5869 skb linear: 00000000: 45 00 00 54 be 12 40 00 3f 01 f9 82 c0 00 02 01 skb linear: 00000010: c0 00 02 12 08 00 fe ad 8c 39 00 01 7c 65 2f 61 skb linear: 00000020: 00 00 00 00 f8 7d 0a 00 00 00 00 00 10 11 12 13 skb linear: 00000030: 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 skb linear: 00000040: 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 skb linear: 00000050: 34 35 36 37
Thanks for the detailed report, Ido.
This is a gre tunnel device with csum/ocsum enabled, correct?
How was this packet generated: does it come from the local stack or is
it a custom packet injected from userspace, e.g., with a packet socket
with vnet_hdr?
The bug that this patch intended to protect against only occurs with
ip_summed == CHECKSUM_PARTIAL (3):
if (skb->ip_summed == CHECKSUM_PARTIAL) {
*(__sum16 *)ptr = csum_fold(lco_csum(skb));
} else {
skb->ip_summed = CHECKSUM_PARTIAL;
skb->csum_start =
skb_transport_header(skb) - skb->head;
skb->csum_offset = sizeof(*greh);
}
So this packet would not hit that code anyway, as it has ip_summed
CHECKSUM_NONE (0), which computes the offsets manually.
Perhaps the check needs to be refined. But I'd like to also understand
how to reproduce this / how common this false positive is.