| Message ID | 20230822162350.1.I96423a31e88428004c2f4a28ccad13828adf433e@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | kernel: Add Mount Option For Efivarfs | expand |
(cc Peter and Matthew) On Tue, 22 Aug 2023 at 18:24, Jiao Zhou <jiaozhou@google.com> 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 > > Signed-off-by: Jiao Zhou <jiaozhou@google.com> > --- > > fs/efivarfs/inode.c | 4 ++++ > fs/efivarfs/internal.h | 9 +++++++ > fs/efivarfs/super.c | 54 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 67 insertions(+) > > diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c > index b973a2c03dde..86175e229b0f 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 8ebf3a6a8aa2..2c7b6b24df19 100644 > --- a/fs/efivarfs/internal.h > +++ b/fs/efivarfs/internal.h > @@ -48,6 +48,15 @@ bool efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data, > bool efivar_variable_is_removable(efi_guid_t vendor, const char *name, > size_t len); > > +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 e028fafa04f3..e3c81fac8208 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> > @@ -60,10 +61,27 @@ static int efivarfs_statfs(struct dentry *dentry, struct kstatfs *buf) > > return 0; > } > +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 = efivarfs_statfs, > .drop_inode = generic_delete_inode, > .evict_inode = efivarfs_evict_inode, > + .show_options = efivarfs_show_options, > }; > > /* > @@ -225,6 +243,40 @@ 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; > @@ -271,6 +323,7 @@ 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) > @@ -295,6 +348,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.rc1.204.g551eb34607-goog >
On Wed, Aug 23, 2023 at 06:30:12PM +0200, Ard Biesheuvel wrote: > (cc Peter and Matthew) > > On Tue, 22 Aug 2023 at 18:24, Jiao Zhou <jiaozhou@google.com> 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 > > > > Signed-off-by: Jiao Zhou <jiaozhou@google.com> No inherent objection, but what's the use case?
On Thu, Aug 24, 2023 at 12:51:07PM +0100, Jiao Zhou 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 I think it should be upstreamable. Makes sense. Acked-by: Matthew Garrett <mgarrett@aurora.tech>
Hello,
kernel test robot noticed "BUG:KASAN:global-out-of-bounds_in_fs_validate_description" on:
commit: 635056da10724c7af59483a8251f8e6432b50faa ("[PATCH] kernel: Add Mount Option For Efivarfs")
url: https://github.com/intel-lab-lkp/linux/commits/Jiao-Zhou/kernel-Add-Mount-Option-For-Efivarfs/20230823-002613
base: https://git.kernel.org/cgit/linux/kernel/git/efi/efi.git next
patch link: https://lore.kernel.org/all/20230822162350.1.I96423a31e88428004c2f4a28ccad13828adf433e@changeid/
patch subject: [PATCH] kernel: Add Mount Option For Efivarfs
in testcase: boot
compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202308291443.ea96ac66-oliver.sang@intel.com
[ 17.276007][ T1] ==================================================================
[ 17.277001][ T1] BUG: KASAN: global-out-of-bounds in fs_validate_description+0x1c2/0x1e0
[ 17.278097][ T1] Read of size 8 at addr ffffffff84f20b80 by task swapper/1
[ 17.278821][ T1]
[ 17.278821][ T1] CPU: 0 PID: 1 Comm: swapper Not tainted 6.5.0-rc1-00012-g635056da1072 #1
[ 17.278821][ T1] Call Trace:
[ 17.278821][ T1] <TASK>
[ 17.278821][ T1] dump_stack_lvl+0x32/0xa0
[ 17.278821][ T1] print_address_description+0x33/0x3c0
[ 17.278821][ T1] ? fs_validate_description+0x1c2/0x1e0
[ 17.278821][ T1] print_report+0xc1/0x280
[ 17.278821][ T1] ? kasan_addr_to_slab+0x11/0x80
[ 17.278821][ T1] ? fs_validate_description+0x1c2/0x1e0
[ 17.278821][ T1] kasan_report+0x154/0x190
[ 17.278821][ T1] ? fs_validate_description+0x1c2/0x1e0
[ 17.278821][ T1] __asan_report_load8_noabort+0x18/0x20
[ 17.278821][ T1] fs_validate_description+0x1c2/0x1e0
[ 17.278821][ T1] register_filesystem+0x73/0x2b0
[ 17.278821][ T1] ? pstore_blk_init+0x720/0x720
[ 17.278821][ T1] efivarfs_init+0x24/0x50
[ 17.278821][ T1] do_one_initcall+0xfc/0x480
[ 17.278821][ T1] ? trace_event_raw_event_initcall_level+0x1c0/0x1c0
[ 17.278821][ T1] ? __kasan_kmalloc+0x96/0xa0
[ 17.278821][ T1] ? do_initcalls+0x47/0x5b0
[ 17.278821][ T1] do_initcalls+0x2ae/0x5b0
[ 17.278821][ T1] kernel_init_freeable+0x313/0x570
[ 17.278821][ T1] ? rest_init+0x260/0x260
[ 17.278821][ T1] kernel_init+0x23/0x240
[ 17.278821][ T1] ? rest_init+0x260/0x260
[ 17.278821][ T1] ret_from_fork+0x22/0x30
[ 17.278821][ T1] </TASK>
[ 17.278821][ T1]
[ 17.278821][ T1] The buggy address belongs to the variable:
[ 17.278821][ T1] efivarfs_parameters+0x40/0x80
[ 17.278821][ T1]
[ 17.278821][ T1] The buggy address belongs to the physical page:
[ 17.278821][ T1] page:(____ptrval____) refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x4f20
[ 17.278821][ T1] flags: 0x4000000000001000(reserved|zone=1)
[ 17.278821][ T1] page_type: 0xffffffff()
[ 17.278821][ T1] raw: 4000000000001000 ffffea000013c808 ffffea000013c808 0000000000000000
[ 17.278821][ T1] raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000
[ 17.278821][ T1] page dumped because: kasan: bad access detected
[ 17.278821][ T1] page_owner info is not present (never set?)
[ 17.278821][ T1]
[ 17.278821][ T1] Memory state around the buggy address:
[ 17.278821][ T1] ffffffff84f20a80: 00 00 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
[ 17.278821][ T1] ffffffff84f20b00: 04 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00
[ 17.278821][ T1] >ffffffff84f20b80: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
[ 17.278821][ T1] ^
[ 17.278821][ T1] ffffffff84f20c00: 00 00 00 00 00 00 00 00 f9 f9 f9 f9 00 00 00 00
[ 17.278821][ T1] ffffffff84f20c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 17.278821][ T1] ==================================================================
[ 17.311721][ T1] Disabling lock debugging due to kernel taint
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20230829/202308291443.ea96ac66-oliver.sang@intel.com
On Tue, 22 Aug 2023 at 18:24, Jiao Zhou <jiaozhou@google.com> 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 > > Signed-off-by: Jiao Zhou <jiaozhou@google.com> > --- > > fs/efivarfs/inode.c | 4 ++++ > fs/efivarfs/internal.h | 9 +++++++ > fs/efivarfs/super.c | 54 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 67 insertions(+) > > diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c > index b973a2c03dde..86175e229b0f 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 8ebf3a6a8aa2..2c7b6b24df19 100644 > --- a/fs/efivarfs/internal.h > +++ b/fs/efivarfs/internal.h > @@ -48,6 +48,15 @@ bool efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data, > bool efivar_variable_is_removable(efi_guid_t vendor, const char *name, > size_t len); > > +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 e028fafa04f3..e3c81fac8208 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> > @@ -60,10 +61,27 @@ static int efivarfs_statfs(struct dentry *dentry, struct kstatfs *buf) > > return 0; > } > +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 = efivarfs_statfs, > .drop_inode = generic_delete_inode, > .evict_inode = efivarfs_evict_inode, > + .show_options = efivarfs_show_options, > }; > > /* > @@ -225,6 +243,40 @@ 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), > +}; > + The kasan report seems to suggest that this array needs a trailing empty entry {} > +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; > @@ -271,6 +323,7 @@ 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) > @@ -295,6 +348,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.rc1.204.g551eb34607-goog >
diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c index b973a2c03dde..86175e229b0f 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 8ebf3a6a8aa2..2c7b6b24df19 100644 --- a/fs/efivarfs/internal.h +++ b/fs/efivarfs/internal.h @@ -48,6 +48,15 @@ bool efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data, bool efivar_variable_is_removable(efi_guid_t vendor, const char *name, size_t len); +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 e028fafa04f3..e3c81fac8208 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> @@ -60,10 +61,27 @@ static int efivarfs_statfs(struct dentry *dentry, struct kstatfs *buf) return 0; } +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 = efivarfs_statfs, .drop_inode = generic_delete_inode, .evict_inode = efivarfs_evict_inode, + .show_options = efivarfs_show_options, }; /* @@ -225,6 +243,40 @@ 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; @@ -271,6 +323,7 @@ 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) @@ -295,6 +348,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)
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 Signed-off-by: Jiao Zhou <jiaozhou@google.com> --- fs/efivarfs/inode.c | 4 ++++ fs/efivarfs/internal.h | 9 +++++++ fs/efivarfs/super.c | 54 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+)