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

[Yonghong Song <yonghong.song@linux.dev>]

On 3/7/25 1:29 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>
> ---
>   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;
>   	};
>   
>   	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;
>   
> -	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)) {

If bpf_token_allow_cmd() failed, token is reset to NULL and used in
the above bpf_token_capable(). I think this is not correct, if token
is available from bpf_token_get_from_fd(), here we should use that
token to represent the proper userns encoded in that token.

Something like below?

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c51193ced383..5bb10b531174 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -5121,20 +5121,20 @@ static int bpf_btf_load(const union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_
                 token = bpf_token_get_from_fd(attr->btf_token_fd);
                 if (IS_ERR(token))
                         return PTR_ERR(token);
-               if (!bpf_token_allow_cmd(token, BPF_BTF_LOAD)) {
-                       bpf_token_put(token);
-                       token = NULL;
-               }
+               if (!bpf_token_allow_cmd(token, BPF_BTF_LOAD))
+                       goto out;
         }
  
-       if (!bpf_token_capable(token, CAP_BPF)) {
-               bpf_token_put(token);
-               return -EPERM;
-       }
+       if (!bpf_token_capable(token, CAP_BPF))
+               goto out;
  
         bpf_token_put(token);
  
         return btf_new_fd(attr, uattr, uattr_size);
+
+out:
+       bpf_token_put(token);
+       return -EPERM;
  }

> +		bpf_token_put(token);
>   		return -EPERM;
> +	}
> +
> +	bpf_token_put(token);
>   
>   	return btf_get_fd_by_id(attr->btf_id);
>   }
[...]