efi: Make efivarfs entries immutable by default
authorPeter Jones <pjones@redhat.com>
Mon, 8 Feb 2016 19:48:15 +0000 (14:48 -0500)
committerMatt Fleming <matt@codeblueprint.co.uk>
Wed, 10 Feb 2016 16:25:52 +0000 (16:25 +0000)
"rm -rf" is bricking some peoples' laptops because of variables being
used to store non-reinitializable firmware driver data that's required
to POST the hardware.

These are 100% bugs, and they need to be fixed, but in the mean time it
shouldn't be easy to *accidentally* brick machines.

We have to have delete working, and picking which variables do and don't
work for deletion is quite intractable, so instead make everything
immutable by default (except for a whitelist), and make tools that
aren't quite so broad-spectrum unset the immutable flag.

Signed-off-by: Peter Jones <pjones@redhat.com>
Tested-by: Lee, Chun-Yi <jlee@suse.com>
Acked-by: Matthew Garrett <mjg59@coreos.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
Documentation/filesystems/efivarfs.txt
drivers/firmware/efi/vars.c
fs/efivarfs/file.c
fs/efivarfs/inode.c
fs/efivarfs/internal.h
fs/efivarfs/super.c
include/linux/efi.h
tools/testing/selftests/efivarfs/efivarfs.sh
tools/testing/selftests/efivarfs/open-unlink.c

index c477af086e6569398d85b3f938e9f067b1976f7e..686a64bba775e0af9990cf305ab81f1ac4c8c278 100644 (file)
@@ -14,3 +14,10 @@ filesystem.
 efivarfs is typically mounted like this,
 
        mount -t efivarfs none /sys/firmware/efi/efivars
+
+Due to the presence of numerous firmware bugs where removing non-standard
+UEFI variables causes the system firmware to fail to POST, efivarfs
+files that are not well-known standardized variables are created
+as immutable files.  This doesn't prevent removal - "chattr -i" will work -
+but it does prevent this kind of failure from being accomplished
+accidentally.
index 9a53da21e7b63951613e24e67b237ec9aa02740a..50f10bad260453b2e49c1306b0bd1c8ed87dab26 100644 (file)
@@ -172,10 +172,12 @@ struct variable_validate {
 };
 
 /*
- * This is the list of variables we need to validate.
+ * This is the list of variables we need to validate, as well as the
+ * whitelist for what we think is safe not to default to immutable.
  *
  * If it has a validate() method that's not NULL, it'll go into the
- * validation routine.  If not, it is assumed valid.
+ * validation routine.  If not, it is assumed valid, but still used for
+ * whitelisting.
  *
  * Note that it's sorted by {vendor,name}, but globbed names must come after
  * any other name with the same prefix.
@@ -193,11 +195,37 @@ static const struct variable_validate variable_validate[] = {
        { EFI_GLOBAL_VARIABLE_GUID, "ErrOut", validate_device_path },
        { EFI_GLOBAL_VARIABLE_GUID, "ErrOutDev", validate_device_path },
        { EFI_GLOBAL_VARIABLE_GUID, "Lang", validate_ascii_string },
+       { EFI_GLOBAL_VARIABLE_GUID, "OsIndications", NULL },
        { EFI_GLOBAL_VARIABLE_GUID, "PlatformLang", validate_ascii_string },
        { EFI_GLOBAL_VARIABLE_GUID, "Timeout", validate_uint16 },
        { NULL_GUID, "", NULL },
 };
 
+static bool
+variable_matches(const char *var_name, size_t len, const char *match_name,
+                int *match)
+{
+       for (*match = 0; ; (*match)++) {
+               char c = match_name[*match];
+               char u = var_name[*match];
+
+               /* Wildcard in the matching name means we've matched */
+               if (c == '*')
+                       return true;
+
+               /* Case sensitive match */
+               if (!c && *match == len)
+                       return true;
+
+               if (c != u)
+                       return false;
+
+               if (!c)
+                       return true;
+       }
+       return true;
+}
+
 bool
 efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
                unsigned long data_size)
@@ -221,35 +249,48 @@ efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
                if (efi_guidcmp(vendor, variable_validate[i].vendor))
                        continue;
 
