linux-kernel - Re: [PATCH] kernel: Add Mount Option For Efivarfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20230904-erben-coachen-7ca9a30cdc05@brauner>
Date:   Mon, 4 Sep 2023 14:17:08 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Jiao Zhou <jiaozhou@...gle.com>
Cc:     Linux FS Development <linux-fsdevel@...r.kernel.org>,
        Ard Biesheuvel <ardb@...nel.org>, Jeremy Kerr <jk@...abs.org>,
        linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH] kernel: Add Mount Option For Efivarfs

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@...gle.com>
> Reported-by: kernel test robot <oliver.sang@...el.com>
> Closes: https://lore.kernel.org/oe-lkp/202308291443.ea96ac66-oliver.sang@intel.com
> ---
>  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", ...)