[PATCH] ip_gre/ip6_gre: add check for invalid csum_start

If we get a ip gre packet with TUNNEL_CSUM set, an invalid csum_start
value causes skb->csum_start offset to be less than the offset for
skb->data after we pull the ip header from the packet during the
ipgre_xmit call.

This patch adds a sanity check to gre_handle_offloads, which checks the
validity of skb->csum_start after we have pulled the ip header from the
packet in the ipgre_xmit call.

Reported-by: syzbot+ff8e1b9f2f36481e2efc@syzkaller.appspotmail.com
Signed-off-by: Shreyansh Chouhan <redacted>
---
 net/ipv4/ip_gre.c  | 2 ++
 net/ipv6/ip6_gre.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 12dca0c85f3c..95419b7adf5c 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c

@@ -473,6 +473,8 @@ 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)
+		return -EINVAL;
 	return iptunnel_handle_offloads(skb, csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE);
 }
 

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index bc224f917bbd..7a5e90e09363 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c

@@ -629,6 +629,8 @@ static int gre_rcv(struct sk_buff *skb)
 
 static int gre_handle_offloads(struct sk_buff *skb, bool csum)
 {
+	if (csum && skb_checksum_start(skb) < skb->data)
+		return -EINVAL;
 	return iptunnel_handle_offloads(skb,
 					csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE);
 }

-- 
2.31.1

On Thu, Aug 19, 2021 at 10:35 AM Shreyansh Chouhan
[off-list ref] wrote:

If we get a ip gre packet with TUNNEL_CSUM set, an invalid csum_start
value causes skb->csum_start offset to be less than the offset for
skb->data after we pull the ip header from the packet during the
ipgre_xmit call.

This patch adds a sanity check to gre_handle_offloads, which checks the
validity of skb->csum_start after we have pulled the ip header from the
packet in the ipgre_xmit call.

Reported-by: syzbot+ff8e1b9f2f36481e2efc@syzkaller.appspotmail.com
Signed-off-by: Shreyansh Chouhan <redacted>

For the ipv4 portion:

Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")

For the ipv6 portion:

Fixes: b05229f44228 ("gre6: Cleanup GREv6 transmit path, call common
GRE functions")

It's possible that a similar bug exists before those, but the patch
wouldn't apply anyway.

Technically, for backporting purposes, the patch needs to be split
into two, each with their own Fixes tag. And target [PATCH net]

On Thu, 19 Aug 2021 12:56:59 -0400 Willem de Bruijn wrote:

Technically, for backporting purposes, the patch needs to be split
into two, each with their own Fixes tag. And target [PATCH net]

Indeed, looks like the two parts will need to go to different
depths of stable - Shreyansh, would you mind splitting and 
reposting as instructed? Thanks.

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>
---
 net/ipv4/ip_gre.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 12dca0c85f3c..95419b7adf5c 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c

@@ -473,6 +473,8 @@ 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)
+		return -EINVAL;
 	return iptunnel_handle_offloads(skb, csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE);
 }
 

-- 
2.31.1

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 ipv6 code.

