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
| ||
|
Date: Mon, 20 Nov 2017 08:47:10 -0800 From: Yonghong Song <yhs@...com> To: Arnaldo Carvalho de Melo <acme@...nel.org>, Daniel Borkmann <daniel@...earbox.net> CC: Gianluca Borello <g.borello@...il.com>, Alexei Starovoitov <ast@...nel.org>, David Miller <davem@...emloft.net>, Linux Networking Development Mailing List <netdev@...r.kernel.org> Subject: Re: len = bpf_probe_read_str(); bpf_perf_event_output(... len) == FAIL On 11/20/17 5:31 AM, Arnaldo Carvalho de Melo wrote: > Em Tue, Nov 14, 2017 at 09:25:17PM +0100, Daniel Borkmann escreveu: >> On 11/14/2017 07:15 PM, Yonghong Song wrote: >>> On 11/14/17 6:19 AM, Daniel Borkmann wrote: >>>> On 11/14/2017 02:42 PM, Arnaldo Carvalho de Melo wrote: >>>>> Em Tue, Nov 14, 2017 at 02:09:34PM +0100, Daniel Borkmann escreveu: >>>>>> On 11/14/2017 01:58 PM, Arnaldo Carvalho de Melo wrote: >>>>>>> Em Tue, Nov 14, 2017 at 01:09:39AM +0100, Daniel Borkmann escreveu: >>>>>>>> On 11/13/2017 04:08 PM, Arnaldo Carvalho de Melo wrote: >>>>>>>>> libbpf: -- BEGIN DUMP LOG --- >>>>>>>>> libbpf: >>>>>>>>> 0: (79) r3 = *(u64 *)(r1 +104) >>>>>>>>> 1: (b7) r2 = 0 >>>>>>>>> 2: (bf) r6 = r1 >>>>>>>>> 3: (bf) r1 = r10 >>>>>>>>> 4: (07) r1 += -128 >>>>>>>>> 5: (b7) r2 = 128 >>>>>>>>> 6: (85) call bpf_probe_read_str#45 >>>>>>>>> 7: (bf) r1 = r0 >>>>>>>>> 8: (07) r1 += -1 >>>>>>>>> 9: (67) r1 <<= 32 >>>>>>>>> 10: (77) r1 >>= 32 >>>>>>>>> 11: (25) if r1 > 0x7f goto pc+11 >>>>>>>> >>>>>>>> Right, so the compiler is optimizing the two tests into a single one above, >>>>>>>> which means lower bound cannot properly be derived again by the verifier due >>>>>>>> to this and thus you'll get the error. Similar issue was seen recently [1]. >>>>>>>> >>>>>>>> Does the below hack work for you? >>>>>>>> >>>>>>>> int prog([...]) >>>>>>>> { >>>>>>>> char filename[128]; >>>>>>>> int ret = bpf_probe_read_str(filename, sizeof(filename), filename_ptr); >>>>>>>> if (ret > 0) >>>>>>>> bpf_perf_event_output(ctx, &__bpf_stdout__, BPF_F_CURRENT_CPU, filename, >>>>>>>> ret & (sizeof(filename) - 1)); >>>>>>>> return 1; >>>>>>>> } >>>>>>>> >>>>>>>> r0 should keep on tracking bounds here at least: >>>>>>>> >>>>>>>> prog: >>>>>>>> 0: bf 16 00 00 00 00 00 00 r6 = r1 >>>>>>>> 1: bf a1 00 00 00 00 00 00 r1 = r10 >>>>>>>> 2: 07 01 00 00 80 ff ff ff r1 += -128 >>>>>>>> 3: b7 02 00 00 80 00 00 00 r2 = 128 >>>>>>>> 4: 85 00 00 00 2d 00 00 00 call 45 >>>>>>>> 5: 67 00 00 00 20 00 00 00 r0 <<= 32 >>>>>>>> 6: c7 00 00 00 20 00 00 00 r0 s>>= 32 >>>>>>>> 7: b7 01 00 00 01 00 00 00 r1 = 1 >>>>>>>> 8: 6d 01 0a 00 00 00 00 00 if r1 s> r0 goto 10 >>>>>>>> 9: 57 00 00 00 7f 00 00 00 r0 &= 127 >>>>>>>> 10: bf a4 00 00 00 00 00 00 r4 = r10 >>>>>>>> 11: 07 04 00 00 80 ff ff ff r4 += -128 >>>>>>>> 12: bf 61 00 00 00 00 00 00 r1 = r6 >>>>>>>> 13: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll >>>>>>>> 15: 18 03 00 00 ff ff ff ff 00 00 00 00 00 00 00 00 r3 = 4294967295ll >>>>>>>> 17: bf 05 00 00 00 00 00 00 r5 = r0 >>>>>>>> 18: 85 00 00 00 19 00 00 00 call 25 >>>>>>>> >>>>>>>> [1] https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_project_netdev_list_-3Fseries-3D13211&d=DwIDaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=Qp3xFfXEz-CT8rzYtrHeXbow2M6FlsUzwcY32i3_2Q0&s=z0d6b_hxStA845Kh7epJ-JiFwkiWqUH_z3fEadwqAQY&e= >>>>>>> >>>>>>> Not yet: >>>>>>> >>>>>>> 6: (85) call bpf_probe_read_str#45 >>>>>>> 7: (bf) r1 = r0 >>>>>>> 8: (67) r1 <<= 32 >>>>>>> 9: (77) r1 >>= 32 >>>>>>> 10: (15) if r1 == 0x0 goto pc+10 >>>>>>> R0=inv(id=0) R1=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6=ctx(id=0,off=0,imm=0) R10=fp0 >>>>>>> 11: (57) r0 &= 127 >>>>>>> 12: (bf) r4 = r10 >>>>>>> 13: (07) r4 += -128 >>>>>>> 14: (bf) r1 = r6 >>>>>>> 15: (18) r2 = 0xffff92bfc2aba840u >>>>>>> 17: (18) r3 = 0xffffffff >>>>>>> 19: (bf) r5 = r0 >>>>>>> 20: (85) call bpf_perf_event_output#25 >>>>>>> invalid stack type R4 off=-128 access_size=0 >>>>>>> >>>>>>> I'll try updating clang/llvm... >>>>>>> >>>>>>> Full details: >>>>>>> >>>>>>> [root@...et bpf]# cat open.c >>>>>>> #include "bpf.h" >>>>>>> >>>>>>> SEC("prog=do_sys_open filename") >>>>>>> int prog(void *ctx, int err, const char __user *filename_ptr) >>>>>>> { >>>>>>> char filename[128]; >>>>>>> const unsigned len = bpf_probe_read_str(filename, sizeof(filename), filename_ptr); >>>>>> >>>>>> Btw, I was using 'int' here above instead of 'unsigned' as strncpy_from_unsafe() >>>>>> could potentially return errors like -EFAULT. >>>>> >>>>> I changed to int, didn't help >>>>> >>>>>> Currently having a version compiled from the git tree: >>>>>> >>>>>> # llc --version >>>>>> LLVM (https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_&d=DwIDaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=Qp3xFfXEz-CT8rzYtrHeXbow2M6FlsUzwcY32i3_2Q0&s=BKC_Gu9s1hw0v13OCgCpfsGtAY2hE7dujFqg8LNaK2I&e=): >>>>>> LLVM version 6.0.0git-2d810c2 >>>>>> Optimized build. >>>>>> Default target: x86_64-unknown-linux-gnu >>>>>> Host CPU: skylake >>>>> >>>>> [root@...et bpf]# llc --version >>>>> LLVM (https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_&d=DwIDaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=Qp3xFfXEz-CT8rzYtrHeXbow2M6FlsUzwcY32i3_2Q0&s=BKC_Gu9s1hw0v13OCgCpfsGtAY2hE7dujFqg8LNaK2I&e=): >>>>> LLVM version 4.0.0svn >>>>> >>>>> Old stuff! ;-) Will change, but improving these messages should be on >>>>> the radar, I think :-) >>>> >>>> Yep, agree, I think we need a generic, better solution for this type of >>>> issue instead of converting individual helpers to handle 0 min bound and >>>> then only bailing out in such case; need to brainstorm a bit on that. >>>> >>>> I think for the above in your case ... >>>> >>>> [...] >>>> 6: (85) call bpf_probe_read_str#45 >>>> 7: (bf) r1 = r0 >>>> 8: (67) r1 <<= 32 >>>> 9: (77) r1 >>= 32 >>>> 10: (15) if r1 == 0x0 goto pc+10 >>>> R0=inv(id=0) R1=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6=ctx(id=0,off=0,imm=0) R10=fp0 >>>> 11: (57) r0 &= 127 >>>> [...] >>>> >>>> ... the shifts on r1 might be due to using 32 bit type, so if you find > > Where is it using a 32 bit type? > > Here is it again, now using clang 6: > > [root@...et bpf]# trace -v -e *open,open.c usleep 2 > bpf: builtin compilation failed: -95, try external compiler > Kernel build dir is set to /lib/modules/4.14.0+/build > set env: KBUILD_DIR=/lib/modules/4.14.0+/build > unset env: KBUILD_OPTS > include option is set to -nostdinc -isystem /usr/lib/gcc/x86_64-redhat-linux/7/include -I/home/acme/git/linux/arch/x86/include -I./arch/x86/include/generated -I/home/acme/git/linux/include -I./include -I/home/acme/git/linux/arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I/home/acme/git/linux/include/uapi -I./include/generated/uapi -include /home/acme/git/linux/include/linux/kconfig.h > set env: NR_CPUS=4 > set env: LINUX_VERSION_CODE=0x40e00 > set env: CLANG_EXEC=/usr/local/bin/clang > unset env: CLANG_OPTIONS > set env: KERNEL_INC_OPTIONS= -nostdinc -isystem /usr/lib/gcc/x86_64-redhat-linux/7/include -I/home/acme/git/linux/arch/x86/include -I./arch/x86/include/generated -I/home/acme/git/linux/include -I./include -I/home/acme/git/linux/arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I/home/acme/git/linux/include/uapi -I./include/generated/uapi -include /home/acme/git/linux/include/linux/kconfig.h > set env: WORKING_DIR=/lib/modules/4.14.0+/build > set env: CLANG_SOURCE=/home/acme/bpf/open.c > llvm compiling command template: $CLANG_EXEC -D__KERNEL__ -D__NR_CPUS__=$NR_CPUS -DLINUX_VERSION_CODE=$LINUX_VERSION_CODE $CLANG_OPTIONS $KERNEL_INC_OPTIONS -Wno-unused-value -Wno-pointer-sign -working-directory $WORKING_DIR -c "$CLANG_SOURCE" -target bpf -O2 -o - > libbpf: loading object 'open.c' from buffer > libbpf: section .strtab, size 103, link 0, flags 0, type=3 > libbpf: section .text, size 0, link 0, flags 6, type=1 > libbpf: section prog=do_sys_open filename, size 168, link 0, flags 6, type=1 > libbpf: found program prog=do_sys_open filename > libbpf: section .relprog=do_sys_open filename, size 16, link 8, flags 0, type=9 > libbpf: section maps, size 16, link 0, flags 3, type=1 > libbpf: section license, size 4, link 0, flags 3, type=1 > libbpf: license of open.c is GPL > libbpf: section version, size 4, link 0, flags 3, type=1 > libbpf: kernel version of open.c is 40e00 > libbpf: section .symtab, size 144, link 1, flags 0, type=2 > libbpf: maps in open.c: 1 maps in 16 bytes > libbpf: map 0 is "__bpf_stdout__" > libbpf: collecting relocating info for: 'prog=do_sys_open filename' > libbpf: relocation: insn_idx=13 > libbpf: relocation: find map 0 (__bpf_stdout__) for insn 13 > bpf: config program 'prog=do_sys_open filename' > symbol:do_sys_open file:(null) line:0 offset:0 return:0 lazy:(null) > parsing arg: filename into filename > bpf: config 'prog=do_sys_open filename' is ok > Looking at the vmlinux_path (8 entries long) > Using /lib/modules/4.14.0+/build/vmlinux for symbols > Open Debuginfo file: /lib/modules/4.14.0+/build/vmlinux > Try to find probe point from debuginfo. > Matched function: do_sys_open [2a2e5f8] > Probe point found: do_sys_open+0 > Searching 'filename' variable in context. > Converting variable filename into trace event. > filename type is (null). > Opening /sys/kernel/debug/tracing//README write=0 > Found 1 probe_trace_events. > Opening /sys/kernel/debug/tracing//kprobe_events write=1 > Writing event: p:perf_bpf_probe/prog _text+2493856 filename=%si:x64 > In map_prologue, ntevs=1 > mapping[0]=0 > libbpf: create map __bpf_stdout__: fd=3 > prologue: pass validation > prologue: fast path > libbpf: load bpf program failed: Permission denied > libbpf: -- BEGIN DUMP LOG --- > libbpf: > 0: (79) r3 = *(u64 *)(r1 +104) > 1: (b7) r2 = 0 > 2: (bf) r6 = r1 > 3: (bf) r1 = r10 > 4: (07) r1 += -128 > 5: (b7) r2 = 128 > 6: (85) call bpf_probe_read_str#45 > 7: (bf) r1 = r0 > 8: (67) r1 <<= 32 > 9: (77) r1 >>= 32 This shift is due to the return type of bpf_probe_read_str which is int. The compiler does a sign extension to get it to u64 type. > 10: (15) if r1 == 0x0 goto pc+10 Since it is u64 type, the compiler uses "==" instead of ">". > R0=inv(id=0) R1=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff)) R6=ctx(id=0,off=0,imm=0) R10=fp0 > 11: (57) r0 &= 127 > 12: (bf) r4 = r10 > 13: (07) r4 += -128 > 14: (bf) r1 = r6 > 15: (18) r2 = 0xffff9b56ca5a2a80 > 17: (18) r3 = 0xffffffff > 19: (bf) r5 = r0 r5 = r0, and r0 is still possbily 0. r0 = bpf_probe_read_str ... r0 &= 127 ... The same symptom as "int len" type. > 20: (85) call bpf_perf_event_output#25 > invalid stack type R4 off=-128 access_size=0 > > libbpf: -- END LOG -- > libbpf: Loading the 0th instance of program 'prog=do_sys_open filename' failed > libbpf: failed to load program 'prog=do_sys_open filename' > libbpf: failed to load object 'open.c' > bpf: load objects failed > event syntax error: 'open.c' > \___ Kernel verifier blocks program loading > > (add -v to see detail) > Run 'perf list' for a list of valid events > > Usage: perf trace [<options>] [<command>] > or: perf trace [<options>] -- <command> [<options>] > or: perf trace record [<options>] [<command>] > or: perf trace record [<options>] -- <command> [<options>] > > -e, --event <event> event/syscall selector. use 'perf list' to list available events > Opening /sys/kernel/debug/tracing//kprobe_events write=1 > Opening /sys/kernel/debug/tracing//uprobe_events write=1 > Parsing probe_events: p:perf_bpf_probe/prog _text+2493856 filename=%si:x64 > Group:perf_bpf_probe Event:prog probe:p > Writing event: -:perf_bpf_probe/prog > [root@...et bpf]# /usr/local/bin/clang --version > clang version 6.0.0 (https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_git_clang.git&d=DwIDAw&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=lALOhwxSDXp7fA6co6UyLBnLfqv0e2pf97GCuu5hGSw&s=FGoodXMmAdx4dqZJtvq6Vosu03to1-GFIRbagBiz0fg&e= 56cc8f8880db2ebc433eeb6b6a707c101467a186) (https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_git_llvm.git&d=DwIDAw&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=lALOhwxSDXp7fA6co6UyLBnLfqv0e2pf97GCuu5hGSw&s=_pQdnamFeu6n_um7WGrLwaCMtXqIVGLLEmYbcss_0aU&e= 3656d83960a4f3fedf6d8f19043abf52379f78c3) > Target: x86_64-unknown-linux-gnu > Thread model: posix > InstalledDir: /usr/local/bin > [root@...et bpf]# cat open.c > #include "bpf.h" > > SEC("prog=do_sys_open filename") > int prog(void *ctx, int err, const char __user *filename_ptr) > { > char filename[128]; > u64 len = bpf_probe_read_str(filename, sizeof(filename), filename_ptr); > if (len > 0) > perf_event_output(ctx, &__bpf_stdout__, BPF_F_CURRENT_CPU, filename, > len & (sizeof(filename) - 1)); > return 1; > } > [root@...et bpf]# > >>>> a way to avoid these and have the test on r0 directly, we might get there. >>>> Perhaps keep using a 64 bit type to avoid them. It would be useful to >>>> propagate the deduced bound information back to r0 when we know that >>>> neither r0 nor r1 has changed in the meantime. >>> >>> It is tricky to do in the bpf_program. Compiler tries hard to optimize :-). >>> >>> The issue is at "r0 &= 127". >>> >>> 9: (6d) if r1 s> r0 goto pc+10 >>> R0=inv(id=0,umin_value=1,umax_value=9223372036854775807,var_off=(0x0; 0x7fffffffffffffff)) R1=inv1 R6=ctx(id=0,off=0,imm=0) R10=fp0 >>> 10: R0=inv(id=0,umin_value=1,umax_value=9223372036854775807,var_off=(0x0; 0x7fffffffffffffff)) R1=inv1 R6=ctx(id=0,off=0,imm=0) R10=fp0 >>> 10: (57) r0 &= 127 >>> 11: R0=inv(id=0,umax_value=127,var_off=(0x0; 0x7f)) R1=inv1 R6=ctx(id=0,off=0,imm=0) R10=fp0 >>> >>> One possible solution for this problem is to relax the arg4 type >>> from ARG_CONST_SIZE to ARG_CONST_SIZE_OR_ZERO. >> >> Yeah, I know, that's what I mentioned earlier in this thread to resolve it, >> but do we really want to add this hack everywhere? :( Potentially any function >> having ARG_CONST_SIZE would need to handle size 0 and bail out again in their >> helper implementation and it ends up that progs start relying on this runtime >> check where we won't be able to get rid of it later on anymore. >> >>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >>> index a5580c6..a68d8bd 100644 >>> --- a/kernel/trace/bpf_trace.c >>> +++ b/kernel/trace/bpf_trace.c >>> @@ -393,6 +393,9 @@ BPF_CALL_5(bpf_perf_event_output, struct pt_regs *, regs, struct bpf_map *, map, >>> }, >>> }; >>> >>> + if (unlikely(size == 0)) >>> + return 0; >>> + >>> if (unlikely(flags & ~(BPF_F_INDEX_MASK))) >>> return -EINVAL; >>> >>> @@ -407,7 +410,7 @@ static const struct bpf_func_proto bpf_perf_event_output_proto = { >>> .arg2_type = ARG_CONST_MAP_PTR, >>> .arg3_type = ARG_ANYTHING, >>> .arg4_type = ARG_PTR_TO_MEM, >>> - .arg5_type = ARG_CONST_SIZE, >>> + .arg5_type = ARG_CONST_SIZE_OR_ZERO, >>> };
Powered by blists - more mailing lists