[PATCH 0/2] bpf: Retry access to a map in read-only mode

[PATCH 0/2] bpf: Retry access to a map in read-only mode

[Roberto Sassu]

If a map is write-protected, for example by an eBPF program implementing
the bpf_map security hook, some read-like operations like show and dump
cannot be performed by bpftool even if bpftool has the right to do so.

The reason is that bpftool sets the open flags to zero, at the time it gets
a map file descriptor. The kernel interprets this as a request for full
access to the map (with read and write permissions).

The simple solution is to set only the necessary open flags for a requested
operation, so that only those operations requiring more privileges than the
ones granted by the enforcing eBPF programs are denied.

There are different ways to solve the problem. One would be to introduce a
new function to acquire a read-only file descriptor and use it from the
functions implementing read-like operations.

Or more simply, another is to attempt to get a read-only file descriptor in
the original function when the first request with full permissions failed.

This patch set implements the second solution in patch 1, and adds a
corresponding test in patch 2. Depending on the feedback, the first
solution can be implemented.

Roberto Sassu (2):
  libbpf: Retry map access with read-only permission
  selftests/bpf: Add test for retrying access to map with read-only perm

 tools/lib/bpf/bpf.c                           |  5 ++
 .../bpf/prog_tests/test_map_retry_access.c    | 54 +++++++++++++++++++
 .../selftests/bpf/progs/map_retry_access.c    | 36 +++++++++++++
 3 files changed, 95 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_map_retry_access.c
 create mode 100644 tools/testing/selftests/bpf/progs/map_retry_access.c

-- 
2.25.1

[PATCH 1/2] libbpf: Retry map access with read-only permission

[Roberto Sassu]

Retry map access with read-only permission, if access was denied when all
permissions were requested (open_flags is set to zero). Write access might
have been denied by the bpf_map security hook.

Some operations, such as show and dump, don't need write permissions, so
there is a good chance of success with retrying.

Prefer this solution to extending the API, as otherwise a new mechanism
would need to be implemented to determine the right permissions for an
operation.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 tools/lib/bpf/bpf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 240186aac8e6..b4eec39021a4 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -1056,6 +1056,11 @@ int bpf_map_get_fd_by_id(__u32 id)
 	attr.map_id = id;
 
 	fd = sys_bpf_fd(BPF_MAP_GET_FD_BY_ID, &attr, sizeof(attr));
+	if (fd < 0) {
+		attr.open_flags = BPF_F_RDONLY;
+		fd = sys_bpf_fd(BPF_MAP_GET_FD_BY_ID, &attr, sizeof(attr));
+	}
+
 	return libbpf_err_errno(fd);
 }
 
-- 
2.25.1

Re: [PATCH 1/2] libbpf: Retry map access with read-only permission

[Daniel Borkmann]

On 5/30/22 10:45 AM, Roberto Sassu wrote:
> Retry map access with read-only permission, if access was denied when all
> permissions were requested (open_flags is set to zero). Write access might
> have been denied by the bpf_map security hook.
> 
> Some operations, such as show and dump, don't need write permissions, so
> there is a good chance of success with retrying.
> 
> Prefer this solution to extending the API, as otherwise a new mechanism
> would need to be implemented to determine the right permissions for an
> operation.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   tools/lib/bpf/bpf.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 240186aac8e6..b4eec39021a4 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -1056,6 +1056,11 @@ int bpf_map_get_fd_by_id(__u32 id)
>   	attr.map_id = id;
>   
>   	fd = sys_bpf_fd(BPF_MAP_GET_FD_BY_ID, &attr, sizeof(attr));
> +	if (fd < 0) {
> +		attr.open_flags = BPF_F_RDONLY;
> +		fd = sys_bpf_fd(BPF_MAP_GET_FD_BY_ID, &attr, sizeof(attr));
> +	}
> +

But then what about bpf_obj_get() API in libbpf? attr.file_flags has similar
purpose as attr.open_flags in this case.

The other issue is that this could have upgrade implications, e.g. where an
application bailed out before, it is now passing wrt bpf_map_get_fd_by_id(),
but then suddenly failing during map update calls.

Imho, it might be better to be explicit about user intent w/o the lib doing
guess work upon failure cases (... or have the BPF LSM set the attr.open_flags
to BPF_F_RDONLY from within the BPF prog).

>   	return libbpf_err_errno(fd);
>   }
>   
> 

RE: [PATCH 1/2] libbpf: Retry map access with read-only permission

[Roberto Sassu]

