netdev - Re: [PATCH 1/6] bpf: Allow ctx access for pointers to scalar

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]

Date:   Wed, 22 Jan 2020 02:33:32 +0000
From:   Yonghong Song <yhs@...com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Jiri Olsa <jolsa@...nel.org>
CC:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>,
        "Network Development" <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, "Andrii Nakryiko" <andriin@...com>,
        Martin Lau <kafai@...com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        David Miller <davem@...hat.com>,
        Björn Töpel <bjorn.topel@...el.com>
Subject: Re: [PATCH 1/6] bpf: Allow ctx access for pointers to scalar



On 1/21/20 5:51 PM, Alexei Starovoitov wrote:
> On Tue, Jan 21, 2020 at 4:05 AM Jiri Olsa <jolsa@...nel.org> wrote:
>>
>> When accessing the context we allow access to arguments with
>> scalar type and pointer to struct. But we omit pointer to scalar
>> type, which is the case for many functions and same case as
>> when accessing scalar.
>>
>> Adding the check if the pointer is to scalar type and allow it.
>>
>> Acked-by: John Fastabend <john.fastabend@...il.com>
>> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
>> ---
>>   kernel/bpf/btf.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 832b5d7fd892..207ae554e0ce 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -3668,7 +3668,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>                      const struct bpf_prog *prog,
>>                      struct bpf_insn_access_aux *info)
>>   {
>> -       const struct btf_type *t = prog->aux->attach_func_proto;
>> +       const struct btf_type *tp, *t = prog->aux->attach_func_proto;
>>          struct bpf_prog *tgt_prog = prog->aux->linked_prog;
>>          struct btf *btf = bpf_prog_get_target_btf(prog);
>>          const char *tname = prog->aux->attach_func_name;
>> @@ -3730,6 +3730,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>                   */
>>                  return true;
>>
>> +       tp = btf_type_by_id(btf, t->type);
>> +       /* skip modifiers */
>> +       while (btf_type_is_modifier(tp))
>> +               tp = btf_type_by_id(btf, tp->type);
>> +
>> +       if (btf_type_is_int(tp) || btf_type_is_enum(tp))
>> +               /* This is a pointer scalar.
>> +                * It is the same as scalar from the verifier safety pov.
>> +                */
>> +               return true;
> 
> The reason I didn't do it earlier is I was thinking to represent it
> as PTR_TO_BTF_ID as well, so that corresponding u8..u64
> access into this memory would still be possible.
> I'm trying to analyze the situation that returning a scalar now
> and converting to PTR_TO_BTF_ID in the future will keep progs
> passing the verifier. Is it really the case?
> Could you give a specific example that needs this support?
> It will help me understand this backward compatibility concern.
> What prog is doing with that 'u32 *' that is seen as scalar ?
> It cannot dereference it. Use it as what?

If this is from original bcc code, it will use bpf_probe_read for 
dereference. This is what I understand when I first reviewed this patch.
But it will be good to get Jiri's confirmation.