netdev - Re: [PATCH v2 bpf-next 1/3] bpf: Introduce dynamic program extensions

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: <20200121160018.2w4o6o5nnhbdqicn@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 21 Jan 2020 08:00:19 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>, davem@...emloft.net,
        daniel@...earbox.net, netdev@...r.kernel.org, bpf@...r.kernel.org,
        kernel-team@...com
Subject: Re: [PATCH v2 bpf-next 1/3] bpf: Introduce dynamic program extensions

On Mon, Jan 20, 2020 at 11:36:43PM -0800, John Fastabend wrote:
> 
> > +
> > +	t1 = btf_type_skip_modifiers(btf1, t1->type, NULL);
> > +	t2 = btf_type_skip_modifiers(btf2, t2->type, NULL);
> 
> Is it really best to skip modifiers? I would expect that if the
> signature is different including modifiers then we should just reject it.
> OTOH its not really C code here either so modifiers may not have the same
> meaning. With just integers and struct it may be ok but if we add pointers
> to ints then what would we expect from a const int*?
> 
> So whats the reasoning for skipping modifiers? Is it purely an argument
> that its not required for safety so solve it elsewhere? In that case then
> checking names of functions is also equally not required.

Function names are not checked by the kernel. It's purely libbpf and bpf_prog.c
convention. The kernel operates on prog_fd+btf_id only. The names of function
arguments are not compared either.

The code has to skip modifiers. Otherwise the type comparison algorithm will be
quite complex, since typedef is such modifier. Like 'u32' in original program
and 'u32' in extension program would have to be recursively checked.

Another reason to skip modifiers is 'volatile' modifier. I suspect we would
have to use it from time to time in original placeholder functions. Yet new
replacement function will be written without volatile. The placeholder may need
volatile to make sure compiler doesn't optimize things away. I found cases
where 'noinline' in placeholder was not enough. clang would still inline the
body of the function and remove call instruction. So far I've been using
volatile as a workaround. May be we will introduce new function attribute to
clang.

Having said that I share your concern regarding skipping 'const'. For 'const
int arg' it's totally ok to skip it, since it's meaningless from safety pov,
but for 'const int *arg' and 'const struct foo *arg' I'm planning to preserve
it. It will be preserved at the verifier bpf_reg_state level though. Just
checking that 'const' is present in extension prog's BTF doesn't help safety.
I'm planing to make the verifier enforce that bpf prog cannot write into
argument which type is pointer to const struct. That part is still wip. It will
be implemented for global functions first and then for extension programs.
Currently the verifier rejects any pointer to struct (other than context), so
no backward compatibility issues.