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.
close_prog:
if (fd >= 0)
--
2.48.1
