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]

Date:   Thu, 14 Mar 2019 12:27:56 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>, bpf@...r.kernel.org,
        Networking <netdev@...r.kernel.org>,
        Joe Stringer <joe@...d.net.nz>,
        john fastabend <john.fastabend@...il.com>,
        Yonghong Song <yhs@...com>,
        Jakub Kicinski <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 4:06 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> 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?

Just curious, what about the cases of special arrays (e.g., at least
PROG_ARRAY). Is doing tail call a read from that map?

> 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.
>