* [PATCH net] ip_gre: test csum_start instead of transport header
@ 2022-06-06 13:21 Willem de Bruijn
2022-06-07 17:33 ` Eric Dumazet
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Willem de Bruijn @ 2022-06-06 13:21 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, edumazet, pabeni, Willem de Bruijn, syzbot
From: Willem de Bruijn <willemb@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-willemdebruijn.kernel@gmail.com/T/#u
Fixes: 8a0ed250f911 ("ip_gre: validate csum_start only on pull")
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Willem de Bruijn <willemb@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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] ip_gre: test csum_start instead of transport header
2022-06-06 13:21 [PATCH net] ip_gre: test csum_start instead of transport header Willem de Bruijn
@ 2022-06-07 17:33 ` Eric Dumazet
2022-06-08 0:18 ` Jakub Kicinski
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2022-06-07 17:33 UTC (permalink / raw)
To: Willem de Bruijn
Cc: netdev, David Miller, Jakub Kicinski, Paolo Abeni,
Willem de Bruijn, syzbot
On Mon, Jun 6, 2022 at 6:21 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@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-willemdebruijn.kernel@gmail.com/T/#u
> Fixes: 8a0ed250f911 ("ip_gre: validate csum_start only on pull")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] ip_gre: test csum_start instead of transport header
2022-06-06 13:21 [PATCH net] ip_gre: test csum_start instead of transport header Willem de Bruijn
2022-06-07 17:33 ` Eric Dumazet
@ 2022-06-08 0:18 ` Jakub Kicinski
2022-06-08 14:54 ` Alexander H Duyck
2022-06-09 3:40 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2022-06-08 0:18 UTC (permalink / raw)
To: Willem de Bruijn
Cc: netdev, davem, edumazet, pabeni, Willem de Bruijn, syzbot,
Alexander Duyck
On Mon, 6 Jun 2022 09:21:07 -0400 Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@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-willemdebruijn.kernel@gmail.com/T/#u
> Fixes: 8a0ed250f911 ("ip_gre: validate csum_start only on pull")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
Missing a CC for Alex, adding now.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] ip_gre: test csum_start instead of transport header
2022-06-06 13:21 [PATCH net] ip_gre: test csum_start instead of transport header Willem de Bruijn
2022-06-07 17:33 ` Eric Dumazet
2022-06-08 0:18 ` Jakub Kicinski
@ 2022-06-08 14:54 ` Alexander H Duyck
2022-06-09 3:40 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Alexander H Duyck @ 2022-06-08 14:54 UTC (permalink / raw)
To: Willem de Bruijn, netdev
Cc: davem, kuba, edumazet, pabeni, Willem de Bruijn, syzbot
On Mon, 2022-06-06 at 09:21 -0400, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@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-willemdebruijn.kernel@gmail.com/T/#u
> Fixes: 8a0ed250f911 ("ip_gre: validate csum_start only on pull")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Willem de Bruijn <willemb@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;
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 <alexanderduyck@fb.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] ip_gre: test csum_start instead of transport header
2022-06-06 13:21 [PATCH net] ip_gre: test csum_start instead of transport header Willem de Bruijn
` (2 preceding siblings ...)
2022-06-08 14:54 ` Alexander H Duyck
@ 2022-06-09 3:40 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-06-09 3:40 UTC (permalink / raw)
To: Willem de Bruijn
Cc: netdev, davem, kuba, edumazet, pabeni, willemb, syzkaller
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Mon, 6 Jun 2022 09:21:07 -0400 you wrote:
> From: Willem de Bruijn <willemb@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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-09 3:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-06 13:21 [PATCH net] ip_gre: test csum_start instead of transport header Willem de Bruijn
2022-06-07 17:33 ` Eric Dumazet
2022-06-08 0:18 ` Jakub Kicinski
2022-06-08 14:54 ` Alexander H Duyck
2022-06-09 3:40 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).