Re: [PATCH v2 0/9] bpf: Per-operation map permissions

[Andrii Nakryiko]

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