[PATCH net] ip_gre: test csum_start instead of transport header

[Willem de Bruijn]

From: Willem de Bruijn <wil...@google.com>

GRE with TUNNEL_CSUM will apply local checksum offload on
CHECKSUM_PARTIAL packets.

ipgre_xmit must validate csum_start after an optional skb_pull,
else lco_csum may trigger an overflow. The original check was

if (csum && skb_checksum_start(skb) < skb->data)
return -EINVAL;

This had false positives when skb_checksum_start is undefined:
when ip_summed is not CHECKSUM_PARTIAL. A discussed refinement
was straightforward

if (csum && skb->ip_summed == CHECKSUM_PARTIAL &&
skb_checksum_start(skb) < skb->data)
return -EINVAL;

But was eventually revised more thoroughly:
- restrict the check to the only branch where needed, in an
uncommon GRE path that uses header_ops and calls skb_pull.
- test skb_transport_header, which is set along with csum_start
in skb_partial_csum_set in the normal header_ops datapath.

Turns out skbs can arrive in this branch without the transport
header set, e.g., through BPF redirection.

Revise the check back to check csum_start directly, and only if
CHECKSUM_PARTIAL. Do leave the check in the updated location.
Check field regardless of whether TUNNEL_CSUM is configured.

Link: https://lore.kernel.org/netdev/YS+h%2FtqCJJiQei+W@shredder/
Link: https://lore.kernel.org/all/20210902193447.94039-2...@gmail.com/T/#u
Fixes: 8a0ed250f911 ("ip_gre: validate csum_start only on pull")
Reported-by: syzbot <syzk...@googlegroups.com>
Signed-off-by: Willem de Bruijn <wil...@google.com>
---
net/ipv4/ip_gre.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 7e474a85deaf..3b9cd487075a 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -629,21 +629,20 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
}

if (dev->header_ops) {
- const int pull_len = tunnel->hlen + sizeof(struct iphdr);
-
if (skb_cow_head(skb, 0))
goto free_skb;

tnl_params = (const struct iphdr *)skb->data;

- if (pull_len > skb_transport_offset(skb))
- goto free_skb;
-
/* Pull skb since ip_tunnel_xmit() needs skb->data pointing
* to gre header.
*/
- skb_pull(skb, pull_len);
+ skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
skb_reset_mac_header(skb);
+
+ if (skb->ip_summed == CHECKSUM_PARTIAL &&
+ skb_checksum_start(skb) < skb->data)
+ goto free_skb;
} else {
if (skb_cow_head(skb, dev->needed_headroom))
goto free_skb;
--
2.36.1.255.ge46751e96f-goog

[Eric Dumazet]

Reviewed-by: Eric Dumazet <edum...@google.com>

[Jakub Kicinski]

Missing a CC for Alex, adding now.

[Alexander H Duyck]

The one thing I might change would be the ordering of your two tests at
the end. It is more likely that skb_checksum_start will be less than
skb->data in most cases as skb->csum_start is initialized to 0 at
allocation. So cases where it is greater than skb->data should be rare
whereas the CHECKSUM_PARTIAL check will come up as true probably more
often then not as it isn't uncommon to see checksum or TSO offloaded
frames.

Anyway functionality-wise this looks good to me and what I suggested is
more of an optimization anyway.

Reviewed-by: Alexander Duyck <alexand...@fb.com>

[patchwork-b...@kernel.org]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <ku...@kernel.org>:

On Mon, 6 Jun 2022 09:21:07 -0400 you wrote:
> From: Willem de Bruijn <wil...@google.com>
>
> GRE with TUNNEL_CSUM will apply local checksum offload on
> CHECKSUM_PARTIAL packets.
>
> ipgre_xmit must validate csum_start after an optional skb_pull,
> else lco_csum may trigger an overflow. The original check was
>

> [...]

Here is the summary with links:
- [net] ip_gre: test csum_start instead of transport header
https://git.kernel.org/netdev/net/c/8d21e9963bec

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

[Greg Kroah-Hartman]

From: Willem de Bruijn <wil...@google.com>

[ Upstream commit 8d21e9963bec1aad2280cdd034c8993033ef2948 ]

GRE with TUNNEL_CSUM will apply local checksum offload on
CHECKSUM_PARTIAL packets.

ipgre_xmit must validate csum_start after an optional skb_pull,
else lco_csum may trigger an overflow. The original check was

if (csum && skb_checksum_start(skb) < skb->data)
return -EINVAL;

This had false positives when skb_checksum_start is undefined:
when ip_summed is not CHECKSUM_PARTIAL. A discussed refinement
was straightforward

if (csum && skb->ip_summed == CHECKSUM_PARTIAL &&
skb_checksum_start(skb) < skb->data)
return -EINVAL;

But was eventually revised more thoroughly:
- restrict the check to the only branch where needed, in an
uncommon GRE path that uses header_ops and calls skb_pull.
- test skb_transport_header, which is set along with csum_start
in skb_partial_csum_set in the normal header_ops datapath.

Turns out skbs can arrive in this branch without the transport
header set, e.g., through BPF redirection.

Revise the check back to check csum_start directly, and only if
CHECKSUM_PARTIAL. Do leave the check in the updated location.
Check field regardless of whether TUNNEL_CSUM is configured.

Link: https://lore.kernel.org/netdev/YS+h%2FtqCJJiQei+W@shredder/
Link: https://lore.kernel.org/all/20210902193447.94039-2...@gmail.com/T/#u
Fixes: 8a0ed250f911 ("ip_gre: validate csum_start only on pull")
Reported-by: syzbot <syzk...@googlegroups.com>
Signed-off-by: Willem de Bruijn <wil...@google.com>

Reviewed-by: Eric Dumazet <edum...@google.com>
Reviewed-by: Alexander Duyck <alexand...@fb.com>
Link: https://lore.kernel.org/r/20220606132107.3582565-...@gmail.com
Signed-off-by: Jakub Kicinski <ku...@kernel.org>
Signed-off-by: Sasha Levin <sas...@kernel.org>

---
net/ipv4/ip_gre.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c

index 5b38d03f6d79..614410a6db44 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -602,21 +602,20 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,

}

if (dev->header_ops) {
- const int pull_len = tunnel->hlen + sizeof(struct iphdr);
-
if (skb_cow_head(skb, 0))
goto free_skb;

tnl_params = (const struct iphdr *)skb->data;

- if (pull_len > skb_transport_offset(skb))
- goto free_skb;
-
/* Pull skb since ip_tunnel_xmit() needs skb->data pointing
* to gre header.
*/
- skb_pull(skb, pull_len);
+ skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
skb_reset_mac_header(skb);
+
+ if (skb->ip_summed == CHECKSUM_PARTIAL &&
+ skb_checksum_start(skb) < skb->data)
+ goto free_skb;
} else {
if (skb_cow_head(skb, dev->needed_headroom))
goto free_skb;

--
2.35.1

[Greg Kroah-Hartman]

index 41d0f9bb5191..cf60d0e07965 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -678,21 +678,20 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,

[Greg Kroah-Hartman]

index 2a80038575d2..a7e32be8714f 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -624,21 +624,20 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,

[Greg Kroah-Hartman]

index 276a3b7b0e9c..f23528c77539 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -629,21 +629,20 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,

[Greg Kroah-Hartman]

index aacee9dd771b..bc8dfdf1c48a 100644

[Greg Kroah-Hartman]

index 8cf86e42c1d1..65b6d4c1698e 100644