All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ip_gre/ip6_gre: add check for invalid csum_start
@ 2021-08-19 14:34 Shreyansh Chouhan
  2021-08-19 16:56 ` Willem de Bruijn
  0 siblings, 1 reply; 14+ messages in thread
From: Shreyansh Chouhan @ 2021-08-19 14:34 UTC (permalink / raw)
  To: davem, yoshfuji, dsahern, kuba, willemdebruijn.kernel
  Cc: Shreyansh Chouhan, netdev, linux-kernel, syzbot+ff8e1b9f2f36481e2efc

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 <chouhan.shreyansh630@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] ip_gre/ip6_gre: add check for invalid csum_start
  2021-08-19 14:34 [PATCH] ip_gre/ip6_gre: add check for invalid csum_start Shreyansh Chouhan
@ 2021-08-19 16:56 ` Willem de Bruijn
  2021-08-19 17:04   ` Jakub Kicinski
  0 siblings, 1 reply; 14+ messages in thread
From: Willem de Bruijn @ 2021-08-19 16:56 UTC (permalink / raw)
  To: Shreyansh Chouhan
  Cc: davem, yoshfuji, dsahern, kuba, willemdebruijn.kernel, netdev,
	linux-kernel, syzbot+ff8e1b9f2f36481e2efc

On Thu, Aug 19, 2021 at 10:35 AM Shreyansh Chouhan
<chouhan.shreyansh630@gmail.com> 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 <chouhan.shreyansh630@gmail.com>

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]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] ip_gre/ip6_gre: add check for invalid csum_start
  2021-08-19 16:56 ` Willem de Bruijn
@ 2021-08-19 17:04   ` Jakub Kicinski
  2021-08-21  7:14     ` [PATCH 1/2 net] ip_gre: add validation for csum_start Shreyansh Chouhan
                       ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Jakub Kicinski @ 2021-08-19 17:04 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Shreyansh Chouhan, davem, yoshfuji, dsahern, netdev,
	linux-kernel, syzbot+ff8e1b9f2f36481e2efc

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2 net] ip_gre: add validation for csum_start
  2021-08-19 17:04   ` Jakub Kicinski
@ 2021-08-21  7:14     ` Shreyansh Chouhan
  2021-08-21 13:41       ` Willem de Bruijn
  2021-08-22 20:30       ` patchwork-bot+netdevbpf
  2021-08-21  7:14     ` [PATCH 2/2 net] ip6_gre: " Shreyansh Chouhan
  2021-08-21  7:18     ` [PATCH] ip_gre/ip6_gre: add check for invalid csum_start Shreyansh Chouhan
  2 siblings, 2 replies; 14+ messages in thread
From: Shreyansh Chouhan @ 2021-08-21  7:14 UTC (permalink / raw)
  To: davem, yoshfuji, dsahern, kuba, pshelar, willemdebruijn.kernel
  Cc: Shreyansh Chouhan, netdev, linux-kernel, syzbot+ff8e1b9f2f36481e2efc

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 <chouhan.shreyansh630@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2 net] ip6_gre: add validation for csum_start
  2021-08-19 17:04   ` Jakub Kicinski
  2021-08-21  7:14     ` [PATCH 1/2 net] ip_gre: add validation for csum_start Shreyansh Chouhan
@ 2021-08-21  7:14     ` Shreyansh Chouhan
  2021-08-21 13:42       ` Willem de Bruijn
  2021-08-21  7:18     ` [PATCH] ip_gre/ip6_gre: add check for invalid csum_start Shreyansh Chouhan
  2 siblings, 1 reply; 14+ messages in thread
From: Shreyansh Chouhan @ 2021-08-21  7:14 UTC (permalink / raw)
  To: davem, yoshfuji, dsahern, kuba, pshelar, willemdebruijn.kernel
  Cc: Shreyansh Chouhan, netdev, linux-kernel, syzbot+ff8e1b9f2f36481e2efc

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 <chouhan.shreyansh630@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] ip_gre/ip6_gre: add check for invalid csum_start
  2021-08-19 17:04   ` Jakub Kicinski
  2021-08-21  7:14     ` [PATCH 1/2 net] ip_gre: add validation for csum_start Shreyansh Chouhan
  2021-08-21  7:14     ` [PATCH 2/2 net] ip6_gre: " Shreyansh Chouhan
