| 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
| ||
|
Message-ID: <CAMj1kXEvJPhO6dJGWx8ezX3D6QWwdaLjqw2bFqZCQcQda_BHmw@mail.gmail.com>
Date: Thu, 31 Aug 2023 18:22:20 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Jiao Zhou <jiaozhou@...gle.com>
Cc: Linux FS Development <linux-fsdevel@...r.kernel.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 v2] efivarfs: Add Mount Option For Efivarfs
Hi again,
This is v3 whereas the subject now has v2 - please remain consistent.
On Thu, 31 Aug 2023 at 18:15, Jiao Zhou <jiaozhou@...gle.com> wrote:
>
> We want to support fwupd for updating system firmware on Reven.
> Capsule updates need to create UEFI variables. Our current approach
> to UEFI variables of just allowing access to a static list of them
> at boot time won't work here.
>
> I think we could add mount options to efivarfs to set the uid/gid.
> We'd then mount the file system with fwupd's uid/gid.
> This approach is used by a number of other filesystems that don't
> have native support for ownership, so it should be upstreamable.
>
> 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
I mentioned that you should drop these two lines, and instead, include
the acked-by line from Matthew Garrett's reply to v1. You also failed
to cc Matthew - it is common courtesy to cc people that have taken the
trouble of responding to your previous revisions on subsequent ones.
> ---
> Changelog since v2:
> - Fix a NULL pointer dereference and add a a trailing empty entry to sturct.
>
These are the changes wrt v1 no? Please try to be accurate - these
things may seem unimportant but all these emails are logged and
archived, and someone may need to try and make sense of them 10+ years
from now to figure out why this change was made.
You also forgot to reply to the questions I raised in my previous
reply. Please address those before sending a v4.
> 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;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc)
> {
> struct inode *inode = NULL;
> @@ -233,10 +286,21 @@ static int efivarfs_get_tree(struct fs_context *fc)
>
> static const struct fs_context_operations efivarfs_context_ops = {
> .get_tree = efivarfs_get_tree,
> + .parse_param = efivarfs_parse_param,
> };
>
> static int efivarfs_init_fs_context(struct fs_context *fc)
> {
> + struct efivarfs_fs_info *sfi;
> +
> + sfi = kzalloc(sizeof(struct efivarfs_fs_info), GFP_KERNEL);
> + if (!sfi)
> + return -ENOMEM;
> +
> + sfi->mount_opts.uid = current_uid();
> + sfi->mount_opts.gid = current_gid();
> +
> + fc->s_fs_info = sfi;
> fc->ops = &efivarfs_context_ops;
> return 0;
> }
> @@ -254,6 +318,7 @@ static struct file_system_type efivarfs_type = {
> .name = "efivarfs",
> .init_fs_context = efivarfs_init_fs_context,
> .kill_sb = efivarfs_kill_sb,
> + .parameters = efivarfs_parameters,
> };
>
> static __init int efivarfs_init(void)
> --
> 2.42.0.rc2.253.gd59a3bf2b4-goog
>
Powered by blists - more mailing lists