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

Message-ID: <f03e2ce3-8cf8-0590-1777-f9e8171cd3fa@fb.com>
Date:   Tue, 11 Aug 2020 09:08:13 -0700
From:   Yonghong Song <yhs@...com>
To:     Jiri Olsa <jolsa@...hat.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 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".

> 
> is the change below what you mean? it produces the same code for me:
> 
> 	diff --git a/tools/testing/selftests/bpf/progs/verifier-cond-repro.c b/tools/testing/selftests/bpf/progs/verifier-cond-repro.c
> 	index 2f11027d7e67..9c401bd00ab7 100644
> 	--- a/tools/testing/selftests/bpf/progs/verifier-cond-repro.c
> 	+++ b/tools/testing/selftests/bpf/progs/verifier-cond-repro.c
> 	@@ -41,12 +41,10 @@ static INLINE struct iphdr *get_iphdr (struct __sk_buff *skb)
> 		struct ethhdr *eth;
> 	
> 		if (skb_shorter(skb, ETH_IPV4_UDP_SIZE))
> 	-		goto out;
> 	+		return NULL;
> 	
> 		eth = (void *)(long)skb->data;
> 		ip = (void *)(eth + 1);
> 	-
> 	-out:
> 		return ip;
> 	 }
> 	
> 
> I also tried this one:
> 
> 	diff --git a/tools/testing/selftests/bpf/progs/verifier-cond-repro.c b/tools/testing/selftests/bpf/progs/verifier-cond-repro.c
> 	index 2f11027d7e67..00ff06fe6fdd 100644
> 	--- a/tools/testing/selftests/bpf/progs/verifier-cond-repro.c
> 	+++ b/tools/testing/selftests/bpf/progs/verifier-cond-repro.c
> 	@@ -57,7 +57,7 @@ int my_prog(struct __sk_buff *skb)
> 		__u8 proto = 0;
> 	
> 		if (!(ip = get_iphdr(skb)))
> 	-               goto out;
> 	+               return -1;
> 	
> 		proto = ip->protocol;
> 
> it did just slight change in generated code - added 'w0 = -1'
> before the second condition

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?

> 
> thanks a lot!
> jirka
>