-               for (match = 0; ; match++) {
-                       char c = name[match];
-                       char u = utf8_name[match];
-
-                       /* Wildcard in the matching name means we've matched */
-                       if (c == '*') {
-                               kfree(utf8_name);
-                               return variable_validate[i].validate(var_name,
-                                                       match, data, data_size);
-                       }
-
-                       /* Case sensitive match */
-                       if (c != u)
+               if (variable_matches(utf8_name, utf8_size+1, name, &match)) {
+                       if (variable_validate[i].validate == NULL)
                                break;
-
-                       /* Reached the end of the string while matching */
-                       if (!c) {
-                               kfree(utf8_name);
-                               return variable_validate[i].validate(var_name,
-                                                       match, data, data_size);
-                       }
+                       kfree(utf8_name);
+                       return variable_validate[i].validate(var_name, match,
+                                                            data, data_size);
                }
        }
-
        kfree(utf8_name);
        return true;
 }
 EXPORT_SYMBOL_GPL(efivar_validate);
 
+bool
+efivar_variable_is_removable(efi_guid_t vendor, const char *var_name,
+                            size_t len)
+{
+       int i;
+       bool found = false;
+       int match = 0;
+
+       /*
+        * Check if our variable is in the validated variables list
+        */
+       for (i = 0; variable_validate[i].name[0] != '\0'; i++) {
+               if (efi_guidcmp(variable_validate[i].vendor, vendor))
+                       continue;
+
+               if (variable_matches(var_name, len,
+                                    variable_validate[i].name, &match)) {
+                       found = true;
+                       break;
+               }
+       }
+
+       /*
+        * If it's in our list, it is removable.
+        */
+       return found;
+}
+EXPORT_SYMBOL_GPL(efivar_variable_is_removable);
+
 static efi_status_t
 check_var_size(u32 attributes, unsigned long size)
 {
index c424e4813ec8019b5f8a62feb8eb51c6d13f7f97..d48e0d261d78da6e1ce2830e65000e37ed4bd292 100644 (file)
@@ -10,6 +10,7 @@
 #include <linux/efi.h>
 #include <linux/fs.h>
 #include <linux/slab.h>
+#include <linux/mount.h>
 
 #include "internal.h"
 
@@ -103,9 +104,78 @@ out_free:
        return size;
 }
 
+static int
+efivarfs_ioc_getxflags(struct file *file, void __user *arg)
+{
+       struct inode *inode = file->f_mapping->host;
+       unsigned int i_flags;
+       unsigned int flags = 0;
+
+       i_flags = inode->i_flags;
+       if (i_flags & S_IMMUTABLE)
+               flags |= FS_IMMUTABLE_FL;
+
+       if (copy_to_user(arg, &flags, sizeof(flags)))
+               return -EFAULT;
+       return 0;
+}
+
+static int
+efivarfs_ioc_setxflags(struct file *file, void __user *arg)
+{
+       struct inode *inode = file->f_mapping->host;
+       unsigned int flags;
+       unsigned int i_flags = 0;
+       int error;
+
+       if (!inode_owner_or_capable(inode))
+               return -EACCES;
+
+       if (copy_from_user(&flags, arg, sizeof(flags)))
+               return -EFAULT;
+
+       if (flags & ~FS_IMMUTABLE_FL)
+               return -EOPNOTSUPP;
+
+       if (!capable(CAP_LINUX_IMMUTABLE))
+               return -EPERM;
+
+       if (flags & FS_IMMUTABLE_FL)
+               i_flags |= S_IMMUTABLE;
+
+
+       error = mnt_want_write_file(file);
+       if (error)
+               return error;
+
+       inode_lock(inode);
+       inode_set_flags(inode, i_flags, S_IMMUTABLE);
+       inode_unlock(inode);
+
+       mnt_drop_write_file(file);
+
+       return 0;
+}
+
+long
+efivarfs_file_ioctl(struct file *file, unsigned int cmd, unsigned long p)
+{
+       void __user *arg = (void __user *)p;
+
+       switch (cmd) {
+       case FS_IOC_GETFLAGS:
+               return efivarfs_ioc_getxflags(file, arg);
+       case FS_IOC_SETFLAGS:
+               return efivarfs_ioc_setxflags(file, arg);
+       }
+
+       return -ENOTTY;
+}
+
 const struct file_operations efivarfs_file_operations = {
        .open   = simple_open,
        .read   = efivarfs_file_read,
        .write  = efivarfs_file_write,
        .llseek = no_llseek,
+       .unlocked_ioctl = efivarfs_file_ioctl,
 };
