Re: [PATCH bpf-next v4 1/4] bpf: BPF token support for BPF_BTF_GET_FD_BY_ID

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 10/03/2025 15:57, Andrii Nakryiko wrote:
> On Sun, Mar 9, 2025 at 5:13 PM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> wrote:
>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>
>> 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@meta.com>
>> ---
>>   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.
>>   close_prog:
>>          if (fd >= 0)
>> --
>> 2.48.1
>>