> From: Daniel Borkmann [mailto:daniel@iogearbox.net]
> Sent: Monday, May 30, 2022 11:55 PM
> On 5/30/22 10:45 AM, Roberto Sassu wrote:
> > Retry map access with read-only permission, if access was denied when all
> > permissions were requested (open_flags is set to zero). Write access might
> > have been denied by the bpf_map security hook.
> >
> > Some operations, such as show and dump, don't need write permissions, so
> > there is a good chance of success with retrying.
> >
> > Prefer this solution to extending the API, as otherwise a new mechanism
> > would need to be implemented to determine the right permissions for an
> > operation.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >   tools/lib/bpf/bpf.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index 240186aac8e6..b4eec39021a4 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -1056,6 +1056,11 @@ int bpf_map_get_fd_by_id(__u32 id)
> >   	attr.map_id = id;
> >
> >   	fd = sys_bpf_fd(BPF_MAP_GET_FD_BY_ID, &attr, sizeof(attr));
> > +	if (fd < 0) {
> > +		attr.open_flags = BPF_F_RDONLY;
> > +		fd = sys_bpf_fd(BPF_MAP_GET_FD_BY_ID, &attr, sizeof(attr));
> > +	}
> > +
> 
> But then what about bpf_obj_get() API in libbpf? attr.file_flags has similar
> purpose as attr.open_flags in this case.

Ok, I missed it.

> The other issue is that this could have upgrade implications, e.g. where an
> application bailed out before, it is now passing wrt bpf_map_get_fd_by_id(),
> but then suddenly failing during map update calls.

Good point.

> Imho, it might be better to be explicit about user intent w/o the lib doing
> guess work upon failure cases (... or have the BPF LSM set the attr.open_flags
> to BPF_F_RDONLY from within the BPF prog).

Uhm, I don't like that the users should be aware of permissions assigned
to maps that they don't own.

Maybe, better the original idea, request read-only permission for the
list and dump operations.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

[PATCH 2/2] selftests/bpf: Add test for retrying access to map with read-only perm

[Roberto Sassu]

Add a test to check the ability of bpf_map_get_fd_by_id() to get a map file
descriptor if write permission is denied.

Also ensure that a map update operation fails with the obtained read-only
file descriptor.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 .../bpf/prog_tests/test_map_retry_access.c    | 54 +++++++++++++++++++
 .../selftests/bpf/progs/map_retry_access.c    | 36 +++++++++++++
 2 files changed, 90 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_map_retry_access.c
 create mode 100644 tools/testing/selftests/bpf/progs/map_retry_access.c

diff --git a/tools/testing/selftests/bpf/prog_tests/test_map_retry_access.c b/tools/testing/selftests/bpf/prog_tests/test_map_retry_access.c
new file mode 100644
index 000000000000..beffb2026dcd
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_map_retry_access.c
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
+ *
+ * Author: Roberto Sassu <roberto.sassu@huawei.com>
+ */
+
+#include <test_progs.h>
+
+#include "map_retry_access.skel.h"
+
+void test_test_map_retry_access(void)
+{
+	struct map_retry_access *skel;
+	struct bpf_map_info info;
+	struct bpf_map *map;
+	__u32 len = sizeof(info);
+	int ret, zero = 0, fd, duration = 0;
+
+	skel = map_retry_access__open_and_load();
+	if (CHECK(!skel, "skel", "open_and_load failed\n"))
+		goto close_prog;
+
+	ret = map_retry_access__attach(skel);
+	if (CHECK(ret < 0, "skel", "attach failed\n"))
+		goto close_prog;
+
+	map = bpf_object__find_map_by_name(skel->obj, "data_input");
+	if (CHECK(!map, "bpf_object__find_map_by_name", "not found\n"))
+		goto close_prog;
+
+	ret = bpf_obj_get_info_by_fd(bpf_map__fd(map), &info, &len);
+	if (CHECK(ret < 0, "bpf_obj_get_info_by_fd", "error: %d\n", ret))
+		goto close_prog;
+
+	fd = bpf_map_get_fd_by_id(info.id);
+	if (CHECK(fd < 0, "bpf_map_get_fd_by_id", "error: %d\n", fd))
+		goto close_prog;
+
+	ret = bpf_map_update_elem(fd, &zero, &len, BPF_ANY);
+
+	close(fd);
+
+	if (CHECK(!ret, "bpf_map_update_elem",
+		  "should fail (read-only permission)\n"))
+		goto close_prog;
+
+	ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &len, BPF_ANY);
+
+	CHECK(ret < 0, "bpf_map_update_elem", "error: %d\n", ret);
+close_prog:
+	map_retry_access__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/map_retry_access.c b/tools/testing/selftests/bpf/progs/map_retry_access.c
new file mode 100644
index 000000000000..1ed7b137a286
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/map_retry_access.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
+ *
+ * Author: Roberto Sassu <roberto.sassu@huawei.com>
+ */
+
+#include "vmlinux.h"
+#include <errno.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+/* From include/linux/mm.h. */
+#define FMODE_WRITE	0x2
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, __u32);
+} data_input SEC(".maps");
+
+char _license[] SEC("license") = "GPL";
+
+SEC("lsm/bpf_map")
+int BPF_PROG(bpf_map_retry_access, struct bpf_map *map, fmode_t fmode)
+{
+	if (map != (struct bpf_map *)&data_input)
+		return 0;
+
+	if (fmode & FMODE_WRITE)
+		return -EACCES;
+
+	return 0;
+}
-- 
2.25.1