On Tue, 5 Sept 2023 at 20:05, Jiao Zhou <jiaozhou@xxxxxxxxxx> wrote: > > Hi Christian, > > Thanks for your reply, will adding a guid and uid check like the below be sufficient? I saw this is how most file systems check their gid and uid. I am not quite sure about how to send fd back to init_user_ns. Can you please clarify a little bit? Thank you so much. ... > Please don't top post and please only send plaintext email to the mailing lists. Please consult Documentation/process/submitting-patches.rst in the Linux source tree if you are unfamiliar with the upstream contribution process (and since you appear to be at Google, you might want to consult go/kernel-upstream as well) Thanks, Ard. > > > On Mon, Sep 4, 2023 at 1:17 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: >> >> On Thu, Aug 31, 2023 at 03:31:07PM +0000, Jiao Zhou wrote: >> > Add uid and gid in efivarfs's mount option, so that >> > we can mount the file system with ownership. This approach >> > is used by a number of other filesystems that don't have >> > native support for ownership. >> > >> > TEST=FEATURES=test emerge-reven chromeos-kernel-5_15 >> > >> > Signed-off-by: Jiao Zhou <jiaozhou@xxxxxxxxxx> >> > Reported-by: kernel test robot <oliver.sang@xxxxxxxxx> >> > Closes: https://lore.kernel.org/oe-lkp/202308291443.ea96ac66-oliver.sang@xxxxxxxxx >> > --- >> > fs/efivarfs/inode.c | 4 +++ >> > fs/efivarfs/internal.h | 9 ++++++ >> > fs/efivarfs/super.c | 65 ++++++++++++++++++++++++++++++++++++++++++ >> > 3 files changed, 78 insertions(+) >> > >> > diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c >> > index 939e5e242b98..de57fb6c28e1 100644 >> > --- a/fs/efivarfs/inode.c >> > +++ b/fs/efivarfs/inode.c >> > @@ -20,9 +20,13 @@ struct inode *efivarfs_get_inode(struct super_block *sb, >> > const struct inode *dir, int mode, >> > dev_t dev, bool is_removable) >> > { >> > + struct efivarfs_fs_info *fsi = sb->s_fs_info; >> > struct inode *inode = new_inode(sb); >> > + struct efivarfs_mount_opts *opts = &fsi->mount_opts; >> > >> > if (inode) { >> > + inode->i_uid = opts->uid; >> > + inode->i_gid = opts->gid; >> > inode->i_ino = get_next_ino(); >> > inode->i_mode = mode; >> > inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode); >> > diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h >> > index 30ae44cb7453..57deaf56d8e2 100644 >> > --- a/fs/efivarfs/internal.h >> > +++ b/fs/efivarfs/internal.h >> > @@ -8,6 +8,15 @@ >> > >> > #include <linux/list.h> >> > >> > +struct efivarfs_mount_opts { >> > + kuid_t uid; >> > + kgid_t gid; >> > +}; >> > + >> > +struct efivarfs_fs_info { >> > + struct efivarfs_mount_opts mount_opts; >> > +}; >> > + >> > extern const struct file_operations efivarfs_file_operations; >> > extern const struct inode_operations efivarfs_dir_inode_operations; >> > extern bool efivarfs_valid_name(const char *str, int len); >> > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c >> > index 15880a68faad..d67b0d157ff5 100644 >> > --- a/fs/efivarfs/super.c >> > +++ b/fs/efivarfs/super.c >> > @@ -8,6 +8,7 @@ >> > #include <linux/efi.h> >> > #include <linux/fs.h> >> > #include <linux/fs_context.h> >> > +#include <linux/fs_parser.h> >> > #include <linux/module.h> >> > #include <linux/pagemap.h> >> > #include <linux/ucs2_string.h> >> > @@ -23,10 +24,27 @@ static void efivarfs_evict_inode(struct inode *inode) >> > clear_inode(inode); >> > } >> > >> > +static int efivarfs_show_options(struct seq_file *m, struct dentry *root) >> > +{ >> > + struct super_block *sb = root->d_sb; >> > + struct efivarfs_fs_info *sbi = sb->s_fs_info; >> > + struct efivarfs_mount_opts *opts = &sbi->mount_opts; >> > + >> > + /* Show partition info */ >> > + if (!uid_eq(opts->uid, GLOBAL_ROOT_UID)) >> > + seq_printf(m, ",uid=%u", >> > + from_kuid_munged(&init_user_ns, opts->uid)); >> > + if (!gid_eq(opts->gid, GLOBAL_ROOT_GID)) >> > + seq_printf(m, ",gid=%u", >> > + from_kgid_munged(&init_user_ns, opts->gid)); >> > + return 0; >> > +} >> > + >> > static const struct super_operations efivarfs_ops = { >> > .statfs = simple_statfs, >> > .drop_inode = generic_delete_inode, >> > .evict_inode = efivarfs_evict_inode, >> > + .show_options = efivarfs_show_options, >> > }; >> > >> > /* >> > @@ -190,6 +208,41 @@ static int efivarfs_destroy(struct efivar_entry *entry, void *data) >> > return 0; >> > } >> > >> > +enum { >> > + Opt_uid, Opt_gid, >> > +}; >> > + >> > +static const struct fs_parameter_spec efivarfs_parameters[] = { >> > + fsparam_u32("uid", Opt_uid), >> > + fsparam_u32("gid", Opt_gid), >> > + {}, >> > +}; >> > + >> > +static int efivarfs_parse_param(struct fs_context *fc, struct fs_parameter *param) >> > +{ >> > + struct efivarfs_fs_info *sbi = fc->s_fs_info; >> > + struct efivarfs_mount_opts *opts = &sbi->mount_opts; >> > + struct fs_parse_result result; >> > + int opt; >> > + >> > + opt = fs_parse(fc, efivarfs_parameters, param, &result); >> > + if (opt < 0) >> > + return opt; >> > + >> > + switch (opt) { >> > + case Opt_uid: >> > + opts->uid = make_kuid(current_user_ns(), result.uint_32); >> > + break; >> > + case Opt_gid: >> > + opts->gid = make_kgid(current_user_ns(), result.uint_32); >> > + break; >> >> This will allow the following: >> >> # initial user namespace >> fd_fs = fsopen("efivarfs") >> >> # switch to some unprivileged userns >> fsconfig(fd_fs, FSCONFIG_SET_STRING, "uid", "1000") >> ==> This now resolves within the caller's user namespace which might >> have an idmapping where 1000 cannot be resolved causing sb->{g,u}id >> to be set to INVALID_{G,U}ID. >> >> In fact this is also possible in your patch right now without the >> namespace switching. The caller could just pass -1 and that would >> cause inodes with INVALID_{G,U}ID to be created. >> So you want a check for {g,u}id_valid(). >> >> # send fd back to init_user_ns >> fsconfig(fd_fs, FSCONFIG_CMD_CREATE) >> fd_mnt = fsmount(fd_fs, ...) >> move_mount(fd_fs, "", -EBADF, "/somehwere", ...)
