netdev - Re: [PATCH bpf] bpf: do not modify min/max bounds on scalars with constant values

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: <d2d8d6ac-f074-5abd-8905-2a6e31c4b525@iogearbox.net>
Date:   Mon, 15 Jan 2018 11:40:26 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Y Song <ys114321@...il.com>
Cc:     Alexei Starovoitov <ast@...com>,
        Edward Cree <ecree@...arflare.com>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf] bpf: do not modify min/max bounds on scalars with
 constant values

On 01/15/2018 07:38 AM, Y Song wrote:
> On Fri, Jan 12, 2018 at 11:23 AM, Daniel Borkmann <daniel@...earbox.net> wrote:
[...]
>>
>> I've been thinking to additionally reject arithmetic on ctx
>> pointer in adjust_ptr_min_max_vals() right upfront as well
>> since we reject actual access in such case later on anyway,
>> but there's a use case in tracing (in bcc) in combination
>> with passing such ctx to bpf_probe_read(), so we cannot do
>> that part.
> 
> There is a reason why bcc does this. For example, suppose that we want to
> trace kernel tracepoint, sched_process_exec,
> 
> TRACE_EVENT(sched_process_exec,
> 
>         TP_PROTO(struct task_struct *p, pid_t old_pid,
>                  struct linux_binprm *bprm),
> 
>         TP_ARGS(p, old_pid, bprm),
> 
>         TP_STRUCT__entry(
>                 __string(       filename,       bprm->filename  )
>                 __field(        pid_t,          pid             )
>                 __field(        pid_t,          old_pid         )
>         ),
> 
>         TP_fast_assign(
>                 __assign_str(filename, bprm->filename);
>                 __entry->pid            = p->pid;
>                 __entry->old_pid        = old_pid;
>         ),
> 
>         TP_printk("filename=%s pid=%d old_pid=%d", __get_str(filename),
>                   __entry->pid, __entry->old_pid)
> );
> 
> Eventually structure at
> /sys/kernel/debug/tracing/event/sched/sched_process_exec/format:
> ......
>         field:__data_loc char[] filename;       offset:8;
> size:4; signed:1;
>         field:pid_t pid;        offset:12;      size:4; signed:1;
>         field:pid_t old_pid;    offset:16;      size:4; signed:1;
> 
> and "data_loc filename" is the offset in the structure where
> "filename" is stored.
> 
> Therefore, in bcc, the access sequence is:
>     offset = args->filename;  /* field __data_loc filename */
>     bpf_probe_read(&dst, len, (char *)args + offset);
> 
> For this kind of dynamic array in the tracepoint, the offset to access
> certain field in ctx will be unknown at verification time.

Right, that is exactly what I said in above paragraph.

> So I suggest to remove the above paragraph regarding to potential ctx+offset
> rejection.

I'm confused, I mentioned we cannot reject exactly because of this
use-case, I thought it's worth having it in the log for future
reference so we don't forget about it since it's not too obvious.

Cheers,
Daniel