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

[Roberto Sassu]

> 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