index 3381b9da9ee6080881720f0500b71c74ecdf6058..e2ab6d0497f2bb86dee165421487d27e4b64ba0d 100644 (file)
@@ -15,7 +15,8 @@
 #include "internal.h"
 
 struct inode *efivarfs_get_inode(struct super_block *sb,
-                               const struct inode *dir, int mode, dev_t dev)
+                               const struct inode *dir, int mode,
+                               dev_t dev, bool is_removable)
 {
        struct inode *inode = new_inode(sb);
 
@@ -23,6 +24,7 @@ struct inode *efivarfs_get_inode(struct super_block *sb,
                inode->i_ino = get_next_ino();
                inode->i_mode = mode;
                inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+               inode->i_flags = is_removable ? 0 : S_IMMUTABLE;
                switch (mode & S_IFMT) {
                case S_IFREG:
                        inode->i_fop = &efivarfs_file_operations;
@@ -102,22 +104,17 @@ static void efivarfs_hex_to_guid(const char *str, efi_guid_t *guid)
 static int efivarfs_create(struct inode *dir, struct dentry *dentry,
                          umode_t mode, bool excl)
 {
-       struct inode *inode;
+       struct inode *inode = NULL;
        struct efivar_entry *var;
        int namelen, i = 0, err = 0;
+       bool is_removable = false;
 
        if (!efivarfs_valid_name(dentry->d_name.name, dentry->d_name.len))
                return -EINVAL;
 
-       inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
-       if (!inode)
-               return -ENOMEM;
-
        var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
-       if (!var) {
-               err = -ENOMEM;
-               goto out;
-       }
+       if (!var)
+               return -ENOMEM;
 
        /* length of the variable name itself: remove GUID and separator */
        namelen = dentry->d_name.len - EFI_VARIABLE_GUID_LEN - 1;
@@ -125,6 +122,16 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
        efivarfs_hex_to_guid(dentry->d_name.name + namelen + 1,
                        &var->var.VendorGuid);
 
+       if (efivar_variable_is_removable(var->var.VendorGuid,
+                                        dentry->d_name.name, namelen))
+               is_removable = true;
+
+       inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0, is_removable);
+       if (!inode) {
+               err = -ENOMEM;
+               goto out;
+       }
+
        for (i = 0; i < namelen; i++)
                var->var.VariableName[i] = dentry->d_name.name[i];
 
@@ -138,7 +145,8 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 out:
        if (err) {
                kfree(var);
-               iput(inode);
+               if (inode)
+                       iput(inode);
        }
        return err;
 }
index b5ff16addb7ce0984d14d2efe0324be884d20911..b4505188e799b9f5f50b2f792090775fd4244127 100644 (file)
@@ -15,7 +15,8 @@ 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);
 extern struct inode *efivarfs_get_inode(struct super_block *sb,
-                       const struct inode *dir, int mode, dev_t dev);
+                       const struct inode *dir, int mode, dev_t dev,
+                       bool is_removable);
 
 extern struct list_head efivarfs_list;
 
index 8651ac28ec0dccd0a763f92887812e89e8aca2aa..dd029d13ea6140f7df1ed99fb2634a9c858f801f 100644 (file)
@@ -120,6 +120,7 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
        char *name;
        int len;
        int err = -ENOMEM;
+       bool is_removable = false;
 
        entry = kzalloc(sizeof(*entry), GFP_KERNEL);
        if (!entry)
@@ -137,13 +138,17 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
 
        ucs2_as_utf8(name, entry->var.VariableName, len);
 
+       if (efivar_variable_is_removable(entry->var.VendorGuid, name, len))
+               is_removable = true;
+
        name[len] = '-';
 
        efi_guid_to_str(&entry->var.VendorGuid, name + len + 1);
 
        name[len + EFI_VARIABLE_GUID_LEN+1] = '\0';
 
-       inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | 0644, 0);
+       inode = efivarfs_get_inode(sb, d_inode(root), S_IFREG | 0644, 0,
+                                  is_removable);
        if (!inode)
                goto fail_name;
 
@@ -199,7 +204,7 @@ static int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
        sb->s_d_op              = &efivarfs_d_ops;
        sb->s_time_gran         = 1;
 
-       inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0);
+       inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, true);
        if (!inode)
                return -ENOMEM;
        inode->i_op = &efivarfs_dir_inode_operations;
