netdev - Re: len = bpf_probe_read_str(); bpf_perf_event

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: <20171121142905.GJ7918@kernel.org>
Date:   Tue, 21 Nov 2017 11:29:05 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Yonghong Song <yhs@...com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        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

Em Tue, Nov 14, 2017 at 02:58:24PM -0800, Yonghong Song escreveu:
> On 11/14/17 12:25 PM, Daniel Borkmann wrote:
> > 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.
 
> The compiler actually does the right thing for the below code:
>          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));
 
> Just from the above code without consulting bpf_probe_read_str internals, it
> is totally possible that ret = 128, then
> ret & (sizeof(filename) - 1) = 0.

> The issue is that the verifier did not set the "ret" initial range as (-inf,
> sizeof(filename) - 1). We could have this information associated with helper
> and feed back to verifier.
 
> If we have this range, later for ret & (sizeof(filename) - 1) with ret >= 1,
> the verifier should be able to conclude
>  ret & (sizeof(filename) - 1) >= 1.
 
> To workaround the immediate problem, I tested the following hack
> with bcc and it works fine.
 
> BPF_PERF_OUTPUT(events);
> int trace(struct pt_regs *ctx) {
>   char filename[128];
>   int ret = bpf_probe_read_str(filename, sizeof(filename), 0);
>   if (ret > 0) {
>     if (ret == 1)
>           events.perf_submit(ctx, filename, ret);
>     else if (ret < 128)
>           events.perf_submit(ctx, filename, ret);
>   }
>   return 1;
> }
 
> The idea is to make control flow more complex to prevent llvm
> do certain optimizations.

So, the hack makes it work for me, using clang 6.0:

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 -

[root@...et bpf]# perf probe -V do_sys_open
Available variables at do_sys_open
        @<do_sys_open+0>
                char*   filename
                int     dfd
                int     flags
                struct open_flags       op
                umode_t mode
[root@...et bpf]# cat open.c 
#include "bpf.h"

SEC("prog=do_sys_open filename")
int prog(void *ctx, int err, char *filename_ptr)
{
	char filename[128];
	int len = bpf_probe_read_str(filename, sizeof(filename), filename_ptr); 
	if (len > 0) {
		if (len == 1)
       			perf_event_output(ctx, &__bpf_stdout__, BPF_F_CURRENT_CPU, filename, len);
		else if (len < 128)
       			perf_event_output(ctx, &__bpf_stdout__, BPF_F_CURRENT_CPU, filename, len);
	}
	return 1;
}
[root@...et bpf]#
[root@...et bpf]# perf trace -e *open,open.c touch /tmp/Thanks.Yonghong.Song\!
LLVM: dumping open.o
     0.000 ( 0.009 ms): touch/9034 open(filename: 0x5b678e37, flags: CLOEXEC                             ) ...
     0.009 (         ): __bpf_stdout__:/etc/ld.so.cache....)
     0.011 (         ): perf_bpf_probe:prog:(ffffffff8f260da0) filename=0x7f805b678e37)
     0.000 ( 0.016 ms): touch/9034  ... [continued]: open()) = 3
     0.034 ( 0.002 ms): touch/9034 open(filename: 0x5b87c640, flags: CLOEXEC                             ) ...
     0.036 (         ): __bpf_stdout__:/lib64/libc.so.6....)
     0.037 (         ): perf_bpf_probe:prog:(ffffffff8f260da0) filename=0x7f805b87c640)
     0.034 ( 0.009 ms): touch/9034  ... [continued]: open()) = 3
     0.251 ( 0.002 ms): touch/9034 open(filename: 0x5b422c70, flags: CLOEXEC                             ) ...
     0.253 (         ): __bpf_stdout__:/usr/lib/locale/locale-archive......)
     0.254 (         ): perf_bpf_probe:prog:(ffffffff8f260da0) filename=0x7f805b422c70)
     0.251 ( 0.009 ms): touch/9034  ... [continued]: open()) = 3
     0.296 ( 0.002 ms): touch/9034 open(filename: 0x1d3a00f1, flags: CREAT|NOCTTY|NONBLOCK|WRONLY, mode: IRUGO|IWUGO) ...
     0.298 (         ): __bpf_stdout__:/tmp/Thanks.Yonghong.Song!..)
     0.299 (         ): perf_bpf_probe:prog:(ffffffff8f260da0) filename=0x7ffd1d3a00f1)
     0.296 ( 0.009 ms): touch/9034  ... [continued]: open()) = 3
[root@...et bpf]#