On Wed, Mar 12, 2025 at 11:53 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Mar 11, 2025 at 2:54 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 | 20 +++++++++++++++++--
> > tools/include/uapi/linux/bpf.h | 1 +
> > .../bpf/prog_tests/libbpf_get_fd_by_id_opts.c | 3 +--
> > 4 files changed, 21 insertions(+), 4 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;
So I almost applied it (I was going to add that flags check myself),
but then I realized that attrs->token_fd looks a bit too good to be
true... It's an ugly "historical" bpf_attr structure, but we have
these weird mixes of named and anon sub-structs, so some fields are
way too easily accessed even when dealing with a completely unrelated
command.
That is just to say that token_fd here is way too error-prone and
generically named, unfortunately. All other token-enabled commands
have more specific name (btf_token_fd, prog_token_fd, map_token_fd),
so I think we should continue the pattern here.
Let's call it `fd_by_id_token_fd` (unless someone has a better name)?
Let's keep a clean "token_fd" on libbpf API side (in opts), like we
did for other APIs.
> > };
> >
> > 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..188f7296cf9f 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -5137,15 +5137,31 @@ 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;
>
> as I mentioned in another email, we used to implicitly reject any
> open_flags set because of BPF_BTF_GET_FD_BY_ID_LAST_FIELD btf_id, but
> now that we do support some flags in open_flags, we need to validate
> that there are no other flags. So you need something like:
>
> if (attr->open_flags & ~BPF_F_TOKEN_FD)
> return -EINVAL;
>
>
> pw-bot: cr
>
> >
> > - if (!capable(CAP_SYS_ADMIN))
> > + 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)) {
> > + bpf_token_put(token);
> > + token = NULL;
> > + }
> > + }
> > +
> > + if (!bpf_token_capable(token, CAP_SYS_ADMIN)) {
> > + bpf_token_put(token);
> > return -EPERM;
> > + }
> > +
> > + bpf_token_put(token);
> >
> > return btf_get_fd_by_id(attr->btf_id);
> > }
> > 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");
> >
> > close_prog:
> > if (fd >= 0)
> > --
> > 2.48.1
> >
