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

From: Maciej Żenczykowski <redacted>

This reverts commit d37300ed182131f1757895a62e556332857417e5.

This breaks Android userspace which expects to be able to
fetch programs with just read permissions.

See: https://cs.android.com/android/platform/superproject/+/master:frameworks/libs/net/common/native/bpf_syscall_wrappers/include/BpfSyscallWrappers.h;drc=7005c764be23d31fa1d69e826b4a2f6689a8c81e;l=124

Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Greg Kroah-Hartman <gregkh@google.com>
Cc: Lorenz Bauer <redacted>
Fixes: d37300ed1821 ("bpf: program: Refuse non-O_RDWR flags in BPF_OBJ_GET")
Signed-off-by: Maciej Żenczykowski <redacted>
---
 kernel/bpf/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index b4ebd60a6c16..80da1db47c68 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c

@@ -543,7 +543,7 @@ int bpf_obj_get_user(const char __user *pathname, int flags)
 		return PTR_ERR(raw);
 
 	if (type == BPF_TYPE_PROG)
-		ret = (f_flags != O_RDWR) ? -EINVAL : bpf_prog_new_fd(raw);
+		ret = bpf_prog_new_fd(raw);
 	else if (type == BPF_TYPE_MAP)
 		ret = bpf_map_new_fd(raw, f_flags);
 	else if (type == BPF_TYPE_LINK)

-- 
2.32.0.288.g62a8d224e6-goog

On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski
[off-list ref] 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.

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

www.cloudflare.com

On Fri, Jun 18, 2021 at 4:55 AM Lorenz Bauer [off-list ref] wrote:

On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski
[off-list ref] wrote:

quoted

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.

AFAIK there is no way to modify a program after it was loaded, has this changed?
if so, the checks should be on the modifications not the fd fetch.

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)

On 6/18/21 12:55 PM, Maciej Żenczykowski wrote:

From: Maciej Żenczykowski <redacted>

This reverts commit d37300ed182131f1757895a62e556332857417e5.

This breaks Android userspace which expects to be able to
fetch programs with just read permissions.

See: https://cs.android.com/android/platform/superproject/+/master:frameworks/libs/net/common/native/bpf_syscall_wrappers/include/BpfSyscallWrappers.h;drc=7005c764be23d31fa1d69e826b4a2f6689a8c81e;l=124

Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Greg Kroah-Hartman <gregkh@google.com>
Cc: Lorenz Bauer <redacted>
Fixes: d37300ed1821 ("bpf: program: Refuse non-O_RDWR flags in BPF_OBJ_GET")
Signed-off-by: Maciej Żenczykowski <redacted>
---

Acked-by: Greg Kroah-Hartman <gregkh@google.com>

On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski
[off-list ref] wrote:

From: Maciej Żenczykowski <redacted>

This reverts commit d37300ed182131f1757895a62e556332857417e5.

This breaks Android userspace which expects to be able to
fetch programs with just read permissions.

See: https://cs.android.com/android/platform/superproject/+/master:frameworks/libs/net/common/native/bpf_syscall_wrappers/include/BpfSyscallWrappers.h;drc=7005c764be23d31fa1d69e826b4a2f6689a8c81e;l=124

As a follow up, what does Android expect to be able to do with this
read only FD?

Lorenz

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

www.cloudflare.com

Reposting since apparently my reply only went to Lorenz.

On Wed, Jun 23, 2021 at 1:45 AM Lorenz Bauer [off-list ref] wrote:

On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski
[off-list ref] wrote:

quoted

From: Maciej Żenczykowski <redacted>

This reverts commit d37300ed182131f1757895a62e556332857417e5.

This breaks Android userspace which expects to be able to
fetch programs with just read permissions.

See: https://cs.android.com/android/platform/superproject/+/master:frameworks/libs/net/common/native/bpf_syscall_wrappers/include/BpfSyscallWrappers.h;drc=7005c764be23d31fa1d69e826b4a2f6689a8c81e;l=124

As a follow up, what does Android expect to be able to do with this
read only FD?

I'm not actually sure of all the use cases, but at a bare minimum:
We use it for iptables xt_bpf, and to attach to cgroup net hooks and
tc bpf hooks.
There's also some still incomplete support for xdp.
There's also non-networking stuff like gpu memory tracking and
tracepoints that I know very little about - probably something perf
related.
So I think the answer is that mostly we expect to be able to attach it
to places (iptables/cgroup/tc/xdp/....others...??)