| Message ID | 20220606132107.3582565-1-willemdebruijn.kernel@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8d21e9963bec1aad2280cdd034c8993033ef2948 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ip_gre: test csum_start instead of transport header | expand |
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>
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.
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>
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!
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;