On Fri, Dec 01, 2023 at 09:47:29AM +0000, Jie Jiang wrote:
> Parse uid and gid in bpf_parse_param() so that they can be passed in as
> the `data` parameter when mount() bpffs. This will be useful when we
> want to control which user/group has the control to the mounted bpffs,
> otherwise a separate chown() call will be needed.
>
> Signed-off-by: Jie Jiang <jiejiang@xxxxxxxxxxxx>
> ---
Sorry, I was asked to take a quick look at this. The patchset looks fine
overall but it will interact with Andrii's patchset which makes bpffs
mountable inside a user namespace (with caveats).
At that point you need additional validation in bpf_parse_param(). The
simplest thing would probably to just put this into this series or into
@Andrii's series. It's basically a copy-pasta from what I did for tmpfs
(see below).
I plan to move this validation into the VFS so that {g,u}id mount
options are validated consistenly for any such filesystem. There is just
some unpleasantness that I have to figure out first.
@Andrii, with the {g,u}id mount option it means that userns root can
fsconfig(..., FSCONFIG_SET_STRING, "uid", "1000", ...)
fsconfig(..., FSCONFIG_SET_STRING, "gid", "1000", ...)
fsconfig(..., FSCONFIG_CMD_CREATE, ...)
If you delegate CAP_BPF in that userns to uid 1000 then an unpriv user
in that userns can create bpf tokens. Currently this would require
userns root to give both CAP_DAC_READ_SEARCH and CAP_BPF to such an
unprivileged user.
Depending on whether or not that's intended you might want to add an
additional check into bpf_token_create() to verify that the caller's
{g,u}id resolves to 0:
if (from_kuid(current_user_ns(), current_fsuid()) != 0)
return -EINVAL;
That's basically saying you're restricting this to userns root. Idk,
that's up to you. (Note that you currently enforce current_user_ns() ==
token->user_ns == s_user_ns which is why it doesn't matter what userns
you pass here. You'd just error out later.)
> kernel/bpf/inode.c | 33 +++++++++++++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
> index 1aafb2ff2e953..826fe48745ee2 100644
> --- a/kernel/bpf/inode.c
> +++ b/kernel/bpf/inode.c
> @@ -599,8 +599,15 @@ EXPORT_SYMBOL(bpf_prog_get_type_path);
> */
> static int bpf_show_options(struct seq_file *m, struct dentry *root)
> {
> - umode_t mode = d_inode(root)->i_mode & S_IALLUGO & ~S_ISVTX;
> -
> + struct inode *inode = d_inode(root);
> + umode_t mode = inode->i_mode & S_IALLUGO & ~S_ISVTX;
> +
> + if (!uid_eq(inode->i_uid, GLOBAL_ROOT_UID))
> + seq_printf(m, ",uid=%u",
> + from_kuid_munged(&init_user_ns, inode->i_uid));
> + if (!gid_eq(inode->i_gid, GLOBAL_ROOT_GID))
> + seq_printf(m, ",gid=%u",
> + from_kgid_munged(&init_user_ns, inode->i_gid));
> if (mode != S_IRWXUGO)
> seq_printf(m, ",mode=%o", mode);
> return 0;
> @@ -625,15 +632,21 @@ static const struct super_operations bpf_super_ops = {
> };
>
> enum {
> + OPT_UID,
> + OPT_GID,
> OPT_MODE,
> };
>
> static const struct fs_parameter_spec bpf_fs_parameters[] = {
> + fsparam_u32 ("gid", OPT_GID),
> fsparam_u32oct ("mode", OPT_MODE),
> + fsparam_u32 ("uid", OPT_UID),
> {}
> };
>
> struct bpf_mount_opts {
> + kuid_t uid;
> + kgid_t gid;
> umode_t mode;
> };
>
> @@ -641,6 +654,8 @@ static int bpf_parse_param(struct fs_context *fc, struct fs_parameter *param)
> {
> struct bpf_mount_opts *opts = fc->fs_private;
> struct fs_parse_result result;
> + kuid_t uid;
> + kgid_t gid;
> int opt;
>
> opt = fs_parse(fc, bpf_fs_parameters, param, &result);
> @@ -662,6 +677,18 @@ static int bpf_parse_param(struct fs_context *fc, struct fs_parameter *param)
> }
>
> switch (opt) {
> + case OPT_UID:
> + uid = make_kuid(current_user_ns(), result.uint_32);
> + if (!uid_valid(uid))
> + return invalf(fc, "Unknown uid");
/*
* The requested uid must be representable in the
* filesystem's idmapping.
*/
if (!kuid_has_mapping(fc->user_ns, kuid))
goto bad_value;
> + opts->uid = uid;
> + break;
> + case OPT_GID:
> + gid = make_kgid(current_user_ns(), result.uint_32);
> + if (!gid_valid(gid))
> + return invalf(fc, "Unknown gid");
/*
* The requested gid must be representable in the
* filesystem's idmapping.
*/
if (!kgid_has_mapping(fc->user_ns, kgid))
goto bad_value;
> + opts->gid = gid;
> + break;
> case OPT_MODE:
> opts->mode = result.uint_32 & S_IALLUGO;
> break;
> @@ -750,6 +777,8 @@ static int bpf_fill_super(struct super_block *sb, struct fs_context *fc)
> sb->s_op = &bpf_super_ops;
>
> inode = sb->s_root->d_inode;
> + inode->i_uid = opts->uid;
> + inode->i_gid = opts->gid;
> inode->i_op = &bpf_dir_iops;
> inode->i_mode &= ~S_IALLUGO;
> populate_bpffs(sb->s_root);
> --
> 2.43.0.rc2.451.g8631bc7472-goog
>
