Date:   Mon, 21 Jun 2021 14:37:37 -0700
From:   Maciej Żenczykowski <zenczykowski@...il.com>
To:     unlisted-recipients:; (no To-header on input)
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Linux Network Development Mailing List 
        <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        BPF Mailing List <bpf@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Greg Kroah-Hartman <gregkh@...gle.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Lorenz Bauer <lmb@...udflare.com>,
        Lorenzo Colitti <lorenzo@...gle.com>
Subject: Re: [PATCH bpf] Revert "bpf: program: Refuse non-O_RDWR flags in BPF_OBJ_GET"

The patch this reverts breaks (Android) userspace, and I've even
pointed out the specific code in question that it breaks.
What happened to the policy of not breaking userspace?

Why are 'Changes Requested' (see
https://patchwork.kernel.org/project/netdevbpf/patch/20210618105526.265003-1-zenczykowski@gmail.com/
)
by whom? What changes?  What do you expect me to do?

Why should I even care?  Why should it even be me?  I'm not the one
that broke things.

On Mon, Jun 21, 2021 at 2:02 AM Lorenz Bauer <lmb@...udflare.com> wrote:
>
> On Fri, 18 Jun 2021 at 19:30, Maciej Żenczykowski
> <zenczykowski@...il.com> wrote:
> >
> > On Fri, Jun 18, 2021 at 4:55 AM Lorenz Bauer <lmb@...udflare.com> wrote:
> > >
> > > On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski
> > > <zenczykowski@...il.com> wrote:
> > > >
> > > > This reverts commit d37300ed182131f1757895a62e556332857417e5.
> > > >
> > > > This breaks Android userspace which expects to be able to
> > > > fetch programs with just read permissions.
> > >
> > > Sorry about this! I'll defer to the maintainers what to do here.
> > > Reverting leaves us with a gaping hole for access control of pinned
> > > programs.
> >
> > Not sure what hole you're referring to.  Could you provide more details/explanation?
> >
> > It seems perfectly reasonable to be able to get a program with just read privs.
> > After all, you're not modifying it, just using it.
>
> Agreed, if that was what the kernel is doing. What you get with
> BPF_F_RDONLY is a fully read-write fd, since the rest of the BPF
> subsystem doesn't check program fd flags. Hence my fix to only allow
> O_RDWR, which matches what the kernel actually does. Otherwise any
> user with read-only access can get a R/W fd.
>
> > AFAIK there is no way to modify a program after it was loaded, has this changed?
>
> You can't modify the program, but you can detach it, for example. Any
> program related bpf command that takes a program fd basically.

I fail to see how this is a problem, since it's not modifying the program,
why should it need a rdwr file descriptor to do so?

AFAIK in many cases, you don't even need the bpf file descriptor at
all (for example
you can detach a tc bpf program via removing the tc filter or tc qdisc
or the network interface).

Do you perhaps mean to say you can unpin it?
But, if so, then that's a problem in the unpin code...

Or it's even entirely unrelated, since deleting files does not need
read or write access to the file, just the folder the file is in.

[maze@...s ~]$ touch foo; chmod a-rwx foo; sudo chown root:root foo; ls -al foo
----------. 1 root root 0 Jun 21 14:26 foo
[maze@...s ~]$ rm -f foo; ls -al foo
ls: cannot access 'foo': No such file or directory

> > if so, the checks should be on the modifications not the fd fetch.
>
> True, unfortunately that code doesn't exist. It's also not
> straightforward to write and probably impossible to backport.

Now you're suggesting you expect this broken patch (that I'm trying to revert)
to make it into older LTS releases and break things out in the field???

> > I guess one could argue fetching with write only privs doesn't make sense?
> >
> > Anyway... userspace is broken... so revert is the answer.
> >
> > In Android the process loading/pinning bpf maps/programs is a different
> > process (the 'bpfloader') to the users (which are far less privileged)
>
> If the revert happens you need to make sure that all of your pinned
> state is only readable by the bpfloader user. And everybody else,
> realistically.

On Android selinux prevents anyone from doing untoward things, since
they can't get the fds in the first place.

I *want* less privileged users (that can't load bpf programs, but have
the selinux privs to get pinned program fds) to be able to
attach/detach them (via xdp, tc or to cgroups). That's how stuff works
right *now*.

Could I perhaps redesign the system to work around this?
I don't know.  Perhaps.  Perhaps not.

I haven't given it that much thought - I'm still trying to fix
(workaround) an hrtimer ncm performance regression that was introduced
in 5.10.~24 LTS (there at least we can argue the old code was buggy,
and the hrtimer implementation on the hardware in question outright
terrible and super slow).

I'm guessing changes would be needed to the progRetrieve() function,
and the tests, and the bpfloader, and the permissions embedded in the
programs themselves, and possibly the iptables binary (and the
netutils wrapper) since it uses xt_bpf, and possibly to binaries
privileges and/or selinux capabilities/policies.  Or maybe not.  I
simply don't know.

Well tested patches to make things work are welcome at the aosp project.
A word of warning: I haven't checked / thought things through, but it
may take 5+ years to roll them out.
At this point it's far too late to make such changes in Android 12/S.

Here's another example of fetching programs with BPF_F_RDONLY in
iptables (yes, it was added by me, due to our use in Android):
  https://android.googlesource.com/platform/external/iptables.git/+/refs/heads/master/extensions/libxt_bpf.c#64

The fact that this snuck into the 5.12 final release is not relevant
(ie. this is a regression in 5.12 vs 5.11, and it is still broken in
5.13-rcX).

Please revert immediately.  I've got better things to do.  I shouldn't
have to be thinking about this or arguing about this.
It already took me significantly more than a day simply to track this
down (arguably due to miscommunications with Greg, who'd earlier
actually found this in 5.12, but misunderstood the problem, but
still...).

Thanks,
Maciej

Powered by blists - more mailing lists