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: <20171121223157.v3nodaugpuzl5dwg@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 21 Nov 2017 14:31:59 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Yonghong Song <yhs@...com>, 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

On Tue, Nov 21, 2017 at 11:29:05AM -0300, Arnaldo Carvalho de Melo wrote:
> 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);
...
> 
> 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);

yeah sorry about this hack. Gianluca reported this issue as well.
Yonghong fixed it for bpf_probe_read only. We will extend
the fix to bpf_probe_read_str() and bpf_perf_event_output() asap.
The above workaround gets too much into llvm and verifier details
we should strive to make bpf program writing as easy as possible.