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

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@huawei.com/

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

On Thu, Jun 2, 2022 at 7:38 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> Introduce bpf_map_get_fd_by_id_flags(), to let a caller specify the open
> flags needed for the operation. This could make an operation succeed, if
> access to a map is restricted (i.e. it allows only certain operations).
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  tools/lib/bpf/bpf.c      | 8 +++++++-
>  tools/lib/bpf/bpf.h      | 1 +
>  tools/lib/bpf/libbpf.map | 1 +
>  3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 240186aac8e6..33bac2006043 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -1047,18 +1047,24 @@ int bpf_prog_get_fd_by_id(__u32 id)
>         return libbpf_err_errno(fd);
>  }
>
> -int bpf_map_get_fd_by_id(__u32 id)
> +int bpf_map_get_fd_by_id_flags(__u32 id, __u32 flags)

let's go the OPTS route instead so that we don't have to add any more
new variants? We can probably use common bpf_get_fd_by_id_opts for
map/prog/link/btf get_fd_by_id operations (and let's add all variants
for consistency)?


>  {
>         union bpf_attr attr;
>         int fd;
>
>         memset(&attr, 0, sizeof(attr));
>         attr.map_id = id;
> +       attr.open_flags = flags;
>
>         fd = sys_bpf_fd(BPF_MAP_GET_FD_BY_ID, &attr, sizeof(attr));
>         return libbpf_err_errno(fd);
>  }
>
> +int bpf_map_get_fd_by_id(__u32 id)
> +{
> +       return bpf_map_get_fd_by_id_flags(id, 0);
> +}
> +
>  int bpf_btf_get_fd_by_id(__u32 id)
>  {
>         union bpf_attr attr;
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index cabc03703e29..20e4c852362d 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -438,6 +438,7 @@ LIBBPF_API int bpf_map_get_next_id(__u32 start_id, __u32 *next_id);
>  LIBBPF_API int bpf_btf_get_next_id(__u32 start_id, __u32 *next_id);
>  LIBBPF_API int bpf_link_get_next_id(__u32 start_id, __u32 *next_id);
>  LIBBPF_API int bpf_prog_get_fd_by_id(__u32 id);
> +LIBBPF_API int bpf_map_get_fd_by_id_flags(__u32 id, __u32 flags);
>  LIBBPF_API int bpf_map_get_fd_by_id(__u32 id);
>  LIBBPF_API int bpf_btf_get_fd_by_id(__u32 id);
>  LIBBPF_API int bpf_link_get_fd_by_id(__u32 id);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 38e284ff057d..019278e66836 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -466,6 +466,7 @@ LIBBPF_1.0.0 {
>                 libbpf_bpf_link_type_str;
>                 libbpf_bpf_map_type_str;
>                 libbpf_bpf_prog_type_str;
> +               bpf_map_get_fd_by_id_flags;
>
>         local: *;
>  };
> --
> 2.25.1
>

On Thu, Jun 2, 2022 at 7:38 AM Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> Introduce the bpf_obj_get_flags() function, so that it is possible to
> specify the needed permissions for obtaining a file descriptor from a
> pinned object.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  tools/lib/bpf/bpf.c      | 8 +++++++-
>  tools/lib/bpf/bpf.h      | 1 +
>  tools/lib/bpf/libbpf.map | 1 +
>  3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 33bac2006043..a5fc40f6ce13 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -670,18 +670,24 @@ int bpf_obj_pin(int fd, const char *pathname)
>         return libbpf_err_errno(ret);
>  }
>
> -int bpf_obj_get(const char *pathname)
> +int bpf_obj_get_flags(const char *pathname, __u32 flags)

same note about bpf_obj_get_opts() instead


>  {
>         union bpf_attr attr;
>         int fd;
>
>         memset(&attr, 0, sizeof(attr));
>         attr.pathname = ptr_to_u64((void *)pathname);
> +       attr.file_flags = flags;
>
>         fd = sys_bpf_fd(BPF_OBJ_GET, &attr, sizeof(attr));
>         return libbpf_err_errno(fd);
>  }
>
> +int bpf_obj_get(const char *pathname)
> +{
> +       return bpf_obj_get_flags(pathname, 0);
> +}
> +
>  int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
>                     unsigned int flags)
>  {
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 20e4c852362d..6d0ceb2e90c4 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -339,6 +339,7 @@ LIBBPF_API int bpf_map_update_batch(int fd, const void *keys, const void *values
>                                     const struct bpf_map_batch_opts *opts);
>
>  LIBBPF_API int bpf_obj_pin(int fd, const char *pathname);
> +LIBBPF_API int bpf_obj_get_flags(const char *pathname, __u32 flags);
>  LIBBPF_API int bpf_obj_get(const char *pathname);
>
>  struct bpf_prog_attach_opts {
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 019278e66836..6c3ace12b27b 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -467,6 +467,7 @@ LIBBPF_1.0.0 {
>                 libbpf_bpf_map_type_str;
>                 libbpf_bpf_prog_type_str;
>                 bpf_map_get_fd_by_id_flags;
> +               bpf_obj_get_flags;
>
>         local: *;
>  };
> --
> 2.25.1
>

On Thu, Jun 2, 2022 at 7:38 AM Roberto Sassu <roberto.sassu@huawei.com> 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@huawei.com/
>
> 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

> From: Andrii Nakryiko [mailto:andrii.nakryiko@gmail.com]
> Sent: Friday, June 3, 2022 11:00 PM
> On Thu, Jun 2, 2022 at 7:38 AM Roberto Sassu <roberto.sassu@huawei.com>
> 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@huawei.com/
> >
> > 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