On Tue, Mar 11, 2025 at 1:59 PM Mykyta Yatsenko
<mykyta.yatsenko5@xxxxxxxxx> wrote:
>
> On 10/03/2025 15:57, 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
> Got it, thanks.
> > 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.
> I agree this does not look right, I think the test itself is not ideal.
> The behavior this test checked for has changed,
> `btf_get_fd_by_id` was returning EINVAL from here:
> ```
> if (CHECK_ATTR(BPF_BTF_GET_FD_BY_ID))
> return -EINVAL;
>
> ```
>
> That no longer fails because I added new field (token_fd) to the attr
> structure.
> Function now fails further down the road.//I'm on the fence whether
> delete this check at all or change to new error code.
Ah, ok, so I did spot a problem then. You need to add validation of
valid flags and reject any unknown flag (test provides "unsupported"
BPF_F_RDONLY). But let me look at the latest version first, before
submitting a new version.
> >> close_prog:
> >> if (fd >= 0)
> >> --
> >> 2.48.1
> >>
>
