| 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
| ||
|
Message-ID: <e4abe45b-2c80-9448-677c-e352f0ecb24e@fb.com>
Date: Mon, 10 Aug 2020 10:16:12 -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/10/20 6:54 AM, Jiri Olsa wrote:
> On Sun, Aug 09, 2020 at 06:21:01PM -0700, Yonghong Song wrote:
>>
>>
>> On 8/7/20 10:30 AM, Jiri Olsa wrote:
>>> hi,
>>> we have a customer facing some odd verifier fails on following
>>> sk_skb program:
>>>
>>> 0. r2 = *(u32 *)(r1 + data_end)
>>> 1. r4 = *(u32 *)(r1 + data)
>>> 2. r3 = r4
>>> 3. r3 += 42
>>> 4. r1 = 0
>>> 5. if r3 > r2 goto 8
>>> 6. r4 += 14
>>> 7. r1 = r4
>>> 8. if r3 > r2 goto 10
>>> 9. r2 = *(u8 *)(r1 + 9)
>>> 10. r0 = 0
>>> 11. exit
>>>
>>> The code checks if the skb data is big enough (5) and if it is,
>>> it prepares pointer in r1 (7), then there's again size check (8)
>>> and finally data load from r1 (9).
>>>
>>> It's and odd code, but apparently this is something that can
>>> get produced by clang.
>>
>> Could you provide a test case where clang generates the above code?
>> I would like to see whether clang can do a better job to avoid
>> such codes.
>
> I get that code genrated by using recent enough upstream clang
> on the attached source.
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.
Will try to fix this issue in llvm12 as well.
Thanks!
>
> /opt/clang/bin/clang --version
> clang version 11.0.0 (https://github.com/llvm/llvm-project.git 4cbfb98eb362b0629d5d1cd113af4427e2904763)
> Target: x86_64-unknown-linux-gnu
> Thread model: posix
> InstalledDir: /opt/clang/bin
>
> $ llvm-objdump -d verifier-cond-repro.o
>
> verifier-cond-repro.o: file format ELF64-BPF
>
> Disassembly of section .text:
>
> 0000000000000000 my_prog:
> 0: 61 12 50 00 00 00 00 00 r2 = *(u32 *)(r1 + 80)
> 1: 61 14 4c 00 00 00 00 00 r4 = *(u32 *)(r1 + 76)
> 2: bf 43 00 00 00 00 00 00 r3 = r4
> 3: 07 03 00 00 2a 00 00 00 r3 += 42
> 4: b7 01 00 00 00 00 00 00 r1 = 0
> 5: 2d 23 02 00 00 00 00 00 if r3 > r2 goto +2 <LBB0_2>
> 6: 07 04 00 00 0e 00 00 00 r4 += 14
> 7: bf 41 00 00 00 00 00 00 r1 = r4
>
> 0000000000000040 LBB0_2:
> 8: 2d 23 05 00 00 00 00 00 if r3 > r2 goto +5 <LBB0_5>
> 9: 71 12 09 00 00 00 00 00 r2 = *(u8 *)(r1 + 9)
> 10: 56 02 03 00 11 00 00 00 if w2 != 17 goto +3 <LBB0_5>
> 11: b4 00 00 00 d2 04 00 00 w0 = 1234
> 12: 69 11 16 00 00 00 00 00 r1 = *(u16 *)(r1 + 22)
> 13: 16 01 01 00 d2 04 00 00 if w1 == 1234 goto +1 <LBB0_6>
>
> 0000000000000070 LBB0_5:
> 14: b4 00 00 00 ff ff ff ff w0 = -1
>
> 0000000000000078 LBB0_6:
> 15: 95 00 00 00 00 00 00 00 exit
>
>
> thanks,
> jirka
>
>
> ---
> // Copyright (c) 2019 Tigera, Inc. All rights reserved.
> //
> // Licensed under the Apache License, Version 2.0 (the "License");
> // you may not use this file except in compliance with the License.
> // You may obtain a copy of the License at
> //
> // https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=bsFf5gOsaXXCIBYvnl6AK78aRCaFS1zhqvVCYbn4JBA&s=5MVLZXPlXMo2B2K5wDP5P3Lmn4-TQTKHvQfvZupEFvs&e=
> //
> // Unless required by applicable law or agreed to in writing, software
> // distributed under the License is distributed on an "AS IS" BASIS,
> // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> // See the License for the specific language governing permissions and
> // limitations under the License.
>
> #include <stddef.h>
> #include <string.h>
> #include <linux/bpf.h>
> #include <linux/if_ether.h>
> #include <linux/if_packet.h>
> #include <linux/ip.h>
> #include <linux/ipv6.h>
> #include <linux/in.h>
> #include <linux/udp.h>
> #include <linux/tcp.h>
> #include <linux/pkt_cls.h>
> #include <sys/socket.h>
> #include <bpf/bpf_helpers.h>
> #include <bpf/bpf_endian.h>
>
> #include <stddef.h>
>
> #define INLINE inline __attribute__((always_inline))
>
> #define skb_shorter(skb, len) ((void *)(long)(skb)->data + (len) > (void *)(long)skb->data_end)
>
> #define ETH_IPV4_UDP_SIZE (14+20+8)
>
> 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;
> }
>
> int my_prog(struct __sk_buff *skb)
> {
> struct iphdr *ip = NULL;
> struct udphdr *udp;
> __u8 proto = 0;
>
> if (!(ip = get_iphdr(skb)))
> goto out;
>
> proto = ip->protocol;
>
> if (proto != IPPROTO_UDP)
> goto out;
>
> udp = (void*)(ip + 1);
>
> if (udp->dest != 1234)
> goto out;
>
> if (!udp)
> goto out;
>
> return udp->dest;
>
> out:
> return -1;
> }
>
Powered by blists - more mailing lists