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