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
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); }
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); }
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
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; +}
linux-kselftest-mirror@lists.linaro.org
-
Daniel Borkmann -
Roberto Sassu