Since commit 94f7f6182c72 ("efivarfs: automatically update super block flag") efivarfs_ops_notifier() sets or clears SB_RDONLY whenever the efivar ops switch between ops that can and ops that can't set variables, e.g. when tee_stmm_efi is probed or removed. It does that at any time without holding s_umount. efivarfs is frozen on every suspend and if the ops switch while it's frozen thaw_super() either leaves the s_writers rwsems locked or releases rwsems nobody holds. So keep the state in efivarfs. The notifier sets and clears a new EFIVARFS_OPS_RDONLY flag instead and efivarfs_fill_super() initializes it under efivar_lock() so a switch can't slip in. fill_super doesn't set SB_RDONLY either. systemd mounts efivarfs before tee_stmm_efi is loaded and the mount has to become writable then. create, unlink, setattr, fileattr_set, write and opening for write fail with EROFS while the flag is set. Like the SB_RDONLY check this runs without efivars_lock so a write racing with a switch can still call a NULL ->set_variable. That's not new. efivarfs_reconfigure() only refuses a remount from read-only to read-write while the flag is set and returns EROFS. It used to refuse every remount without "ro" which would break option-only remounts. With read-only ops efivarfs is mounted "rw" now. statfs() doesn't report ST_RDONLY anymore and access(W_OK) succeeds. Fixes: 94f7f6182c72 ("efivarfs: automatically update super block flag") Cc: stable@vger.kernel.org Signed-off-by: Christian Brauner (Amutable) --- fs/efivarfs/file.c | 6 ++++++ fs/efivarfs/inode.c | 15 +++++++++++++++ fs/efivarfs/internal.h | 14 +++++++++++++- fs/efivarfs/super.c | 26 ++++++++++++++++++-------- 4 files changed, 52 insertions(+), 9 deletions(-) diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c index cb1b6d0c34545..a074145fa57e1 100644 --- a/fs/efivarfs/file.c +++ b/fs/efivarfs/file.c @@ -23,6 +23,9 @@ static ssize_t efivarfs_file_write(struct file *file, ssize_t bytes; bool set = false; + if (efivarfs_ops_rdonly(inode->i_sb)) + return -EROFS; + if (count < sizeof(attributes)) return -EINVAL; @@ -137,6 +140,9 @@ static int efivarfs_file_open(struct inode *inode, struct file *file) { struct efivar_entry *entry = inode->i_private; + if ((file->f_mode & FMODE_WRITE) && efivarfs_ops_rdonly(inode->i_sb)) + return -EROFS; + file->private_data = entry; inode_lock(inode); diff --git a/fs/efivarfs/inode.c b/fs/efivarfs/inode.c index 07602cd5d33c3..be183de4b7716 100644 --- a/fs/efivarfs/inode.c +++ b/fs/efivarfs/inode.c @@ -15,6 +15,8 @@ #include "internal.h" static const struct inode_operations efivarfs_file_inode_operations; +static int efivarfs_setattr(const struct mnt_idmap *idmap, + struct dentry *dentry, struct iattr *iattr); struct inode *efivarfs_get_inode(struct super_block *sb, const struct inode *dir, int mode, @@ -83,6 +85,9 @@ static int efivarfs_create(const struct mnt_idmap *idmap, struct inode *dir, bool is_removable = false; efi_guid_t vendor; + if (efivarfs_ops_rdonly(dir->i_sb)) + return -EROFS; + if (!efivarfs_valid_name(dentry->d_name.name, dentry->d_name.len)) return -EINVAL; @@ -122,6 +127,9 @@ static int efivarfs_unlink(struct inode *dir, struct dentry *dentry) { struct efivar_entry *var = d_inode(dentry)->i_private; + if (efivarfs_ops_rdonly(dir->i_sb)) + return -EROFS; + if (efivar_entry_delete(var)) return -EINVAL; @@ -132,6 +140,7 @@ const struct inode_operations efivarfs_dir_inode_operations = { .lookup = simple_lookup, .unlink = efivarfs_unlink, .create = efivarfs_create, + .setattr = efivarfs_setattr, }; static int @@ -155,6 +164,9 @@ efivarfs_fileattr_set(const struct mnt_idmap *idmap, { unsigned int i_flags = 0; + if (efivarfs_ops_rdonly(dentry->d_sb)) + return -EROFS; + if (fileattr_has_fsx(fa)) return -EOPNOTSUPP; @@ -176,6 +188,9 @@ static int efivarfs_setattr(const struct mnt_idmap *idmap, struct dentry *dentry struct inode *inode = d_inode(dentry); int error; + if (efivarfs_ops_rdonly(inode->i_sb)) + return -EROFS; + error = setattr_prepare(idmap, dentry, iattr); if (error) return error; diff --git a/fs/efivarfs/internal.h b/fs/efivarfs/internal.h index f5c1a25be9969..34a0c53b5993f 100644 --- a/fs/efivarfs/internal.h +++ b/fs/efivarfs/internal.h @@ -16,10 +16,22 @@ struct efivarfs_mount_opts { struct efivarfs_fs_info { struct efivarfs_mount_opts mount_opts; - struct super_block *sb; struct notifier_block nb; + unsigned long flags; }; +/* efivarfs_fs_info->flags */ +enum { + EFIVARFS_OPS_RDONLY, /* the registered efivar ops can't set variables */ +}; + +static inline bool efivarfs_ops_rdonly(const struct super_block *sb) +{ + const struct efivarfs_fs_info *sfi = sb->s_fs_info; + + return test_bit(EFIVARFS_OPS_RDONLY, &sfi->flags); +} + struct efi_variable { efi_char16_t VariableName[EFI_VAR_NAME_LEN/sizeof(efi_char16_t)]; efi_guid_t VendorGuid; diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c index 12746ced6dc63..437dfea9aa42d 100644 --- a/fs/efivarfs/super.c +++ b/fs/efivarfs/super.c @@ -28,12 +28,16 @@ static int efivarfs_ops_notifier(struct notifier_block *nb, unsigned long event, { struct efivarfs_fs_info *sfi = container_of(nb, struct efivarfs_fs_info, nb); + /* + * This runs without s_umount so it can't touch SB_RDONLY. The + * modifying operations check EFIVARFS_OPS_RDONLY instead. + */ switch (event) { case EFIVAR_OPS_RDONLY: - sfi->sb->s_flags |= SB_RDONLY; + set_bit(EFIVARFS_OPS_RDONLY, &sfi->flags); break; case EFIVAR_OPS_RDWR: - sfi->sb->s_flags &= ~SB_RDONLY; + clear_bit(EFIVARFS_OPS_RDONLY, &sfi->flags); break; default: return NOTIFY_DONE; @@ -384,9 +388,6 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc) sb->s_d_flags |= DCACHE_DONTCACHE; sb->s_time_gran = 1; - if (!efivar_supports_writes()) - sb->s_flags |= SB_RDONLY; - inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, true); if (!inode) return -ENOMEM; @@ -397,9 +398,15 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc) if (!root) return -ENOMEM; - sfi->sb = sb; + /* The ops can't switch between sampling them and registering. */ + err = efivar_lock(); + if (err) + return err; + if (!efivar_supports_writes()) + set_bit(EFIVARFS_OPS_RDONLY, &sfi->flags); sfi->nb.notifier_call = efivarfs_ops_notifier; err = blocking_notifier_chain_register(&efivar_ops_nh, &sfi->nb); + efivar_unlock(); if (err) return err; @@ -413,12 +420,15 @@ static int efivarfs_get_tree(struct fs_context *fc) static int efivarfs_reconfigure(struct fs_context *fc) { + struct super_block *sb = fc->root->d_sb; struct efivarfs_fs_info *sfi = fc->root->d_sb->s_fs_info; struct efivarfs_fs_info *new_sfi = fc->s_fs_info; - if (!efivar_supports_writes() && !(fc->sb_flags & SB_RDONLY)) { + /* Only refuse remounting a read-only mount read-write. */ + if ((fc->sb_flags_mask & SB_RDONLY) && !(fc->sb_flags & SB_RDONLY) && + sb_rdonly(sb) && efivarfs_ops_rdonly(sb)) { pr_err("Firmware does not support SetVariableRT. Can not remount with rw\n"); - return -EINVAL; + return -EROFS; } /* statfs()/show_options() read nostatfs locklessly; benign race. */ -- 2.53.0