| Message ID | 20231201094729.1312133-1-jiejiang@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: Support uid and gid when mounting bpffs | expand |
Acked-by: Mike Frysinger <vapier@chromium.org>
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@chromium.org> > --- 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 >
On Tue, Dec 5, 2023 at 8:31 AM Christian Brauner <brauner@kernel.org> wrote: > > 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@chromium.org> > > --- > > 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. This is probably fine. Basically the only difference is that BPF FS can be instantiated inside an unpriv namespace, instead of in a privileged parent namespace, right? But delegate_xxx options are still guarded by the explicit capable(CAP_SYS_ADMIN) check, so that unprivileged user won't be able to grant themselves BPF token-enabling capabilities without a privileged parent doing it on their behalf. Is my understanding correct or am I missing some nuance here? > > 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 [...]
On Wed, Dec 6, 2023 at 3:28 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Dec 5, 2023 at 8:31 AM Christian Brauner <brauner@kernel.org> wrote: > > > > On Fri, Dec 01, 2023 at 09:47:29AM +0000, Jie Jiang wrote: >> ... > > 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. > > Thank you very much for the suggestions and discussions. I uploaded the v2 version of this patch to include the checks as you suggested. > > @Andrii, with the {g,u}id mount option it means that userns root can > > ... > [...]
On Tue, Dec 05, 2023 at 10:28:39AM -0800, Andrii Nakryiko wrote: > On Tue, Dec 5, 2023 at 8:31 AM Christian Brauner <brauner@kernel.org> wrote: > > > > 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@chromium.org> > > > --- > > > > 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. > > This is probably fine. Basically the only difference is that BPF FS > can be instantiated inside an unpriv namespace, instead of in a > privileged parent namespace, right? Hm, I think this is slightly misphrased but I guess I get what you mean. Basically, userns root can change what {g,u}id bpffs will use to instantiate inodes once init_user_ns root creates the superblock. IOW, the {g,u}id mount option isn't guarded and can thus be changed by userns root. > > But delegate_xxx options are still guarded by the explicit > capable(CAP_SYS_ADMIN) check, so that unprivileged user won't be able > to grant themselves BPF token-enabling capabilities without a > privileged parent doing it on their behalf. > > Is my understanding correct or am I missing some nuance here? No, that's correct.
On Wed, Dec 6, 2023 at 2:58 AM Christian Brauner <brauner@kernel.org> wrote: > > On Tue, Dec 05, 2023 at 10:28:39AM -0800, Andrii Nakryiko wrote: > > On Tue, Dec 5, 2023 at 8:31 AM Christian Brauner <brauner@kernel.org> wrote: > > > > > > 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@chromium.org> > > > > --- > > > > > > 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. > > > > This is probably fine. Basically the only difference is that BPF FS > > can be instantiated inside an unpriv namespace, instead of in a > > privileged parent namespace, right? > > Hm, I think this is slightly misphrased but I guess I get what you mean. > > Basically, userns root can change what {g,u}id bpffs will use to > instantiate inodes once init_user_ns root creates the superblock. IOW, > the {g,u}id mount option isn't guarded and can thus be changed by userns > root. > > > > > But delegate_xxx options are still guarded by the explicit > > capable(CAP_SYS_ADMIN) check, so that unprivileged user won't be able > > to grant themselves BPF token-enabling capabilities without a > > privileged parent doing it on their behalf. > > > > Is my understanding correct or am I missing some nuance here? > > No, that's correct. Ok, thanks, then it seems like it's all good w.r.t. interaction with delegate_xxx options and BPF token creation.
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"); + 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"); + 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);
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@chromium.org> --- kernel/bpf/inode.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)