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] [day] [month] [year] [list]

Message-ID: <CAH3MdRXTdh5equgxvwstrxMc+jhueV+PNqfp1wuAp-CcJzDk1g@mail.gmail.com>
Date:   Mon, 15 Jan 2018 09:33:49 -0800
From:   Y Song <ys114321@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
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 Mon, Jan 15, 2018 at 2:40 AM, Daniel Borkmann <daniel@...earbox.net> wrote:
> 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.

I thought it was irrelevant to the main subject of the patch (e.g., dealing with
constant min/max, etc.) But I agree that having a log for a reference is
better since it is not too obvious.

Could you describe the "use case in bcc" part a little more in detail then?
Otherwise, it may not be really clear for some people.

Sorry for causing your confusion!

Thanks,

>
> Cheers,
> Daniel