netdev - Re: [PATCH rfc v3 bpf-next 2/9] bpf: add program side {rd,wr}only support for maps

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: <f2f8e86e-9cae-7ec3-3753-baed4a317efc@iogearbox.net>
Date:   Tue, 12 Mar 2019 00:34:34 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     ast@...nel.org, bpf@...r.kernel.org, netdev@...r.kernel.org,
        joe@...d.net.nz, john.fastabend@...il.com, yhs@...com,
        andrii.nakryiko@...il.com, jakub.kicinski@...ronome.com,
        tgraf@...g.ch, lmb@...udflare.com
Subject: Re: [PATCH rfc v3 bpf-next 2/9] bpf: add program side {rd,wr}only
 support for maps

On 03/12/2019 12:06 AM, Alexei Starovoitov wrote:
> On Mon, Mar 11, 2019 at 10:51:18PM +0100, Daniel Borkmann wrote:
>> This work adds two new map creation flags BPF_F_RDONLY_PROG
>> and BPF_F_WRONLY_PROG in order to allow for read-only or
>> write-only BPF maps from a BPF program side.
>>
>> Today we have BPF_F_RDONLY and BPF_F_WRONLY, but this only
>> applies to system call side, meaning the BPF program has full
>> read/write access to the map as usual while bpf(2) calls with
>> map fd can either only read or write into the map depending
>> on the flags. BPF_F_RDONLY_PROG and BPF_F_WRONLY_PROG allows
>> for the exact opposite such that verifier is going to reject
>> program loads if write into a read-only map or a read into a
>> write-only map is detected. For read-only map case also some
>> helpers are forbidden for programs that would alter the map
>> state such as map deletion, update, etc.
>>
>> We've enabled this generic map extension to various non-special
>> maps holding normal user data: array, hash, lru, lpm, local
>> storage, queue and stack. Further map types could be followed
>> up in future depending on use-case. Main use case here is to
>> forbid writes into .rodata map values from verifier side.
> 
> I think WRONLY | WRONLY_PROG should be invalid combination?
> Since these attributes are set at creation time and cannot be changed,
> nothing can ever read from it, so why write into it?
> Similarly RDONLY | RDONLY_PROG is invalid too?

Yeah, I can add this. Note that 'these attributes are set at creation
time and cannot be changed' does not fully hold for WRONLY/RDONLY,
e.g. you can create a map as RDONLY, but later on retrieve fd by id
or from bpf fs but without RDONLY, so this fd will be able to write
into it from syscall side (we're checking struct file flags, not the
attributes' map flags at runtime). Given that I thought it made more
sense to keep these two logically separated since one is only revelant
for lifetime of fd and the other one for map. (I also think keeping
RDONLY/WRONLY stored in map_flags attr and exposing it is pretty
confusing since it doesn't say anything, this should probably be
masked out as a fix, thoughts?)

> Also looking at the next patch and 'lock' command...
> May be it would be cleaner to do add WRONCE (from syscall) flag?
> Then for .rodata the attrs will be RDONLY_PROG | WRONCE
> and no 'lock' necessary.
> WRONCE_PROG probably doesn't make sense.

Agree, from prog side not and would probably also just make fast-path
slower. Hm, for the WRONCE at creation flags, we'd need some form of
locking on syscall side to avoid racing I'd think. The lock cmd,
simply just does the write_once() and rcu sync to wait for everything
to complete, so this is pretty trivial w/o slowing down anything on
syscall side. I can take a look, but feels worse to me right now.
The other thing is, given RDONLY/WRONLY semantics are _only_ for fd
and _not_ map, mixing these might be confusing as well, since WRONCE
would then be a map property whereas RDONLY/WRONLY not.

> Storing dangling task_struct pointer in the next patch doesn't look great.

Hm, it's actually not dangling, on close we exchange it with NULL
(which current can never be) such that either root or only original
map creator may lock it as read-only.

> The whole 'lock' concept feels useful, but in the context of implementing
> .rodata it feels that WRONCE would be a better fit,
> since libbpf won't be able to make a mistake and forget to 'lock'.
> 'lock' syscall cmd can be confused with BPF_F_LOCK too.

That's a naming detail, but sure it can be changed. :)

Thanks,
Daniel