diff mbox series

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

Message ID 20250310001319.41393-2-mykyta.yatsenko5@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Support freplace prog from user namespace | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 194 this patch: 194
netdev/build_tools success Errors and warnings before: 26 (+0) this patch: 26 (+0)
netdev/cc_maintainers warning 10 maintainers not CCed: jolsa@kernel.org kpsingh@kernel.org linux-kselftest@vger.kernel.org sdf@fomichev.me john.fastabend@gmail.com haoluo@google.com mykolal@fb.com martin.lau@linux.dev shuah@kernel.org song@kernel.org
netdev/build_clang success Errors and warnings before: 225 this patch: 225
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 7021 this patch: 7021
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 59 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-11 success Logs for aarch64-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-12 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-gcc / veristat-meta / x86_64-gcc veristat_meta
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-18 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for s390x-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-21 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-50 success Logs for x86_64-llvm-18 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-51 success Logs for x86_64-llvm-18 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-49 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-47 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-48 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-7 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-10 success Logs for x86_64-gcc / build-release

Commit Message

Mykyta Yatsenko March 10, 2025, 12:13 a.m. UTC 
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(-)

Comments

Yonghong Song March 10, 2025, 3:43 p.m. UTC | #1 
On 3/9/25 5:13 PM, Mykyta Yatsenko 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>

Acked-by: Yonghong Song<yonghong.song@linux.dev>
Andrii Nakryiko March 10, 2025, 3:57 p.m. UTC | #2 
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

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
>
Yonghong Song March 10, 2025, 6:29 p.m. UTC | #3 
On 3/10/25 8:57 AM, 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

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
>>
Mykyta Yatsenko March 11, 2025, 8:59 p.m. UTC | #4 
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
>>
Andrii Nakryiko March 12, 2025, 6:50 p.m. UTC | #5 
On Tue, Mar 11, 2025 at 1:59 PM Mykyta Yatsenko
<mykyta.yatsenko5@gmail.com> wrote:
>
> 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.

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
> >>
>
diff mbox series

Patch

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;
+	}
+
+	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");
 
 close_prog:
 	if (fd >= 0)