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: <20200811071438.GC699846@krava>
Date:   Tue, 11 Aug 2020 09:14:38 +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 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.

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

> 
> Will try to fix this issue in llvm12 as well.
> Thanks!

great, could you please CC me on the changes?

thanks a lot!
jirka