Message-ID: <CAMDZJNXGTGHPLw0Lkz4k5-f_j5Pi1s6A1wWuRUuOMZAnv2EnMg@mail.gmail.com>
Date:   Fri, 10 Dec 2021 12:48:22 +0800
From:   Tonghao Zhang <xiangxia.m.yue@...il.com>
To:     "zhounan (E)" <zhounan14@...wei.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "dev@...nvswitch.org" <dev@...nvswitch.org>,
        "bugs@...nvswitch.org" <bugs@...nvswitch.org>,
        "pravin.ovn@...il.com" <pravin.ovn@...il.com>,
        Lichunhe <lichunhe@...wei.com>,
        "liucheng (J)" <liucheng11@...wei.com>,
        "Hejiajun (he jiajun, SOCF&uDF )" <hejiajun@...wei.com>,
        Greg Rose <gvrose8192@...il.com>
Subject: Re: [ovs-discuss] [ovs-dev] [PATCH] datapath: fix crash when ipv6
 fragment pkt recalculate L4 checksum

On Fri, Dec 10, 2021 at 10:59 AM zhounan (E) via discuss
<ovs-discuss@...nvswitch.org> wrote:
>
> From: Zhou Nan <zhounan14@...wei.com>
>
> When we set ipv6 addr, we need to recalculate checksum of L4 header.
> In our testcase, after send ipv6 fragment package, KASAN detect "use after free" when calling function update_ipv6_checksum, and crash occurred after a while.
> If ipv6 package is fragment, and it is not first seg, we should not recalculate checksum of L4 header since this kind of package has no
> L4 header.
> To prevent crash, we set "recalc_csum" "false" when calling function "set_ipv6_addr".
> We also find that function skb_ensure_writable (make sure L4 header is writable) is helpful before calling inet_proto_csum_replace16 to recalculate checksum.
>
> Fixes: ada5efce102d6191e5c66fc385ba52a2d340ef50
>        ("datapath: Fix IPv6 later frags parsing")
>
> Signed-off-by: Zhou Nan <zhounan14@...wei.com>
> ---
>  datapath/actions.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/datapath/actions.c b/datapath/actions.c index fbf4457..52cf03e 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -456,12 +456,21 @@ static void update_ipv6_checksum(struct sk_buff *skb, u8 l4_proto,
>                                  __be32 addr[4], const __be32 new_addr[4])  {
>         int transport_len = skb->len - skb_transport_offset(skb);
> +       int err;
>
>         if (l4_proto == NEXTHDR_TCP) {
> +               err = skb_ensure_writable(skb, skb_transport_offset(skb) +
> +                               sizeof(struct tcphdr));
> +               if (unlikely(err))
> +                       return;
>                 if (likely(transport_len >= sizeof(struct tcphdr)))
>                         inet_proto_csum_replace16(&tcp_hdr(skb)->check, skb,
>                                                   addr, new_addr, true);
>         } else if (l4_proto == NEXTHDR_UDP) {
> +               err = skb_ensure_writable(skb, skb_transport_offset(skb) +
> +                               sizeof(struct udphdr));
> +               if (unlikely(err))
> +                       return;
>                 if (likely(transport_len >= sizeof(struct udphdr))) {
>                         struct udphdr *uh = udp_hdr(skb);
>
> @@ -473,6 +482,10 @@ static void update_ipv6_checksum(struct sk_buff *skb, u8 l4_proto,
>                         }
>                 }
>         } else if (l4_proto == NEXTHDR_ICMP) {
> +               err = skb_ensure_writable(skb, skb_transport_offset(skb) +
> +                               sizeof(struct icmp6hdr));
> +               if (unlikely(err))
> +                       return;
>                 if (likely(transport_len >= sizeof(struct icmp6hdr)))
>                         inet_proto_csum_replace16(&icmp6_hdr(skb)->icmp6_cksum,
>                                                   skb, addr, new_addr, true);
> @@ -589,12 +602,15 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key *flow_key,
>         if (is_ipv6_mask_nonzero(mask->ipv6_src)) {
>                 __be32 *saddr = (__be32 *)&nh->saddr;
>                 __be32 masked[4];
> +               bool recalc_csum = true;
>
>                 mask_ipv6_addr(saddr, key->ipv6_src, mask->ipv6_src, masked);
>
>                 if (unlikely(memcmp(saddr, masked, sizeof(masked)))) {
> +                       if (flow_key->ip.frag == OVS_FRAG_TYPE_LATER)
> +                               recalc_csum = false;
>                         set_ipv6_addr(skb, flow_key->ip.proto, saddr, masked,
> -                                     true);
> +                                     recalc_csum);
>                         memcpy(&flow_key->ipv6.addr.src, masked,
>                                sizeof(flow_key->ipv6.addr.src));
>                 }
> @@ -614,6 +630,8 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key *flow_key,
>                                                              NEXTHDR_ROUTING,
>                                                              NULL, &flags)
>                                                != NEXTHDR_ROUTING);
> +                       if (flow_key->ip.frag == OVS_FRAG_TYPE_LATER)
> +                               recalc_csum = false;
>
>                         set_ipv6_addr(skb, flow_key->ip.proto, daddr, masked,
>                                       recalc_csum);
> --
> 2.27.0
>
> _______________________________________________
> discuss mailing list

As Gregory said, you should rebase your patch on linux upstream. and
patch is reviewd in netdev@...r.kernel.org mail list.
OvS kernel module in upstream is:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/openvswitch

When the patch is applied in linux upstream, you can backport it.
Please see the section "Changes to Linux kernel components"
https://docs.openvswitch.org/en/latest/internals/contributing/backporting-patches/

> discuss@...nvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-discuss



-- 
Best regards, Tonghao

Powered by blists - more mailing lists