index 16ca611aabc8f9d756b83b9f73f1c27b9d935d7e..47be3ad7d3e5bad63b48a8fa344dbea65c0a97dd 100644 (file)
@@ -1201,6 +1201,8 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
 
 bool efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
                     unsigned long data_size);
+bool efivar_variable_is_removable(efi_guid_t vendor, const char *name,
+                                 size_t len);
 
 extern struct work_struct efivar_work;
 void efivar_run_worker(void);
index 77edcdcc016bbe9e891dbee8ce27c5824caaae63..057278448515a455a17ff2b48af2bf0902c2e7fb 100755 (executable)
@@ -88,7 +88,11 @@ test_delete()
                exit 1
        fi
 
-       rm $file
+       rm $file 2>/dev/null
+       if [ $? -ne 0 ]; then
+               chattr -i $file
+               rm $file
+       fi
 
        if [ -e $file ]; then
                echo "$file couldn't be deleted" >&2
@@ -111,6 +115,7 @@ test_zero_size_delete()
                exit 1
        fi
 
+       chattr -i $file
        printf "$attrs" > $file
 
        if [ -e $file ]; then
@@ -141,7 +146,11 @@ test_valid_filenames()
                        echo "$file could not be created" >&2
                        ret=1
                else
-                       rm $file
+                       rm $file 2>/dev/null
+                       if [ $? -ne 0 ]; then
+                               chattr -i $file
+                               rm $file
+                       fi
                fi
        done
 
@@ -174,7 +183,11 @@ test_invalid_filenames()
 
                if [ -e $file ]; then
                        echo "Creating $file should have failed" >&2
-                       rm $file
+                       rm $file 2>/dev/null
+                       if [ $? -ne 0 ]; then
+                               chattr -i $file
+                               rm $file
+                       fi
                        ret=1
                fi
        done
index 8c0764407b3c349431a6b78ca595e450e2661066..4af74f7330365e668d9e1379f6894f12d974374d 100644 (file)
@@ -1,10 +1,68 @@
+#include <errno.h>
 #include <stdio.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include <sys/ioctl.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
+#include <linux/fs.h>
+
+static int set_immutable(const char *path, int immutable)
+{
+       unsigned int flags;
+       int fd;
+       int rc;
+       int error;
+
+       fd = open(path, O_RDONLY);
+       if (fd < 0)
+               return fd;
+
+       rc = ioctl(fd, FS_IOC_GETFLAGS, &flags);
+       if (rc < 0) {
+               error = errno;
+               close(fd);
+               errno = error;
+               return rc;
+       }
+
+       if (immutable)
+               flags |= FS_IMMUTABLE_FL;
+       else
+               flags &= ~FS_IMMUTABLE_FL;
+
+       rc = ioctl(fd, FS_IOC_SETFLAGS, &flags);
+       error = errno;
+       close(fd);
+       errno = error;
+       return rc;
+}
+
+static int get_immutable(const char *path)
+{
+       unsigned int flags;
+       int fd;
+       int rc;
+       int error;
+
+       fd = open(path, O_RDONLY);
+       if (fd < 0)
+               return fd;
+
+       rc = ioctl(fd, FS_IOC_GETFLAGS, &flags);
+       if (rc < 0) {
+               error = errno;
+               close(fd);
+               errno = error;
+               return rc;
+       }
+       close(fd);
+       if (flags & FS_IMMUTABLE_FL)
+               return 1;
+       return 0;
+}
 
 int main(int argc, char **argv)
 {
@@ -27,7 +85,7 @@ int main(int argc, char **argv)
        buf[4] = 0;
 
        /* create a test variable */
-       fd = open(path, O_WRONLY | O_CREAT);
+       fd = open(path, O_WRONLY | O_CREAT, 0600);
        if (fd < 0) {
                perror("open(O_WRONLY)");
                return EXIT_FAILURE;
@@ -41,6 +99,18 @@ int main(int argc, char **argv)
 
        close(fd);
 
+       rc = get_immutable(path);
+       if (rc < 0) {
+               perror("ioctl(FS_IOC_GETFLAGS)");
+               return EXIT_FAILURE;
+       } else if (rc) {
+               rc = set_immutable(path, 0);
+               if (rc < 0) {
+                       perror("ioctl(FS_IOC_SETFLAGS)");
+                       return EXIT_FAILURE;
+               }
+       }
+
        fd = open(path, O_RDONLY);
        if (fd < 0) {
                perror("open");