Re: len = bpf_probe_read_str(); bpf_perf_event_output(... len) == FAIL

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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] http://patchwork.ozlabs.org/project/netdev/list/?series=13211
> > 
> > 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 = 0xffff92bfc2aba840
> > 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@jouet 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 (http://llvm.org/):
>   LLVM version 6.0.0git-2d810c2
>   Optimized build.
>   Default target: x86_64-unknown-linux-gnu
>   Host CPU: skylake

[root@jouet bpf]# llc --version
LLVM (http://llvm.org/):
  LLVM version 4.0.0svn

Old stuff! ;-) Will change, but improving these messages should be on
the radar, I think :-)

- Arnaldo
 
>   Registered Targets:
>     bpf    - BPF (host endian)
>     bpfeb  - BPF (big endian)
>     bpfel  - BPF (little endian)
>     x86    - 32-bit X86: Pentium-Pro and above
>     x86-64 - 64-bit X86: EM64T and AMD64
> 
> > 	if (len > 0)
> >        		perf_event_output(ctx, &__bpf_stdout__, BPF_F_CURRENT_CPU, filename,
> > 				  len & (sizeof(filename) - 1));
> > 	return 1;
> > }