netdev - Re: [PATCH net] ip6_gre: validate csum_start only if CHECKSUM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 2 Sep 2021 13:27:33 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Ido Schimmel <idosch@...sch.org>,
        chouhan.shreyansh630@...il.com,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net] ip6_gre: validate csum_start only if CHECKSUM_PARTIAL

On Thu, Sep 2, 2021 at 12:36 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> From: Willem de Bruijn <willemb@...gle.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@...gle.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.