Re: [PATCH bpf] Revert "bpf: program: Refuse non-O_RDWR flags in BPF_OBJ_GET"

From: Lorenz Bauer <lmb@cloudflare.com>
To: "Maciej Żenczykowski" <zenczykowski@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Linux Network Development Mailing List  <netdev@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	BPF Mailing List <bpf@vger.kernel.org>,
	"David S . Miller" <davem@davemloft.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Greg Kroah-Hartman <gregkh@google.com>
Subject: Re: [PATCH bpf] Revert "bpf: program: Refuse non-O_RDWR flags in BPF_OBJ_GET"
Date: Mon, 21 Jun 2021 10:02:12 +0100	[thread overview]
Message-ID: <CACAyw9948drqRE=0tC=5OrdX=nOVR3JSPScXrkdAv+kGD_P3ZA@mail.gmail.com> (raw)
In-Reply-To: <CANP3RGdrpb+KiD+a29zTSU3LKR8Qo6aFdo4QseRvPdNhZ_AOJw@mail.gmail.com>

On Fri, 18 Jun 2021 at 19:30, Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
>
> On Fri, Jun 18, 2021 at 4:55 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski
> > <zenczykowski@gmail.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.

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

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

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com