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: <20190311230643.hpbcxczrq46hnn7r@ast-mbp>
Date:   Mon, 11 Mar 2019 16:06:44 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
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 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?

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.
Storing dangling task_struct pointer in the next patch doesn't look great.
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.