Re: [PATCH net] ip6_gre: validate csum_start only if CHECKSUM_PARTIAL

From: Alexander Duyck <alexander.duyck@gmail.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Netdev <netdev@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Ido Schimmel <idosch@idosch.org>,
	chouhan.shreyansh630@gmail.com,
	Willem de Bruijn <willemb@google.com>
Subject: Re: [PATCH net] ip6_gre: validate csum_start only if CHECKSUM_PARTIAL
Date: Thu, 2 Sep 2021 13:27:33 -0700	[thread overview]
Message-ID: <CAKgT0Uc43z1qZr+wsB-UaxKV-3_RMjA3aw_LeeX-HswTvxitng@mail.gmail.com> (raw)
In-Reply-To: <20210902193447.94039-1-willemdebruijn.kernel@gmail.com>

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.