On 3/10/25 8:57 AM, Andrii Nakryiko wrote:
On Sun, Mar 9, 2025 at 5:13 PM Mykyta Yatsenko <mykyta.yatsenko5@xxxxxxxxx> wrote:From: Mykyta Yatsenko <yatsenko@xxxxxxxx> Currently BPF_BTF_GET_FD_BY_ID requires CAP_SYS_ADMIN, which does not allow running it from user namespace. This creates a problem when freplace program running from user namespace needs to query target program BTF. This patch relaxes capable check from CAP_SYS_ADMIN to CAP_BPF and adds support for BPF token that can be passed in attributes to syscall. Signed-off-by: Mykyta Yatsenko <yatsenko@xxxxxxxx> --- include/uapi/linux/bpf.h | 1 + kernel/bpf/syscall.c | 21 ++++++++++++++++--- tools/include/uapi/linux/bpf.h | 1 + .../bpf/prog_tests/libbpf_get_fd_by_id_opts.c | 3 +-- 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index bb37897c0393..73c23daacabf 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -1652,6 +1652,7 @@ union bpf_attr { }; __u32 next_id; __u32 open_flags; + __s32 token_fd; }; struct { /* anonymous struct used by BPF_OBJ_GET_INFO_BY_FD */ diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 57a438706215..eb3a31aefa70 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -5137,17 +5137,32 @@ static int bpf_btf_load(const union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_ return btf_new_fd(attr, uattr, uattr_size); } -#define BPF_BTF_GET_FD_BY_ID_LAST_FIELD btf_id +#define BPF_BTF_GET_FD_BY_ID_LAST_FIELD token_fd static int bpf_btf_get_fd_by_id(const union bpf_attr *attr) { + struct bpf_token *token = NULL; + if (CHECK_ATTR(BPF_BTF_GET_FD_BY_ID)) return -EINVAL; - if (!capable(CAP_SYS_ADMIN)) - return -EPERM; + if (attr->open_flags & BPF_F_TOKEN_FD) { + token = bpf_token_get_from_fd(attr->token_fd); + if (IS_ERR(token)) + return PTR_ERR(token); + if (!bpf_token_allow_cmd(token, BPF_BTF_GET_FD_BY_ID)) + goto out;Look at map_create() and its handling of BPF token. If bpf_token_allow_cmd() returns false, we still perform bpf_token_capable(token, <cap>) check (where token will be NULL, so it's effectively just capable() check). While here you will just return -EPERM *even if the process actually has real CAP_SYS_ADMIN* capability. Instead, do: bpf_token_put(token); token = NULL; and carry on the rest of the logic
It looks like my earlier suggestion, which leads to this version, is incorrect. Sorry about this. I need to dig out a little more for func cap_capable_helper(). But it is apparent that task cred is used for capability checking.
pw-bot: cr+ } + + if (!bpf_token_capable(token, CAP_SYS_ADMIN)) + goto out; + + bpf_token_put(token); return btf_get_fd_by_id(attr->btf_id); +out: + bpf_token_put(token); + return -EPERM; } static int bpf_task_fd_query_copy(const union bpf_attr *attr, diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index bb37897c0393..73c23daacabf 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -1652,6 +1652,7 @@ union bpf_attr { }; __u32 next_id; __u32 open_flags; + __s32 token_fd; }; struct { /* anonymous struct used by BPF_OBJ_GET_INFO_BY_FD */ diff --git a/tools/testing/selftests/bpf/prog_tests/libbpf_get_fd_by_id_opts.c b/tools/testing/selftests/bpf/prog_tests/libbpf_get_fd_by_id_opts.c index a3f238f51d05..976ff38a6d43 100644 --- a/tools/testing/selftests/bpf/prog_tests/libbpf_get_fd_by_id_opts.c +++ b/tools/testing/selftests/bpf/prog_tests/libbpf_get_fd_by_id_opts.c @@ -75,9 +75,8 @@ void test_libbpf_get_fd_by_id_opts(void) if (!ASSERT_EQ(ret, -EINVAL, "bpf_link_get_fd_by_id_opts")) goto close_prog; - /* BTF get fd with opts set should not work (no kernel support). */ ret = bpf_btf_get_fd_by_id_opts(0, &fd_opts_rdonly); - ASSERT_EQ(ret, -EINVAL, "bpf_btf_get_fd_by_id_opts"); + ASSERT_EQ(ret, -ENOENT, "bpf_btf_get_fd_by_id_opts");Why would your patch change this behavior? and if it does, should it? This looks fishy.close_prog: if (fd >= 0) -- 2.48.1
