| Message ID | 20210902193447.94039-1-willemdebruijn.kernel@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ip6_gre: validate csum_start only if CHECKSUM_PARTIAL | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: yoshfuji@linux-ipv6.org dsahern@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 12 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Thu, Sep 2, 2021 at 12:36 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > From: Willem de Bruijn <willemb@google.com> > > Only test integrity of csum_start if field is defined. > > With checksum offload and GRE tunnel checksum, gre_build_header will > cheaply build the GRE checksum using local checksum offload. This > depends on inner packet csum offload, and thus that csum_start points > behind GRE. But validate this condition only with checksum offload. > > Link: https://lore.kernel.org/netdev/YS+h%2FtqCJJiQei+W@shredder/ > Fixes: 9cf448c200ba ("ip6_gre: add validation for csum_start") > Signed-off-by: Willem de Bruijn <willemb@google.com> > --- > net/ipv6/ip6_gre.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c > index 7baf41d160f5..c456bc7f7cdc 100644 > --- a/net/ipv6/ip6_gre.c > +++ b/net/ipv6/ip6_gre.c > @@ -629,8 +629,11 @@ 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) > + /* Local checksum offload requires csum offload of the inner packet */ > + if (csum && skb->ip_summed == CHECKSUM_PARTIAL && > + skb_checksum_start(skb) < skb->data) > return -EINVAL; > + > return iptunnel_handle_offloads(skb, > csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE); > } Didn't see this come through until I had replied to the other patch. Same comments apply here. The "csum" value probably doesn't need to be checked when checking skb_checksum_start, and maybe this should trigger a WARN_ON_ONCE since it should be rare and we should be fixing any path that is requesting CHECKSUM_PARTIAL without setting skb_checksum_start.
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c index 7baf41d160f5..c456bc7f7cdc 100644 --- a/net/ipv6/ip6_gre.c +++ b/net/ipv6/ip6_gre.c @@ -629,8 +629,11 @@ 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) + /* Local checksum offload requires csum offload of the inner packet */ + if (csum && skb->ip_summed == CHECKSUM_PARTIAL && + skb_checksum_start(skb) < skb->data) return -EINVAL; + return iptunnel_handle_offloads(skb, csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE); }