> From: Andrii Nakryiko [mailto:andrii.nakryiko@xxxxxxxxx] > Sent: Friday, June 3, 2022 11:00 PM > On Thu, Jun 2, 2022 at 7:38 AM Roberto Sassu <roberto.sassu@xxxxxxxxxx> > wrote: > > > > With the bpf_map security hook, an eBPF program is able to restrict access > > to a map. For example, it might allow only read accesses and deny write > > accesses. > > > > Unfortunately, permissions are not accurately specified by libbpf and > > bpftool. As a consequence, even if they are requested to perform a > > read-like operation, such as a map lookup, that operation fails even if the > > caller has the right to do so. > > > > Even worse, the iteration over existing maps stops as soon as a > > write-protected one is encountered. Maps after the write-protected one are > > not accessible, even if the user has the right to perform operations on > > them. > > > > At low level, the problem is that open_flags and file_flags, respectively > > in the bpf_map_get_fd_by_id() and bpf_obj_get(), are set to zero. The > > kernel interprets this as a request to obtain a file descriptor with full > > permissions. > > > > For some operations, like show or dump, a read file descriptor is enough. > > Those operations could be still performed even in a write-protected map. > > > > Also for searching a map by name, which requires getting the map info, a > > read file descriptor is enough. If an operation requires more permissions, > > they could still be requested later, after the search. > > > > First, solve both problems by extending libbpf with two new functions, > > bpf_map_get_fd_by_id_flags() and bpf_obj_get_flags(), which unlike their > > counterparts bpf_map_get_fd_by_id() and bpf_obj_get(), have the additional > > parameter flags to specify the needed permissions for an operation. > > > > Then, propagate the flags in bpftool from the functions implementing the > > subcommands down to the functions calling bpf_map_get_fd_by_id() and > > bpf_obj_get(), and replace the latter functions with their new variant. > > Initially, set the flags to zero, so that the current behavior does not > > change. > > > > The only exception is for map search by name, where a read-only permission > > is requested, regardless of the operation, to get the map info. In this > > case, request a new file descriptor if a write-like operation needs to be > > performed after the search. > > > > Finally, identify other read-like operations in bpftool and for those > > replace the zero value for flags with BPF_F_RDONLY. > > > > The patch set is organized as follows. > > > > Patches 1-2 introduce the two new variants of bpf_map_get_fd_by_id() and > > bpf_obj_get() in libbpf, named respectively bpf_map_get_fd_by_id_flags() > > and bpf_obj_get_flags(). > > > > Patches 3-7 propagate the flags in bpftool from the functions implementing > > the subcommands to the two new libbpf functions, and always set flags to > > BPF_F_RDONLY for the map search operation. > > > > Patch 8 adjusts permissions depending on the map operation performed. > > > > Patch 9 ensures that read-only accesses to a write-protected map succeed > > and write accesses still fail. Also ensure that map search is always > > successful even if there are write-protected maps. > > > > Changelog > > > > v1: > > - Define per-operation permissions rather than retrying access with > > read-only permission (suggested by Daniel) > > https://lore.kernel.org/bpf/20220530084514.10170-1- > roberto.sassu@xxxxxxxxxx/ > > > > Roberto Sassu (9): > > libbpf: Introduce bpf_map_get_fd_by_id_flags() > > libbpf: Introduce bpf_obj_get_flags() > > bpftool: Add flags parameter to open_obj_pinned_any() and > > open_obj_pinned() > > bpftool: Add flags parameter to *_parse_fd() functions > > bpftool: Add flags parameter to map_parse_fds() > > bpftool: Add flags parameter to map_parse_fd_and_info() > > bpftool: Add flags parameter in struct_ops functions > > bpftool: Adjust map permissions > > selftests/bpf: Add map access tests > > > > tools/bpf/bpftool/btf.c | 11 +- > > tools/bpf/bpftool/cgroup.c | 4 +- > > tools/bpf/bpftool/common.c | 52 ++-- > > tools/bpf/bpftool/iter.c | 2 +- > > tools/bpf/bpftool/link.c | 9 +- > > tools/bpf/bpftool/main.h | 17 +- > > tools/bpf/bpftool/map.c | 24 +- > > tools/bpf/bpftool/map_perf_ring.c | 3 +- > > tools/bpf/bpftool/net.c | 2 +- > > tools/bpf/bpftool/prog.c | 12 +- > > tools/bpf/bpftool/struct_ops.c | 39 ++- > > tools/lib/bpf/bpf.c | 16 +- > > tools/lib/bpf/bpf.h | 2 + > > tools/lib/bpf/libbpf.map | 2 + > > .../bpf/prog_tests/test_map_check_access.c | 264 ++++++++++++++++++ > > .../selftests/bpf/progs/map_check_access.c | 65 +++++ > > 16 files changed, 452 insertions(+), 72 deletions(-) > > create mode 100644 > tools/testing/selftests/bpf/prog_tests/test_map_check_access.c > > create mode 100644 tools/testing/selftests/bpf/progs/map_check_access.c > > > > -- > > 2.25.1 > > > > Also check CI failures ([0]). Thanks for the review Andrii. Will send a new version of the patch set soon. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He > test_test_map_check_access:PASS:skel 0 nsec > test_test_map_check_access:PASS:skel 0 nsec > test_test_map_check_access:PASS:bpf_object__find_map_by_name 0 nsec > test_test_map_check_access:PASS:bpf_obj_get_info_by_fd 0 nsec > test_test_map_check_access:PASS:bpf_map_get_fd_by_id 0 nsec > test_test_map_check_access:PASS:bpf_map_get_fd_by_id_flags 0 nsec > test_test_map_check_access:PASS:bpf_map_get_fd_by_id_flags 0 nsec > test_test_map_check_access:PASS:bpf_map_lookup_elem 0 nsec > test_test_map_check_access:PASS:bpf_map_update_elem 0 nsec > test_test_map_check_access:PASS:bpf_map_update_elem 0 nsec > test_test_map_check_access:PASS:bpf_map__pin 0 nsec > test_test_map_check_access:PASS:bpf_obj_get_flags 0 nsec > test_test_map_check_access:PASS:bpf_obj_get_flags 0 nsec > test_test_map_check_access:FAIL:bpftool map list - error: 127 > #189 test_map_check_access:FAIL > > [0] https://github.com/kernel- > patches/bpf/runs/6730796689?check_suite_focus=true