@ 2021-08-21  7:18     ` Shreyansh Chouhan
  2021-08-21 13:44       ` Willem de Bruijn
  2 siblings, 1 reply; 14+ messages in thread
From: Shreyansh Chouhan @ 2021-08-21  7:18 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Willem de Bruijn, davem, yoshfuji, dsahern, netdev, linux-kernel,
	syzbot+ff8e1b9f2f36481e2efc

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2 net] ip_gre: add validation for csum_start
  2021-08-21  7:14     ` [PATCH 1/2 net] ip_gre: add validation for csum_start Shreyansh Chouhan
@ 2021-08-21 13:41       ` Willem de Bruijn
  2021-09-01 11:53         ` Ido Schimmel
  2021-08-22 20:30       ` patchwork-bot+netdevbpf
  1 sibling, 1 reply; 14+ messages in thread
From: Willem de Bruijn @ 2021-08-21 13:41 UTC (permalink / raw)
  To: Shreyansh Chouhan
  Cc: davem, yoshfuji, dsahern, kuba, pshelar, willemdebruijn.kernel,
	netdev, linux-kernel, syzbot+ff8e1b9f2f36481e2efc

On Sat, Aug 21, 2021 at 3:14 AM Shreyansh Chouhan
<chouhan.shreyansh630@gmail.com> 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 <chouhan.shreyansh630@gmail.com>

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2 net] ip6_gre: add validation for csum_start
  2021-08-21  7:14     ` [PATCH 2/2 net] ip6_gre: " Shreyansh Chouhan
@ 2021-08-21 13:42       ` Willem de Bruijn
  0 siblings, 0 replies; 14+ messages in thread
From: Willem de Bruijn @ 2021-08-21 13:42 UTC (permalink / raw)
  To: Shreyansh Chouhan
  Cc: davem, yoshfuji, dsahern, kuba, pshelar, willemdebruijn.kernel,
	netdev, linux-kernel, syzbot+ff8e1b9f2f36481e2efc

On Sat, Aug 21, 2021 at 3:14 AM Shreyansh Chouhan
<chouhan.shreyansh630@gmail.com> 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 <chouhan.shreyansh630@gmail.com>

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] ip_gre/ip6_gre: add check for invalid csum_start
  2021-08-21  7:18     ` [PATCH] ip_gre/ip6_gre: add check for invalid csum_start Shreyansh Chouhan
@ 2021-08-21 13:44       ` Willem de Bruijn
  0 siblings, 0 replies; 14+ messages in thread
From: Willem de Bruijn @ 2021-08-21 13:44 UTC (permalink / raw)
  To: Shreyansh Chouhan
  Cc: Jakub Kicinski, Willem de Bruijn, davem, yoshfuji, dsahern,
	netdev, linux-kernel, syzbot+ff8e1b9f2f36481e2efc

On Sat, Aug 21, 2021 at 3:18 AM Shreyansh Chouhan
<chouhan.shreyansh630@gmail.com> 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2 net] ip_gre: add validation for csum_start
  2021-08-21  7:14     ` [PATCH 1/2 net] ip_gre: add validation for csum_start Shreyansh Chouhan
  2021-08-21 13:41       ` Willem de Bruijn
