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
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) { 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: *; };
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
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) { 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: *; };
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
Add the flags parameter to open_obj_pinned_any() and open_obj_pinned(), so that it is possible to specify the right permissions when obtaining a file descriptor from a pinned object.
By default, the value passed to those functions is zero, so that there is no change in the current behavior.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- tools/bpf/bpftool/common.c | 15 ++++++++------- tools/bpf/bpftool/link.c | 2 +- tools/bpf/bpftool/main.h | 5 +++-- 3 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c index a45b42ee8ab0..88e5e1900270 100644 --- a/tools/bpf/bpftool/common.c +++ b/tools/bpf/bpftool/common.c @@ -118,7 +118,7 @@ int mount_tracefs(const char *target) return err; }
-int open_obj_pinned(const char *path, bool quiet) +int open_obj_pinned(const char *path, bool quiet, __u32 flags) { char *pname; int fd = -1; @@ -130,7 +130,7 @@ int open_obj_pinned(const char *path, bool quiet) goto out_ret; }
- fd = bpf_obj_get(pname); + fd = bpf_obj_get_flags(pname, flags); if (fd < 0) { if (!quiet) p_err("bpf obj get (%s): %s", pname, @@ -146,12 +146,13 @@ int open_obj_pinned(const char *path, bool quiet) return fd; }
-int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type) +int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type, + __u32 flags) { enum bpf_obj_type type; int fd;
- fd = open_obj_pinned(path, false); + fd = open_obj_pinned(path, false, flags); if (fd < 0) return -1;
@@ -400,7 +401,7 @@ static int do_build_table_cb(const char *fpath, const struct stat *sb, if (typeflag != FTW_F) goto out_ret;
- fd = open_obj_pinned(fpath, true); + fd = open_obj_pinned(fpath, true, 0); if (fd < 0) goto out_ret;
@@ -761,7 +762,7 @@ int prog_parse_fds(int *argc, char ***argv, int **fds) path = **argv; NEXT_ARGP();
- (*fds)[0] = open_obj_pinned_any(path, BPF_OBJ_PROG); + (*fds)[0] = open_obj_pinned_any(path, BPF_OBJ_PROG, 0); if ((*fds)[0] < 0) return -1; return 1; @@ -898,7 +899,7 @@ int map_parse_fds(int *argc, char ***argv, int **fds) path = **argv; NEXT_ARGP();
- (*fds)[0] = open_obj_pinned_any(path, BPF_OBJ_MAP); + (*fds)[0] = open_obj_pinned_any(path, BPF_OBJ_MAP, 0); if ((*fds)[0] < 0) return -1; return 1; diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c index 7a20931c3250..04447ad9b3b3 100644 --- a/tools/bpf/bpftool/link.c +++ b/tools/bpf/bpftool/link.c @@ -44,7 +44,7 @@ static int link_parse_fd(int *argc, char ***argv) path = **argv; NEXT_ARGP();
- return open_obj_pinned_any(path, BPF_OBJ_LINK); + return open_obj_pinned_any(path, BPF_OBJ_LINK, 0); }
p_err("expected 'id' or 'pinned', got: '%s'?", **argv); diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index 6c311f47147e..3f6c03afb2f8 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -141,8 +141,9 @@ void get_prog_full_name(const struct bpf_prog_info *prog_info, int prog_fd, int get_fd_type(int fd); const char *get_fd_type_name(enum bpf_obj_type type); char *get_fdinfo(int fd, const char *key); -int open_obj_pinned(const char *path, bool quiet); -int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type); +int open_obj_pinned(const char *path, bool quiet, __u32 flags); +int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type, + __u32 flags); int mount_bpffs_for_pin(const char *name); int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(int *, char ***)); int do_pin_fd(int fd, const char *name);
Add the flags parameter to map_parse_fd(), prog_parse_fd(), link_parse_fd() and btf_parse_fd() at the same time, as those functions are passed to do_pin_any().
Pass zero to those functions, so that the current behavior does not change, and adjust permissions in a later patch.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- tools/bpf/bpftool/btf.c | 6 +++--- tools/bpf/bpftool/cgroup.c | 4 ++-- tools/bpf/bpftool/common.c | 10 +++++----- tools/bpf/bpftool/iter.c | 2 +- tools/bpf/bpftool/link.c | 7 ++++--- tools/bpf/bpftool/main.h | 7 ++++--- tools/bpf/bpftool/map.c | 6 +++--- tools/bpf/bpftool/net.c | 2 +- tools/bpf/bpftool/prog.c | 10 +++++----- 9 files changed, 28 insertions(+), 26 deletions(-)
diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c index 7e6accb9d9f7..98569252ef4a 100644 --- a/tools/bpf/bpftool/btf.c +++ b/tools/bpf/bpftool/btf.c @@ -559,7 +559,7 @@ static int do_dump(int argc, char **argv) return -1; }
- fd = prog_parse_fd(&argc, &argv); + fd = prog_parse_fd(&argc, &argv, 0); if (fd < 0) return -1;
@@ -661,7 +661,7 @@ static int do_dump(int argc, char **argv) return err; }
-static int btf_parse_fd(int *argc, char ***argv) +static int btf_parse_fd(int *argc, char ***argv, __u32 flags) { unsigned int id; char *endptr; @@ -931,7 +931,7 @@ static int do_show(int argc, char **argv) __u32 id = 0;
if (argc == 2) { - fd = btf_parse_fd(&argc, &argv); + fd = btf_parse_fd(&argc, &argv, 0); if (fd < 0) return -1; } diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c index 42421fe47a58..516d410a3218 100644 --- a/tools/bpf/bpftool/cgroup.c +++ b/tools/bpf/bpftool/cgroup.c @@ -425,7 +425,7 @@ static int do_attach(int argc, char **argv)
argc -= 2; argv = &argv[2]; - prog_fd = prog_parse_fd(&argc, &argv); + prog_fd = prog_parse_fd(&argc, &argv, 0); if (prog_fd < 0) goto exit_cgroup;
@@ -483,7 +483,7 @@ static int do_detach(int argc, char **argv)
argc -= 2; argv = &argv[2]; - prog_fd = prog_parse_fd(&argc, &argv); + prog_fd = prog_parse_fd(&argc, &argv, 0); if (prog_fd < 0) goto exit_cgroup;
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c index 88e5e1900270..54246109516f 100644 --- a/tools/bpf/bpftool/common.c +++ b/tools/bpf/bpftool/common.c @@ -223,12 +223,12 @@ int do_pin_fd(int fd, const char *name) return err; }
-int do_pin_any(int argc, char **argv, int (*get_fd)(int *, char ***)) +int do_pin_any(int argc, char **argv, int (*get_fd)(int *, char ***, __u32)) { int err; int fd;
- fd = get_fd(&argc, &argv); + fd = get_fd(&argc, &argv, 0); if (fd < 0) return fd;
@@ -772,7 +772,7 @@ int prog_parse_fds(int *argc, char ***argv, int **fds) return -1; }
-int prog_parse_fd(int *argc, char ***argv) +int prog_parse_fd(int *argc, char ***argv, __u32 flags) { int *fds = NULL; int nb_fds, fd; @@ -909,7 +909,7 @@ int map_parse_fds(int *argc, char ***argv, int **fds) return -1; }
-int map_parse_fd(int *argc, char ***argv) +int map_parse_fd(int *argc, char ***argv, __u32 flags) { int *fds = NULL; int nb_fds, fd; @@ -941,7 +941,7 @@ int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len) int err; int fd;
- fd = map_parse_fd(argc, argv); + fd = map_parse_fd(argc, argv, 0); if (fd < 0) return -1;
diff --git a/tools/bpf/bpftool/iter.c b/tools/bpf/bpftool/iter.c index f88fdc820d23..f7a35947f4f6 100644 --- a/tools/bpf/bpftool/iter.c +++ b/tools/bpf/bpftool/iter.c @@ -34,7 +34,7 @@ static int do_pin(int argc, char **argv) return -1; }
- map_fd = map_parse_fd(&argc, &argv); + map_fd = map_parse_fd(&argc, &argv, 0); if (map_fd < 0) return -1;
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c index 04447ad9b3b3..61bc6f1473ed 100644 --- a/tools/bpf/bpftool/link.c +++ b/tools/bpf/bpftool/link.c @@ -15,7 +15,7 @@
static struct hashmap *link_table;
-static int link_parse_fd(int *argc, char ***argv) +static int link_parse_fd(int *argc, char ***argv, __u32 flags) { int fd;
@@ -44,6 +44,7 @@ static int link_parse_fd(int *argc, char ***argv) path = **argv; NEXT_ARGP();
+ /* WARNING: flags not passed for links (no security hook). */ return open_obj_pinned_any(path, BPF_OBJ_LINK, 0); }
@@ -321,7 +322,7 @@ static int do_show(int argc, char **argv) build_obj_refs_table(&refs_table, BPF_OBJ_LINK);
if (argc == 2) { - fd = link_parse_fd(&argc, &argv); + fd = link_parse_fd(&argc, &argv, 0); if (fd < 0) return fd; return do_show_link(fd); @@ -385,7 +386,7 @@ static int do_detach(int argc, char **argv) return 1; }
- fd = link_parse_fd(&argc, &argv); + fd = link_parse_fd(&argc, &argv, 0); if (fd < 0) return 1;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index 3f6c03afb2f8..f342b2da4d8d 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -145,7 +145,8 @@ int open_obj_pinned(const char *path, bool quiet, __u32 flags); int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type, __u32 flags); int mount_bpffs_for_pin(const char *name); -int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(int *, char ***)); +int do_pin_any(int argc, char **argv, + int (*get_fd_by_id)(int *, char ***, __u32)); int do_pin_fd(int fd, const char *name);
/* commands available in bootstrap mode */ @@ -166,9 +167,9 @@ int do_struct_ops(int argc, char **argv) __weak; int do_iter(int argc, char **argv) __weak;
int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what); -int prog_parse_fd(int *argc, char ***argv); +int prog_parse_fd(int *argc, char ***argv, __u32 flags); int prog_parse_fds(int *argc, char ***argv, int **fds); -int map_parse_fd(int *argc, char ***argv); +int map_parse_fd(int *argc, char ***argv, __u32 flags); int map_parse_fds(int *argc, char ***argv, int **fds); int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len);
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c index 800834be1bcb..d1231dce7183 100644 --- a/tools/bpf/bpftool/map.c +++ b/tools/bpf/bpftool/map.c @@ -381,7 +381,7 @@ static int parse_elem(char **argv, struct bpf_map_info *info, return -1; }
- fd = map_parse_fd(&argc, &argv); + fd = map_parse_fd(&argc, &argv, 0); if (fd < 0) return -1;
@@ -402,7 +402,7 @@ static int parse_elem(char **argv, struct bpf_map_info *info, p_info("Warning: updating program array via MAP_ID, make sure this map is kept open\n" " by some process or pinned otherwise update will be lost");
- fd = prog_parse_fd(&argc, &argv); + fd = prog_parse_fd(&argc, &argv, 0); if (fd < 0) return -1;
@@ -1397,7 +1397,7 @@ static int do_freeze(int argc, char **argv) if (!REQ_ARGS(2)) return -1;
- fd = map_parse_fd(&argc, &argv); + fd = map_parse_fd(&argc, &argv, 0); if (fd < 0) return -1;
diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c index 526a332c48e6..32360e07a6fa 100644 --- a/tools/bpf/bpftool/net.c +++ b/tools/bpf/bpftool/net.c @@ -571,7 +571,7 @@ static int do_attach(int argc, char **argv) } NEXT_ARG();
- progfd = prog_parse_fd(&argc, &argv); + progfd = prog_parse_fd(&argc, &argv, 0); if (progfd < 0) return -EINVAL;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index e71f0b2da50b..05480bf26a00 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -1027,7 +1027,7 @@ static int parse_attach_detach_args(int argc, char **argv, int *progfd, if (!REQ_ARGS(3)) return -EINVAL;
- *progfd = prog_parse_fd(&argc, &argv); + *progfd = prog_parse_fd(&argc, &argv, 0); if (*progfd < 0) return *progfd;
@@ -1046,7 +1046,7 @@ static int parse_attach_detach_args(int argc, char **argv, int *progfd, if (!REQ_ARGS(2)) return -EINVAL;
- *mapfd = map_parse_fd(&argc, &argv); + *mapfd = map_parse_fd(&argc, &argv, 0); if (*mapfd < 0) return *mapfd;
@@ -1270,7 +1270,7 @@ static int do_run(int argc, char **argv) if (!REQ_ARGS(4)) return -1;
- fd = prog_parse_fd(&argc, &argv); + fd = prog_parse_fd(&argc, &argv, 0); if (fd < 0) return -1;
@@ -1542,7 +1542,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) } NEXT_ARG();
- fd = map_parse_fd(&argc, &argv); + fd = map_parse_fd(&argc, &argv, 0); if (fd < 0) goto err_free_reuse_maps;
@@ -2231,7 +2231,7 @@ static int do_profile(int argc, char **argv) return -EINVAL;
/* parse target fd */ - profile_tgt_fd = prog_parse_fd(&argc, &argv); + profile_tgt_fd = prog_parse_fd(&argc, &argv, 0); if (profile_tgt_fd < 0) { p_err("failed to parse fd"); return -1;
Add the flags parameter to map_parse_fds(), and the static function map_fd_by_name() called by it. In the latter function, request the read permission for the map search, and obtain a new file descriptor if the flags variable has a different value.
Also pass the flags to the new functions bpf_map_get_fd_by_id_flags() and the modified function open_obj_pinned_any().
At this point, there is still no change in the current behavior, as the flags argument passed is always zero or the requested permission is a subset (in map_fd_by_name()).
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- tools/bpf/bpftool/common.c | 25 ++++++++++++++++++------- tools/bpf/bpftool/main.h | 2 +- tools/bpf/bpftool/map.c | 4 ++-- 3 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c index 54246109516f..641810b78581 100644 --- a/tools/bpf/bpftool/common.c +++ b/tools/bpf/bpftool/common.c @@ -799,7 +799,7 @@ int prog_parse_fd(int *argc, char ***argv, __u32 flags) return fd; }
-static int map_fd_by_name(char *name, int **fds) +static int map_fd_by_name(char *name, int **fds, __u32 flags) { unsigned int id = 0; int fd, nb_fds = 0; @@ -819,7 +819,7 @@ static int map_fd_by_name(char *name, int **fds) return nb_fds; }
- fd = bpf_map_get_fd_by_id(id); + fd = bpf_map_get_fd_by_id_flags(id, BPF_F_RDONLY); if (fd < 0) { p_err("can't get map by id (%u): %s", id, strerror(errno)); @@ -838,6 +838,17 @@ static int map_fd_by_name(char *name, int **fds) continue; }
+ if (flags != BPF_F_RDONLY) { + close(fd); + + fd = bpf_map_get_fd_by_id_flags(id, flags); + if (fd < 0) { + p_err("can't get map by id (%u): %s", + id, strerror(errno)); + goto err_close_fds; + } + } + if (nb_fds > 0) { tmp = realloc(*fds, (nb_fds + 1) * sizeof(int)); if (!tmp) { @@ -857,7 +868,7 @@ static int map_fd_by_name(char *name, int **fds) return -1; }
-int map_parse_fds(int *argc, char ***argv, int **fds) +int map_parse_fds(int *argc, char ***argv, int **fds, __u32 flags) { if (is_prefix(**argv, "id")) { unsigned int id; @@ -872,7 +883,7 @@ int map_parse_fds(int *argc, char ***argv, int **fds) } NEXT_ARGP();
- (*fds)[0] = bpf_map_get_fd_by_id(id); + (*fds)[0] = bpf_map_get_fd_by_id_flags(id, flags); if ((*fds)[0] < 0) { p_err("get map by id (%u): %s", id, strerror(errno)); return -1; @@ -890,7 +901,7 @@ int map_parse_fds(int *argc, char ***argv, int **fds) } NEXT_ARGP();
- return map_fd_by_name(name, fds); + return map_fd_by_name(name, fds, flags); } else if (is_prefix(**argv, "pinned")) { char *path;
@@ -899,7 +910,7 @@ int map_parse_fds(int *argc, char ***argv, int **fds) path = **argv; NEXT_ARGP();
- (*fds)[0] = open_obj_pinned_any(path, BPF_OBJ_MAP, 0); + (*fds)[0] = open_obj_pinned_any(path, BPF_OBJ_MAP, flags); if ((*fds)[0] < 0) return -1; return 1; @@ -919,7 +930,7 @@ int map_parse_fd(int *argc, char ***argv, __u32 flags) p_err("mem alloc failed"); return -1; } - nb_fds = map_parse_fds(argc, argv, &fds); + nb_fds = map_parse_fds(argc, argv, &fds, flags); if (nb_fds != 1) { if (nb_fds > 1) { p_err("several maps match this handle"); diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index f342b2da4d8d..70b0ad6245b9 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -170,7 +170,7 @@ int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what); int prog_parse_fd(int *argc, char ***argv, __u32 flags); int prog_parse_fds(int *argc, char ***argv, int **fds); int map_parse_fd(int *argc, char ***argv, __u32 flags); -int map_parse_fds(int *argc, char ***argv, int **fds); +int map_parse_fds(int *argc, char ***argv, int **fds, __u32 flags); int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len);
struct bpf_prog_linfo; diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c index d1231dce7183..9a747918882e 100644 --- a/tools/bpf/bpftool/map.c +++ b/tools/bpf/bpftool/map.c @@ -634,7 +634,7 @@ static int do_show_subset(int argc, char **argv) p_err("mem alloc failed"); return -1; } - nb_fds = map_parse_fds(&argc, &argv, &fds); + nb_fds = map_parse_fds(&argc, &argv, &fds, 0); if (nb_fds < 1) goto exit_free;
@@ -910,7 +910,7 @@ static int do_dump(int argc, char **argv) p_err("mem alloc failed"); return -1; } - nb_fds = map_parse_fds(&argc, &argv, &fds); + nb_fds = map_parse_fds(&argc, &argv, &fds, 0); if (nb_fds < 1) goto exit_free;
Add the flags parameter to map_parse_fd_and_info(), as this function is called directly by the functions implementing bpftool subcommands, so that it is possible to specify the right permissions for accessing a map.
Still pass zero as value for flags, and adjust permissions in a later patch.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- tools/bpf/bpftool/btf.c | 2 +- tools/bpf/bpftool/common.c | 5 +++-- tools/bpf/bpftool/main.h | 3 ++- tools/bpf/bpftool/map.c | 12 ++++++------ tools/bpf/bpftool/map_perf_ring.c | 3 ++- 5 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c index 98569252ef4a..69a7695030f9 100644 --- a/tools/bpf/bpftool/btf.c +++ b/tools/bpf/bpftool/btf.c @@ -529,7 +529,7 @@ static int do_dump(int argc, char **argv) return -1; }
- fd = map_parse_fd_and_info(&argc, &argv, &info, &len); + fd = map_parse_fd_and_info(&argc, &argv, &info, &len, 0); if (fd < 0) return -1;
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c index 641810b78581..0816ea2f0be1 100644 --- a/tools/bpf/bpftool/common.c +++ b/tools/bpf/bpftool/common.c @@ -947,12 +947,13 @@ int map_parse_fd(int *argc, char ***argv, __u32 flags) return fd; }
-int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len) +int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len, + __u32 flags) { int err; int fd;
- fd = map_parse_fd(argc, argv, 0); + fd = map_parse_fd(argc, argv, flags); if (fd < 0) return -1;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index 70b0ad6245b9..46c2f24f66fd 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -171,7 +171,8 @@ int prog_parse_fd(int *argc, char ***argv, __u32 flags); int prog_parse_fds(int *argc, char ***argv, int **fds); int map_parse_fd(int *argc, char ***argv, __u32 flags); int map_parse_fds(int *argc, char ***argv, int **fds, __u32 flags); -int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len); +int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len, + __u32 flags);
struct bpf_prog_linfo; #ifdef HAVE_LIBBFD_SUPPORT diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c index 9a747918882e..f253f69879a9 100644 --- a/tools/bpf/bpftool/map.c +++ b/tools/bpf/bpftool/map.c @@ -998,7 +998,7 @@ static int do_update(int argc, char **argv) if (argc < 2) usage();
- fd = map_parse_fd_and_info(&argc, &argv, &info, &len); + fd = map_parse_fd_and_info(&argc, &argv, &info, &len, 0); if (fd < 0) return -1;
@@ -1077,7 +1077,7 @@ static int do_lookup(int argc, char **argv) if (argc < 2) usage();
- fd = map_parse_fd_and_info(&argc, &argv, &info, &len); + fd = map_parse_fd_and_info(&argc, &argv, &info, &len, 0); if (fd < 0) return -1;
@@ -1128,7 +1128,7 @@ static int do_getnext(int argc, char **argv) if (argc < 2) usage();
- fd = map_parse_fd_and_info(&argc, &argv, &info, &len); + fd = map_parse_fd_and_info(&argc, &argv, &info, &len, 0); if (fd < 0) return -1;
@@ -1199,7 +1199,7 @@ static int do_delete(int argc, char **argv) if (argc < 2) usage();
- fd = map_parse_fd_and_info(&argc, &argv, &info, &len); + fd = map_parse_fd_and_info(&argc, &argv, &info, &len, 0); if (fd < 0) return -1;
@@ -1311,7 +1311,7 @@ static int do_create(int argc, char **argv) if (!REQ_ARGS(2)) usage(); inner_map_fd = map_parse_fd_and_info(&argc, &argv, - &info, &len); + &info, &len, 0); if (inner_map_fd < 0) return -1; attr.inner_map_fd = inner_map_fd; @@ -1358,7 +1358,7 @@ static int do_pop_dequeue(int argc, char **argv) if (argc < 2) usage();
- fd = map_parse_fd_and_info(&argc, &argv, &info, &len); + fd = map_parse_fd_and_info(&argc, &argv, &info, &len, 0); if (fd < 0) return -1;
diff --git a/tools/bpf/bpftool/map_perf_ring.c b/tools/bpf/bpftool/map_perf_ring.c index 6b0c410152de..dcfc4724cd8d 100644 --- a/tools/bpf/bpftool/map_perf_ring.c +++ b/tools/bpf/bpftool/map_perf_ring.c @@ -135,7 +135,8 @@ int do_event_pipe(int argc, char **argv) int err, map_fd;
map_info_len = sizeof(map_info); - map_fd = map_parse_fd_and_info(&argc, &argv, &map_info, &map_info_len); + map_fd = map_parse_fd_and_info(&argc, &argv, &map_info, &map_info_len, + 0); if (map_fd < 0) return -1;
Add the flags parameter to struct_ops functions, to distinguish between read-like and write-like operations on struct_ops maps.
Also, always perform the search with read-only permission, and eventually obtain a new file descriptor if the requested permission is different.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- tools/bpf/bpftool/struct_ops.c | 39 +++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c index 2535f079ed67..e8252a76e115 100644 --- a/tools/bpf/bpftool/struct_ops.c +++ b/tools/bpf/bpftool/struct_ops.c @@ -130,7 +130,8 @@ static struct bpf_map_info *map_info_alloc(__u32 *alloc_len) * -1: Error and the caller should abort the iteration. */ static int get_next_struct_ops_map(const char *name, int *res_fd, - struct bpf_map_info *info, __u32 info_len) + struct bpf_map_info *info, __u32 info_len, + __u32 flags) { __u32 id = info->id; int err, fd; @@ -144,7 +145,7 @@ static int get_next_struct_ops_map(const char *name, int *res_fd, return -1; }
- fd = bpf_map_get_fd_by_id(id); + fd = bpf_map_get_fd_by_id_flags(id, BPF_F_RDONLY); if (fd < 0) { if (errno == ENOENT) continue; @@ -162,6 +163,19 @@ static int get_next_struct_ops_map(const char *name, int *res_fd,
if (info->type == BPF_MAP_TYPE_STRUCT_OPS && (!name || !strcmp(name, info->name))) { + if (flags != BPF_F_RDONLY) { + close(fd); + + fd = bpf_map_get_fd_by_id_flags(id, flags); + if (fd < 0) { + if (errno == ENOENT) + continue; + p_err("can't get map by id (%u): %s", + id, strerror(errno)); + return -1; + } + } + *res_fd = fd; return 1; } @@ -186,7 +200,7 @@ typedef int (*work_func)(int fd, const struct bpf_map_info *info, void *data, * Then call "func(fd, info, data, wtr)" on each struct_ops map found. */ static struct res do_search(const char *name, work_func func, void *data, - struct json_writer *wtr) + struct json_writer *wtr, __u32 flags) { struct bpf_map_info *info; struct res res = {}; @@ -201,7 +215,8 @@ static struct res do_search(const char *name, work_func func, void *data,
if (wtr) jsonw_start_array(wtr); - while ((err = get_next_struct_ops_map(name, &fd, info, info_len)) == 1) { + while ((err = get_next_struct_ops_map(name, &fd, info, info_len, + flags)) == 1) { res.nr_maps++; err = func(fd, info, data, wtr); if (err) @@ -235,7 +250,7 @@ static struct res do_search(const char *name, work_func func, void *data, }
static struct res do_one_id(const char *id_str, work_func func, void *data, - struct json_writer *wtr) + struct json_writer *wtr, __u32 flags) { struct bpf_map_info *info; struct res res = {}; @@ -251,7 +266,7 @@ static struct res do_one_id(const char *id_str, work_func func, void *data, return res; }
- fd = bpf_map_get_fd_by_id(id); + fd = bpf_map_get_fd_by_id_flags(id, flags); if (fd < 0) { p_err("can't get map by id (%lu): %s", id, strerror(errno)); res.nr_errs++; @@ -300,16 +315,16 @@ static struct res do_one_id(const char *id_str, work_func func, void *data, static struct res do_work_on_struct_ops(const char *search_type, const char *search_term, work_func func, void *data, - struct json_writer *wtr) + struct json_writer *wtr, __u32 flags) { if (search_type) { if (is_prefix(search_type, "id")) - return do_one_id(search_term, func, data, wtr); + return do_one_id(search_term, func, data, wtr, flags); else if (!is_prefix(search_type, "name")) usage(); }
- return do_search(search_term, func, data, wtr); + return do_search(search_term, func, data, wtr, flags); }
static int __do_show(int fd, const struct bpf_map_info *info, void *data, @@ -344,7 +359,7 @@ static int do_show(int argc, char **argv) }
res = do_work_on_struct_ops(search_type, search_term, __do_show, - NULL, json_wtr); + NULL, json_wtr, 0);
return cmd_retval(&res, !!search_term); } @@ -433,7 +448,7 @@ static int do_dump(int argc, char **argv) d.prog_id_as_func_ptr = true;
res = do_work_on_struct_ops(search_type, search_term, __do_dump, &d, - wtr); + wtr, 0);
if (!json_output) jsonw_destroy(&wtr); @@ -472,7 +487,7 @@ static int do_unregister(int argc, char **argv) search_term = GET_ARG();
res = do_work_on_struct_ops(search_type, search_term, - __do_unregister, NULL, NULL); + __do_unregister, NULL, NULL, 0);
return cmd_retval(&res, true); }
Request a read file descriptor for: - map subcommands: show_subset, show, dump, lookup, getnext and pin; - btf subcommand: dump; - prog subcommand: show (metadata); - struct_ops subcommands: show and dump; - do_build_table_cb(), to show the path of a pinned map.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- tools/bpf/bpftool/btf.c | 5 +++-- tools/bpf/bpftool/common.c | 5 +++-- tools/bpf/bpftool/map.c | 10 +++++----- tools/bpf/bpftool/prog.c | 2 +- tools/bpf/bpftool/struct_ops.c | 4 ++-- 5 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c index 69a7695030f9..a36710903549 100644 --- a/tools/bpf/bpftool/btf.c +++ b/tools/bpf/bpftool/btf.c @@ -529,7 +529,8 @@ static int do_dump(int argc, char **argv) return -1; }
- fd = map_parse_fd_and_info(&argc, &argv, &info, &len, 0); + fd = map_parse_fd_and_info(&argc, &argv, &info, &len, + BPF_F_RDONLY); if (fd < 0) return -1;
@@ -730,7 +731,7 @@ build_btf_type_table(struct hashmap *tab, enum bpf_obj_type type, fd = bpf_prog_get_fd_by_id(id); break; case BPF_OBJ_MAP: - fd = bpf_map_get_fd_by_id(id); + fd = bpf_map_get_fd_by_id_flags(id, BPF_F_RDONLY); break; default: err = -1; diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c index 0816ea2f0be1..d20e1fa8a5fd 100644 --- a/tools/bpf/bpftool/common.c +++ b/tools/bpf/bpftool/common.c @@ -228,7 +228,7 @@ int do_pin_any(int argc, char **argv, int (*get_fd)(int *, char ***, __u32)) int err; int fd;
- fd = get_fd(&argc, &argv, 0); + fd = get_fd(&argc, &argv, BPF_F_RDONLY); if (fd < 0) return fd;
@@ -401,7 +401,8 @@ static int do_build_table_cb(const char *fpath, const struct stat *sb, if (typeflag != FTW_F) goto out_ret;
- fd = open_obj_pinned(fpath, true, 0); + /* WARNING: setting flags to BPF_F_RDONLY has effect only for maps. */ + fd = open_obj_pinned(fpath, true, BPF_F_RDONLY); if (fd < 0) goto out_ret;
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c index f253f69879a9..e4346c834e07 100644 --- a/tools/bpf/bpftool/map.c +++ b/tools/bpf/bpftool/map.c @@ -634,7 +634,7 @@ static int do_show_subset(int argc, char **argv) p_err("mem alloc failed"); return -1; } - nb_fds = map_parse_fds(&argc, &argv, &fds, 0); + nb_fds = map_parse_fds(&argc, &argv, &fds, BPF_F_RDONLY); if (nb_fds < 1) goto exit_free;
@@ -702,7 +702,7 @@ static int do_show(int argc, char **argv) break; }
- fd = bpf_map_get_fd_by_id(id); + fd = bpf_map_get_fd_by_id_flags(id, BPF_F_RDONLY); if (fd < 0) { if (errno == ENOENT) continue; @@ -910,7 +910,7 @@ static int do_dump(int argc, char **argv) p_err("mem alloc failed"); return -1; } - nb_fds = map_parse_fds(&argc, &argv, &fds, 0); + nb_fds = map_parse_fds(&argc, &argv, &fds, BPF_F_RDONLY); if (nb_fds < 1) goto exit_free;
@@ -1077,7 +1077,7 @@ static int do_lookup(int argc, char **argv) if (argc < 2) usage();
- fd = map_parse_fd_and_info(&argc, &argv, &info, &len, 0); + fd = map_parse_fd_and_info(&argc, &argv, &info, &len, BPF_F_RDONLY); if (fd < 0) return -1;
@@ -1128,7 +1128,7 @@ static int do_getnext(int argc, char **argv) if (argc < 2) usage();
- fd = map_parse_fd_and_info(&argc, &argv, &info, &len, 0); + fd = map_parse_fd_and_info(&argc, &argv, &info, &len, BPF_F_RDONLY); if (fd < 0) return -1;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index 05480bf26a00..58d573badcb4 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -251,7 +251,7 @@ static void *find_metadata(int prog_fd, struct bpf_map_info *map_info) goto free_map_ids;
for (i = 0; i < prog_info.nr_map_ids; i++) { - map_fd = bpf_map_get_fd_by_id(map_ids[i]); + map_fd = bpf_map_get_fd_by_id_flags(map_ids[i], BPF_F_RDONLY); if (map_fd < 0) goto free_map_ids;
diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c index e8252a76e115..ced5fe62b1d7 100644 --- a/tools/bpf/bpftool/struct_ops.c +++ b/tools/bpf/bpftool/struct_ops.c @@ -359,7 +359,7 @@ static int do_show(int argc, char **argv) }
res = do_work_on_struct_ops(search_type, search_term, __do_show, - NULL, json_wtr, 0); + NULL, json_wtr, BPF_F_RDONLY);
return cmd_retval(&res, !!search_term); } @@ -448,7 +448,7 @@ static int do_dump(int argc, char **argv) d.prog_id_as_func_ptr = true;
res = do_work_on_struct_ops(search_type, search_term, __do_dump, &d, - wtr, 0); + wtr, BPF_F_RDONLY);
if (!json_output) jsonw_destroy(&wtr);
Add some tests to ensure that read-like operations can be performed on a write-protected map, and that write-like operations fail with a read file descriptor.
Do the tests programmatically, with the new functions bpf_map_get_fd_by_id_flags() and bpf_obj_get_flags(), added to libbpf, and with the bpftool binary.
Also ensure that map search by name works when there is a write-protected map. Before, iteration over existing maps stopped due to not being able to get a file descriptor with full permissions.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- .../bpf/prog_tests/test_map_check_access.c | 264 ++++++++++++++++++ .../selftests/bpf/progs/map_check_access.c | 65 +++++ 2 files changed, 329 insertions(+) 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
diff --git a/tools/testing/selftests/bpf/prog_tests/test_map_check_access.c b/tools/testing/selftests/bpf/prog_tests/test_map_check_access.c new file mode 100644 index 000000000000..20ccadcdf10f --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/test_map_check_access.c @@ -0,0 +1,264 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH + * + * Author: Roberto Sassu roberto.sassu@huawei.com + */ + +#include <test_progs.h> + +#include "map_check_access.skel.h" + +#define PINNED_MAP_PATH "/sys/fs/bpf/test_map_check_access_map" +#define BPFTOOL_PATH "./tools/build/bpftool/bpftool" + +enum check_types { CHECK_NONE, CHECK_PINNED, CHECK_METADATA }; + +static int populate_argv(char *argv[], int max_args, char *cmdline) +{ + char *arg; + int i = 0; + + argv[i++] = BPFTOOL_PATH; + + while ((arg = strsep(&cmdline, " "))) { + if (i == max_args - 1) + break; + + argv[i++] = arg; + } + + argv[i] = NULL; + return i; +} + +static void restore_cmdline(char *argv[], int num_args) +{ + int i; + + for (i = 1; i < num_args - 1; i++) + argv[i][strlen(argv[i])] = ' '; +} + +static int _run_bpftool(char *cmdline, enum check_types check) +{ + char *argv[20]; + char output[1024]; + int ret, fd[2], num_args, child_pid, child_status; + + num_args = populate_argv(argv, ARRAY_SIZE(argv), cmdline); + + ret = pipe(fd); + if (ret < 0) + return ret; + + child_pid = fork(); + if (child_pid == 0) { + close(fd[0]); + close(STDOUT_FILENO); + close(STDERR_FILENO); + + ret = dup2(fd[1], STDOUT_FILENO); + if (ret < 0) { + close(fd[1]); + exit(errno); + } + + execv(BPFTOOL_PATH, argv); + close(fd[1]); + exit(errno); + } else if (child_pid > 0) { + close(fd[1]); + + restore_cmdline(argv, num_args); + + waitpid(child_pid, &child_status, 0); + if (WEXITSTATUS(child_status)) { + close(fd[0]); + return WEXITSTATUS(child_status); + } + + ret = read(fd[0], output, sizeof(output) - 1); + + close(fd[0]); + + if (ret < 0) + return ret; + + output[ret] = '\0'; + ret = 0; + + switch (check) { + case CHECK_PINNED: + if (!strstr(output, PINNED_MAP_PATH)) + ret = -ENOENT; + break; + case CHECK_METADATA: + if (!strstr(output, "test_var")) + ret = -ENOENT; + break; + default: + break; + } + + return ret; + } + + close(fd[0]); + close(fd[1]); + + return -EINVAL; +} + +void test_test_map_check_access(void) +{ + struct map_check_access *skel; + struct bpf_map_info info_m = { 0 }; + struct bpf_map *map; + __u32 len = sizeof(info_m); + char cmdline[1024]; + int ret, zero = 0, fd, duration = 0; + + skel = map_check_access__open_and_load(); + if (CHECK(!skel, "skel", "open_and_load failed\n")) + goto close_prog; + + ret = map_check_access__attach(skel); + if (CHECK(ret < 0, "skel", "attach failed\n")) + goto close_prog; + + map = bpf_object__find_map_by_name(skel->obj, "data_input"); + if (CHECK(!map, "bpf_object__find_map_by_name", "not found\n")) + goto close_prog; + + ret = bpf_obj_get_info_by_fd(bpf_map__fd(map), &info_m, &len); + if (CHECK(ret < 0, "bpf_obj_get_info_by_fd", "error: %d\n", ret)) + goto close_prog; + + fd = bpf_map_get_fd_by_id(info_m.id); + if (CHECK(fd >= 0, "bpf_map_get_fd_by_id", + "should fail (map write-protected)\n")) + goto close_prog; + + fd = bpf_map_get_fd_by_id_flags(info_m.id, 0); + if (CHECK(fd >= 0, "bpf_map_get_fd_by_id_flags", + "should fail (map write-protected)\n")) + goto close_prog; + + fd = bpf_map_get_fd_by_id_flags(info_m.id, BPF_F_RDONLY); + if (CHECK(fd < 0, "bpf_map_get_fd_by_id_flags", "error: %d\n", fd)) + goto close_prog; + + ret = bpf_map_lookup_elem(fd, &zero, &len); + if (CHECK(ret < 0, "bpf_map_lookup_elem", "error: %d\n", ret)) { + close(fd); + goto close_prog; + } + + ret = bpf_map_update_elem(fd, &zero, &len, BPF_ANY); + + close(fd); + + if (CHECK(!ret, "bpf_map_update_elem", + "should fail (read-only permission)\n")) + goto close_prog; + + ret = bpf_map_update_elem(bpf_map__fd(map), &zero, &len, BPF_ANY); + if (CHECK(ret < 0, "bpf_map_update_elem", "error: %d\n", ret)) + goto close_prog; + + ret = bpf_map__pin(map, PINNED_MAP_PATH); + if (CHECK(ret < 0, "bpf_map__pin", "error: %d\n", ret)) + goto close_prog; + + fd = bpf_obj_get_flags(PINNED_MAP_PATH, BPF_F_RDONLY); + if (CHECK(fd < 0, "bpf_obj_get_flags", "error: %d\n", fd)) + goto close_prog; + + close(fd); + + fd = bpf_obj_get_flags(PINNED_MAP_PATH, 0); + if (CHECK(fd >= 0, "bpf_obj_get_flags", + "should fail (read-only permission)\n")) { + close(fd); + goto close_prog; + } + + snprintf(cmdline, sizeof(cmdline), "map list"); + ret = _run_bpftool(cmdline, CHECK_NONE); + if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret)) + goto close_prog; + + snprintf(cmdline, sizeof(cmdline), "map show name data_input"); + ret = _run_bpftool(cmdline, CHECK_NONE); + if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret)) + goto close_prog; + + snprintf(cmdline, sizeof(cmdline), "map -f show pinned %s", + PINNED_MAP_PATH); + ret = _run_bpftool(cmdline, CHECK_PINNED); + if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret)) + goto close_prog; + + unlink(PINNED_MAP_PATH); + + snprintf(cmdline, sizeof(cmdline), "map dump name data_input"); + ret = _run_bpftool(cmdline, CHECK_NONE); + if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret)) + goto close_prog; + + snprintf(cmdline, sizeof(cmdline), + "map lookup name data_input key 0 0 0 0"); + ret = _run_bpftool(cmdline, CHECK_NONE); + if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret)) + goto close_prog; + + snprintf(cmdline, sizeof(cmdline), + "map update name data_input key 0 0 0 0 value 0 0 0 0"); + ret = _run_bpftool(cmdline, CHECK_NONE); + if (CHECK(!ret, "bpftool", + "%s - should fail (read-only permission)\n", cmdline)) + goto close_prog; + + snprintf(cmdline, sizeof(cmdline), + "map update name data_input_w key 0 0 0 0 value 0 0 0 0"); + ret = _run_bpftool(cmdline, CHECK_NONE); + if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret)) + goto close_prog; + + snprintf(cmdline, sizeof(cmdline), "prog show name check_access"); + ret = _run_bpftool(cmdline, CHECK_METADATA); + if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret)) + goto close_prog; + + snprintf(cmdline, sizeof(cmdline), "btf show"); + ret = _run_bpftool(cmdline, CHECK_NONE); + if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret)) + goto close_prog; + + snprintf(cmdline, sizeof(cmdline), "btf dump map name data_input"); + ret = _run_bpftool(cmdline, CHECK_NONE); + if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret)) + goto close_prog; + + snprintf(cmdline, sizeof(cmdline), "map pin name data_input %s", + PINNED_MAP_PATH); + ret = _run_bpftool(cmdline, CHECK_NONE); + if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret)) + goto close_prog; + + snprintf(cmdline, sizeof(cmdline), "struct_ops show name dummy_2"); + ret = _run_bpftool(cmdline, CHECK_NONE); + if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret)) + goto close_prog; + + snprintf(cmdline, sizeof(cmdline), "struct_ops dump name dummy_2"); + ret = _run_bpftool(cmdline, CHECK_NONE); + + CHECK(ret, "_run_bpftool", "%s - error: %d\n", cmdline, ret); + +close_prog: + map_check_access__destroy(skel); + unlink(PINNED_MAP_PATH); +} diff --git a/tools/testing/selftests/bpf/progs/map_check_access.c b/tools/testing/selftests/bpf/progs/map_check_access.c new file mode 100644 index 000000000000..3e75b1114f79 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/map_check_access.c @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Copyright (C) 2021. Huawei Technologies Co., Ltd + * Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH + * + * Author: Roberto Sassu roberto.sassu@huawei.com + */ + +#include "vmlinux.h" +#include <errno.h> +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> + +/* From include/linux/mm.h. */ +#define FMODE_WRITE 0x2 + +const char bpf_metadata_test_var[] SEC(".rodata") = "test_var"; + +struct { + __uint(type, BPF_MAP_TYPE_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u32); +} data_input SEC(".maps"); + +struct { + __uint(type, BPF_MAP_TYPE_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u32); +} data_input_w SEC(".maps"); + +char _license[] SEC("license") = "GPL"; + +SEC("lsm/bpf_map") +int BPF_PROG(check_access, struct bpf_map *map, fmode_t fmode) +{ + if (map != (struct bpf_map *)&data_input) + return 0; + + if (fmode & FMODE_WRITE) + return -EACCES; + + return 0; +} + +SEC("struct_ops/test_1") +int BPF_PROG(test_1, struct bpf_dummy_ops_state *state) +{ + return 0; +} + +SEC("struct_ops/test_2") +int BPF_PROG(test_2, struct bpf_dummy_ops_state *state, int a1, + unsigned short a2, char a3, unsigned long a4) +{ + return 0; +} + +SEC(".struct_ops") +struct bpf_dummy_ops dummy_2 = { + .test_1 = (void *)test_1, + .test_2 = (void *)test_2, +};
On Thu, Jun 2, 2022 at 7:39 AM Roberto Sassu roberto.sassu@huawei.com wrote:
Add some tests to ensure that read-like operations can be performed on a write-protected map, and that write-like operations fail with a read file descriptor.
Do the tests programmatically, with the new functions bpf_map_get_fd_by_id_flags() and bpf_obj_get_flags(), added to libbpf, and with the bpftool binary.
Also ensure that map search by name works when there is a write-protected map. Before, iteration over existing maps stopped due to not being able to get a file descriptor with full permissions.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
.../bpf/prog_tests/test_map_check_access.c | 264 ++++++++++++++++++ .../selftests/bpf/progs/map_check_access.c | 65 +++++ 2 files changed, 329 insertions(+) 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
general convention (not universally followed, unfortunately) is opposite, progs/test_<something>.c and prog_tests/<something>.c. This allows to not have weird test_test_<something> naming pattern
diff --git a/tools/testing/selftests/bpf/prog_tests/test_map_check_access.c b/tools/testing/selftests/bpf/prog_tests/test_map_check_access.c new file mode 100644 index 000000000000..20ccadcdf10f --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/test_map_check_access.c @@ -0,0 +1,264 @@ +// SPDX-License-Identifier: GPL-2.0
+/*
- Copyright (C) 2022 Huawei Technologies Duesseldorf GmbH
- Author: Roberto Sassu roberto.sassu@huawei.com
- */
+#include <test_progs.h>
+#include "map_check_access.skel.h"
+#define PINNED_MAP_PATH "/sys/fs/bpf/test_map_check_access_map" +#define BPFTOOL_PATH "./tools/build/bpftool/bpftool"
+enum check_types { CHECK_NONE, CHECK_PINNED, CHECK_METADATA };
+static int populate_argv(char *argv[], int max_args, char *cmdline) +{
char *arg;int i = 0;argv[i++] = BPFTOOL_PATH;while ((arg = strsep(&cmdline, " "))) {if (i == max_args - 1)break;argv[i++] = arg;}argv[i] = NULL;return i;+}
+static void restore_cmdline(char *argv[], int num_args) +{
int i;for (i = 1; i < num_args - 1; i++)argv[i][strlen(argv[i])] = ' ';+}
I'm missing the point of this cmdline restoration?..
+static int _run_bpftool(char *cmdline, enum check_types check) +{
char *argv[20];char output[1024];int ret, fd[2], num_args, child_pid, child_status;num_args = populate_argv(argv, ARRAY_SIZE(argv), cmdline);ret = pipe(fd);if (ret < 0)return ret;
and popen() doesn't work instead of all this forking/execve sequence?
child_pid = fork();if (child_pid == 0) {close(fd[0]);close(STDOUT_FILENO);close(STDERR_FILENO);ret = dup2(fd[1], STDOUT_FILENO);if (ret < 0) {close(fd[1]);exit(errno);}execv(BPFTOOL_PATH, argv);close(fd[1]);exit(errno);} else if (child_pid > 0) {close(fd[1]);restore_cmdline(argv, num_args);waitpid(child_pid, &child_status, 0);if (WEXITSTATUS(child_status)) {close(fd[0]);return WEXITSTATUS(child_status);}ret = read(fd[0], output, sizeof(output) - 1);close(fd[0]);if (ret < 0)return ret;output[ret] = '\0';ret = 0;switch (check) {case CHECK_PINNED:if (!strstr(output, PINNED_MAP_PATH))ret = -ENOENT;break;case CHECK_METADATA:if (!strstr(output, "test_var"))ret = -ENOENT;break;default:break;}return ret;}close(fd[0]);close(fd[1]);return -EINVAL;+}
+void test_test_map_check_access(void) +{
struct map_check_access *skel;struct bpf_map_info info_m = { 0 };struct bpf_map *map;__u32 len = sizeof(info_m);char cmdline[1024];int ret, zero = 0, fd, duration = 0;skel = map_check_access__open_and_load();if (CHECK(!skel, "skel", "open_and_load failed\n"))goto close_prog;ret = map_check_access__attach(skel);if (CHECK(ret < 0, "skel", "attach failed\n"))goto close_prog;
please don't use CHECK(), use ASSERT_xxx() macros instead
map = bpf_object__find_map_by_name(skel->obj, "data_input");if (CHECK(!map, "bpf_object__find_map_by_name", "not found\n"))goto close_prog;ret = bpf_obj_get_info_by_fd(bpf_map__fd(map), &info_m, &len);if (CHECK(ret < 0, "bpf_obj_get_info_by_fd", "error: %d\n", ret))goto close_prog;fd = bpf_map_get_fd_by_id(info_m.id);if (CHECK(fd >= 0, "bpf_map_get_fd_by_id","should fail (map write-protected)\n"))goto close_prog;
[...]
snprintf(cmdline, sizeof(cmdline), "btf dump map name data_input");ret = _run_bpftool(cmdline, CHECK_NONE);if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret))goto close_prog;snprintf(cmdline, sizeof(cmdline), "map pin name data_input %s",PINNED_MAP_PATH);ret = _run_bpftool(cmdline, CHECK_NONE);if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret))goto close_prog;snprintf(cmdline, sizeof(cmdline), "struct_ops show name dummy_2");ret = _run_bpftool(cmdline, CHECK_NONE);
if you don't have to restore anything, you don't need snprintf()'ing, just pass arguments as a string literal directly
if (CHECK(ret, "bpftool", "%s - error: %d\n", cmdline, ret))goto close_prog;snprintf(cmdline, sizeof(cmdline), "struct_ops dump name dummy_2");ret = _run_bpftool(cmdline, CHECK_NONE);CHECK(ret, "_run_bpftool", "%s - error: %d\n", cmdline, ret);+close_prog:
map_check_access__destroy(skel);unlink(PINNED_MAP_PATH);+}
[...]
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
linux-kselftest-mirror@lists.linaro.org
-
Andrii Nakryiko -
Roberto Sassu