netdev - Re: [RFC] bpf: verifier check for dead branch

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] [day] [month] [year] [list]

Date:   Wed, 12 Aug 2020 09:48:26 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Yonghong Song <yhs@...com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, Andrii Nakryiko <andriin@...com>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>
Subject: Re: [RFC] bpf: verifier check for dead branch

On Tue, Aug 11, 2020 at 09:08:13AM -0700, Yonghong Song wrote:
> 
> 
> On 8/11/20 12:14 AM, Jiri Olsa wrote:
> > On Mon, Aug 10, 2020 at 10:16:12AM -0700, Yonghong Song wrote:
> > 
> > SNIP
> > 
> > > 
> > > Thanks for the test case. I can reproduce the issue. The following
> > > is why this happens in llvm.
> > > the pseudo IR code looks like
> > >     data = skb->data
> > >     data_end = skb->data_end
> > >     comp = data + 42 > data_end
> > >     ip = select "comp" nullptr "data + some offset"
> > >           <=== select return one of nullptr or "data + some offset" based on
> > > "comp"
> > >     if comp   // original skb_shorter condition
> > >        ....
> > >     ...
> > >        = ip
> > > 
> > > In llvm, bpf backend "select" actually inlined "comp" to generate proper
> > > control flow. Therefore, comp is computed twice like below
> > >     data = skb->data
> > >     data_end = skb->data_end
> > >     if (data + 42 > data_end) {
> > >        ip = nullptr; goto block1;
> > >     } else {
> > >        ip = data + some_offset;
> > >        goto block2;
> > >     }
> > >     ...
> > >     if (data + 42 > data_end) // original skb_shorter condition
> > > 
> > > The issue can be workarounded the source. Just check data + 42 > data_end
> > > and if failure return. Then you will be able to assign
> > > a value to "ip" conditionally.
> 
> sorry for typo. The above should be "conditionally" -> "unconditionally".

aaah, ok ;-)

> 
> The following is what I mean:
> 
> diff --git a/t.c b/t.c
> index c6baf28..7bf90dc 100644
> --- a/t.c
> +++ b/t.c
> @@ -37,17 +37,10 @@
> 
>  static INLINE struct iphdr *get_iphdr (struct __sk_buff *skb)
>  {
> -       struct iphdr *ip = NULL;
>         struct ethhdr *eth;
> 
> -       if (skb_shorter(skb, ETH_IPV4_UDP_SIZE))
> -               goto out;
> -
>         eth = (void *)(long)skb->data;
> -       ip = (void *)(eth + 1);
> -
> -out:
> -       return ip;
> +       return (void *)(eth + 1);
>  }
> 
>  int my_prog(struct __sk_buff *skb)
> @@ -56,9 +49,10 @@ int my_prog(struct __sk_buff *skb)
>         struct udphdr *udp;
>         __u8 proto = 0;
> 
> -       if (!(ip = get_iphdr(skb)))
> +       if (skb_shorter(skb, ETH_IPV4_UDP_SIZE))
>                 goto out;
> 
> +       ip = get_iphdr(skb);
>         proto = ip->protocol;
> 
>         if (proto != IPPROTO_UDP)
> 
> > 
> > > 
> > > Will try to fix this issue in llvm12 as well.
> > > Thanks!
> > 
> > great, could you please CC me on the changes?
> 
> This will be a llvm change. Do you have llvm phabricator login name
> https://reviews.llvm.org/
> so I can add you as a subscriber?

Jiri (Olsa)
olsajiri@...il.com

thank,
jirka