Fixes: Fixes: b05229f44228 ("gre6: Cleanup GREv6 transmit path, call common
GRE functions")
Reported-by: syzbot+ff8e1b9f2f36481e2efc@syzkaller.appspotmail.com
Signed-off-by: Shreyansh Chouhan <redacted>
---
 net/ipv6/ip6_gre.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index bc224f917bbd..7a5e90e09363 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c

@@ -629,6 +629,8 @@ static int gre_rcv(struct sk_buff *skb)
 
 static int gre_handle_offloads(struct sk_buff *skb, bool csum)
 {
+	if (csum && skb_checksum_start(skb) < skb->data)
+		return -EINVAL;
 	return iptunnel_handle_offloads(skb,
 					csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE);
 }

-- 
2.31.1

Hi,

Thank you Jakub and Willem for your reviews. I have separated the
changes into two differnet patches. Sorry for the delay.

Where can I read about patch targets? I have seen patches with differnet
targets but I do not know what they mean/how they work. I was not able
to find the documentation for these.

Thank you,
Shreyansh Chouhan

On Sat, Aug 21, 2021 at 3:14 AM Shreyansh Chouhan
[off-list ref] wrote:

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>

On Sat, Aug 21, 2021 at 3:14 AM Shreyansh Chouhan
[off-list ref] wrote:

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 ipv6 code.

Fixes: Fixes: b05229f44228 ("gre6: Cleanup GREv6 transmit path, call common
GRE functions")
Reported-by: syzbot+ff8e1b9f2f36481e2efc@syzkaller.appspotmail.com
Signed-off-by: Shreyansh Chouhan <redacted>

Reviewed-by: Willem de Bruijn <willemb@google.com>

On Sat, Aug 21, 2021 at 3:18 AM Shreyansh Chouhan
[off-list ref] wrote:

Hi,

Thank you Jakub and Willem for your reviews. I have separated the
changes into two differnet patches. Sorry for the delay.

Thanks Shreyansh

Where can I read about patch targets? I have seen patches with differnet
targets but I do not know what they mean/how they work. I was not able
to find the documentation for these.

Targeting these bug fixed to net was the right destination.
Documentation/networking/netdev-FAQ.rst has more context on the net vs
net-next distinction.

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Sat, 21 Aug 2021 12:44:24 +0530 you wrote:

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>

[...]

Here is the summary with links:
  - [1/2,net] ip_gre: add validation for csum_start
    https://git.kernel.org/netdev/net/c/1d011c4803c7
  - [2/2,net] ip6_gre: add validation for csum_start
    https://git.kernel.org/netdev/net/c/9cf448c200ba

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

On Sat, Aug 21, 2021 at 09:41:14AM -0400, Willem de Bruijn wrote:

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

On Wed, Sep 1, 2021 at 7:53 AM Ido Schimmel [off-list ref] wrote:

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.

Thanks for the quick reply, Willem.

On Wed, Sep 01, 2021 at 09:46:48AM -0400, Willem de Bruijn wrote:

Thanks for the detailed report, Ido.

This is a gre tunnel device with csum/ocsum enabled, correct?

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 packet is received by a physical port and injected to the kernel's
Rx path by mlxsw (which does not support checksumming). The IPv4 routing
code then forwards the packet to the GRE tunnel.

I was able to reproduce the issue using veth pairs and a packet socket
[1]. Running the reproducer with the debug patch from before, I get the
following output [2].

[1]
#!/bin/bash

ip link add name veth0 type veth peer name veth1
ip link add name veth2 type veth peer name veth3
ip link add name veth4 type veth peer name veth5

ip netns add h1
ip netns add r1
ip netns add r2
ip netns add h2

# h1
ip -n h1 link set dev lo up
ip link set dev veth0 netns h1
ip -n h1 link set dev veth0 up
ip -n h1 address add 192.0.2.1/28 dev veth0
ip -n h1 route add default via 192.0.2.2

# r1
## underlay
ip netns exec r1 sysctl -wq net.ipv4.conf.all.forwarding=1
ip -n r1 link set dev lo up
ip -n r1 address add 1.1.1.1/32 dev lo
ip link set dev veth1 netns r1
ip link set dev veth2 netns r1
ip -n r1 link set dev veth1 up
ip -n r1 link set dev veth2 up
ip -n r1 address add 192.0.2.2/28 dev veth1
ip -n r1 address add 192.0.2.17/28 dev veth2
ip -n r1 route add 2.2.2.2/32 via 192.0.2.18
## overlay
ip -n r1 tunnel add name gre2 mode gre local 1.1.1.1 remote 2.2.2.2 csum
ip -n r1 link set dev gre2 up
ip -n r1 route add 192.0.2.34/32 dev gre2

# r2
## underlay
ip netns exec r2 sysctl -wq net.ipv4.conf.all.forwarding=1
ip -n r2 link set dev lo up
ip -n r2 address add 2.2.2.2/32 dev lo
ip link set dev veth3 netns r2
ip link set dev veth4 netns r2
ip -n r2 link set dev veth3 up
ip -n r2 link set dev veth4 up
ip -n r2 address add 192.0.2.18/28 dev veth3
ip -n r2 address add 192.0.2.33/28 dev veth4
ip -n r2 route add 1.1.1.1/32 via 192.0.2.17
## overlay
ip -n r2 tunnel add name gre2 mode gre local 2.2.2.2 remote 1.1.1.1 csum
ip -n r2 link set dev gre2 up
ip -n r2 route add 192.0.2.1/32 dev gre2

# h2
ip -n h2 link set dev lo up
ip link set dev veth5 netns h2
ip -n h2 link set dev veth5 up
ip -n h2 address add 192.0.2.34/28 dev veth5
ip -n h2 route add default via 192.0.2.33

# test
dmac=$(ip -n r1 -j -p link show dev veth1 | jq -r '.[]["address"]')
ip netns exec h1 mausezahn -a own -b "$dmac" -A 192.0.2.1 -B 192.0.2.34 \
	-t udp "sp=12345,dp=54321" -p 100 -c 10 -d 1msec -q

ip -n r1 -s link show dev gre2

ip netns del h2
ip netns del r2
ip netns del r1
ip netns del h1

[2]
skb len=128 headroom=80 headlen=128 tailroom=496
mac=(80,0) net=(80,20) trans=100
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=16
dev name=gre2 feat=0x0x00000006401d5869
skb linear:   00000000: 45 00 00 80 00 00 00 00 fe 11 38 49 c0 00 02 01
skb linear:   00000010: c0 00 02 22 30 39 d4 31 00 6c 85 96 42 42 42 42
skb linear:   00000020: 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42
skb linear:   00000030: 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42
skb linear:   00000040: 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42
skb linear:   00000050: 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42
skb linear:   00000060: 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42
skb linear:   00000070: 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42 42

On Wed, Sep 1, 2021 at 11:53 AM Ido Schimmel [off-list ref] wrote:

Thanks for the quick reply, Willem.

On Wed, Sep 01, 2021 at 09:46:48AM -0400, Willem de Bruijn wrote:

quoted

Thanks for the detailed report, Ido.

This is a gre tunnel device with csum/ocsum enabled, correct?

Correct.

quoted

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 packet is received by a physical port and injected to the kernel's
Rx path by mlxsw (which does not support checksumming). The IPv4 routing
code then forwards the packet to the GRE tunnel.

I was able to reproduce the issue using veth pairs and a packet socket
[1]. Running the reproducer with the debug patch from before, I get the
following output [2].

Thanks for that device independent repro.

As expected, the following fixes it for these packets:

-       if (csum && skb_checksum_start(skb) < skb->data)
+       if (csum && skb->ip_summed == CHECKSUM_PARTIAL &&
+           skb_checksum_start(skb) < skb->data)

The question is whether we're doing the right thing when CHECKSUM_PARTIAL
is set.

Local checksum offload allows for cheap calculation of outer checksums, by
relying on the fact that the inner packet with the checksum field filled in will
sum to zero. It relies on checksum offload to compute this inner checksum,
so expects csum_start and csum_off to point after the GRE header.

If so, then the existing fix won't break correctly configured skbs as it only
drops packets for which this does not hold.