@ 2021-08-22 20:30       ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 14+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-08-22 20:30 UTC (permalink / raw)
  To: Shreyansh Chouhan
  Cc: davem, yoshfuji, dsahern, kuba, pshelar, willemdebruijn.kernel,
	netdev, linux-kernel, syzbot+ff8e1b9f2f36481e2efc

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 <chouhan.shreyansh630@gmail.com>
> 
> [...]

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



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2 net] ip_gre: add validation for csum_start
  2021-08-21 13:41       ` Willem de Bruijn
@ 2021-09-01 11:53         ` Ido Schimmel
  2021-09-01 13:46           ` Willem de Bruijn
  0 siblings, 1 reply; 14+ messages in thread
From: Ido Schimmel @ 2021-09-01 11:53 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Shreyansh Chouhan, davem, yoshfuji, dsahern, kuba, pshelar,
	netdev, linux-kernel, syzbot+ff8e1b9f2f36481e2efc

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
> <chouhan.shreyansh630@gmail.com> 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 <chouhan.shreyansh630@gmail.com>
> 
> 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2 net] ip_gre: add validation for csum_start
  2021-09-01 11:53         ` Ido Schimmel
@ 2021-09-01 13:46           ` Willem de Bruijn
  2021-09-01 15:53             ` Ido Schimmel
  0 siblings, 1 reply; 14+ messages in thread
From: Willem de Bruijn @ 2021-09-01 13:46 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: Willem de Bruijn, Shreyansh Chouhan, davem, yoshfuji, dsahern,
	kuba, pshelar, netdev, linux-kernel, syzbot+ff8e1b9f2f36481e2efc

On Wed, Sep 1, 2021 at 7:53 AM Ido Schimmel <idosch@idosch.org> wrote:
>
> 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
> > <chouhan.shreyansh630@gmail.com> 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 <chouhan.shreyansh630@gmail.com>
> >
> > 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2 net] ip_gre: add validation for csum_start
  2021-09-01 13:46           ` Willem de Bruijn
@ 2021-09-01 15:53             ` Ido Schimmel
  2021-09-01 21:39               ` Willem de Bruijn
  0 siblings, 1 reply; 14+ messages in thread
From: Ido Schimmel @ 2021-09-01 15:53 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: Shreyansh Chouhan, davem, yoshfuji, dsahern, kuba, pshelar,
	netdev, linux-kernel, syzbot+ff8e1b9f2f36481e2efc

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2 net] ip_gre: add validation for csum_start
  2021-09-01 15:53             ` Ido Schimmel
@ 2021-09-01 21:39               ` Willem de Bruijn
  0 siblings, 0 replies; 14+ messages in thread
From: Willem de Bruijn @ 2021-09-01 21:39 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: Willem de Bruijn, Shreyansh Chouhan, davem, yoshfuji, dsahern,
	kuba, pshelar, netdev, linux-kernel, syzbot+ff8e1b9f2f36481e2efc,
	Edward Cree

On Wed, Sep 1, 2021 at 11:53 AM Ido Schimmel <idosch@idosch.org> wrote:
>
> 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].

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-09-01 21:40 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-19 14:34 [PATCH] ip_gre/ip6_gre: add check for invalid csum_start Shreyansh Chouhan
2021-08-19 16:56 ` Willem de Bruijn
2021-08-19 17:04   ` Jakub Kicinski
2021-08-21  7:14     ` [PATCH 1/2 net] ip_gre: add validation for csum_start Shreyansh Chouhan
2021-08-21 13:41       ` Willem de Bruijn
2021-09-01 11:53         ` Ido Schimmel
2021-09-01 13:46           ` Willem de Bruijn
2021-09-01 15:53             ` Ido Schimmel
2021-09-01 21:39               ` Willem de Bruijn
2021-08-22 20:30       ` patchwork-bot+netdevbpf
2021-08-21  7:14     ` [PATCH 2/2 net] ip6_gre: " Shreyansh Chouhan
2021-08-21 13:42       ` Willem de Bruijn
2021-08-21  7:18     ` [PATCH] ip_gre/ip6_gre: add check for invalid csum_start Shreyansh Chouhan
2021-08-21 13:44       ` Willem de Bruijn

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.