[PATCH 00/20] EFI changes for v3.8

[PATCH 00/20] EFI changes for v3.8

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

I've got the following patches queued up on my next branch for the
v3.8 merge window. The patches are available at,

  git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git next

The main changes are,

    * A new EFI variable filesystem, efivarfs, which addresses the
      shortcomings of the old sysfs EFI variable code.

    * We now switch to a 1:1 mapped pagetable for all virtual EFI
      calls. This is actually a workaround for an ASUS firmware bug
      which expects the physical mapping to still be around at
      runtime.

    * Because of the above fix Jan's patch to use EFI for the platform
      wall clock can be re-applied (it was originally reverted because
      it broke some ASUS machines).

Andy Whitcroft (5):
  efivarfs: efivarfs_file_read ensure we free data in error paths
  efivarfs: efivarfs_create() ensure we drop our reference on inode on
    error
  efivarfs: efivarfs_fill_super() fix inode reference counts
  efivarfs: efivarfs_fill_super() ensure we free our temporary name
  efivarfs: efivarfs_fill_super() ensure we clean up correctly on error

Jan Beulich (1):
  x86-64/efi: Use EFI to deal with platform wall clock (again)

Jeremy Kerr (3):
  efi: Handle deletions and size changes in efivarfs_write_file
  efivarfs: Implement exclusive access for {get,set}_variable
  efi: Clarify GUID length calculations

Lee, Chun-Yi (1):
  efi: add efivars kobject to efi sysfs folder

Matt Fleming (8):
  efivarfs: Add documentation for the EFI variable filesystem
  x86, mm: Include the entire kernel memory map in trampoline_pgd
  x86, efi: 1:1 pagetable mapping for virtual EFI calls
  efivarfs: Return an error if we fail to read a variable
  efivarfs: Replace magic number with sizeof(attributes)
  efivarfs: Add unique magic number
  efivarfs: Make 'datasize' unsigned long
  efivarfs: Return a consistent error when efivarfs_get_inode() fails

Matthew Garrett (1):
  efi: Add support for a UEFI variable filesystem

Xiaoyan Zhang (1):
  x86/kernel: remove tboot 1:1 page table creation code

 Documentation/filesystems/00-INDEX     |   2 +
 Documentation/filesystems/efivarfs.txt |  16 ++
 arch/x86/include/asm/efi.h             |  28 +-
 arch/x86/kernel/tboot.c                |  78 +-----
 arch/x86/mm/init_64.c                  |   9 +-
 arch/x86/mm/ioremap.c                  | 105 +++++++
 arch/x86/mm/pageattr.c                 |  10 +-
 arch/x86/platform/efi/efi.c            |  30 +-
 arch/x86/platform/efi/efi_64.c         |  15 +
 arch/x86/realmode/init.c               |  17 +-
 drivers/firmware/efivars.c             | 487 ++++++++++++++++++++++++++++++++-
 include/linux/efi.h                    |   8 +-
 include/uapi/linux/magic.h             |   1 +
 init/main.c                            |   8 +-
 14 files changed, 687 insertions(+), 127 deletions(-)
 create mode 100644 Documentation/filesystems/efivarfs.txt

-- 
1.7.11.7

[PATCH 01/20] efi: Add support for a UEFI variable filesystem

[Matt Fleming]

From: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

The existing EFI variables code only supports variables of up to 1024
bytes. This limitation existed in version 0.99 of the EFI specification,
but was removed before any full releases. Since variables can now be
larger than a single page, sysfs isn't the best interface for this. So,
instead, let's add a filesystem. Variables can be read, written and
created, with the first 4 bytes of each variable representing its UEFI
attributes. The create() method doesn't actually commit to flash since
zero-length variables can't exist per-spec.

Updates from Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>.

Signed-off-by: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 384 ++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/efi.h        |   5 +
 2 files changed, 383 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index d10c987..a1d6940 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -80,6 +80,10 @@
 #include <linux/slab.h>
 #include <linux/pstore.h>
 
+#include <linux/fs.h>
+#include <linux/ramfs.h>
+#include <linux/pagemap.h>
+
 #include <asm/uaccess.h>
 
 #define EFIVARS_VERSION "0.08"
@@ -91,6 +95,7 @@ MODULE_LICENSE("GPL");
 MODULE_VERSION(EFIVARS_VERSION);
 
 #define DUMP_NAME_LEN 52
+#define GUID_LEN 37
 
 /*
  * The maximum size of VariableName + Data = 1024
@@ -108,7 +113,6 @@ struct efi_variable {
 	__u32         Attributes;
 } __attribute__((packed));
 
-
 struct efivar_entry {
 	struct efivars *efivars;
 	struct efi_variable var;
@@ -122,6 +126,9 @@ struct efivar_attribute {
 	ssize_t (*store)(struct efivar_entry *entry, const char *buf, size_t count);
 };
 
+static struct efivars __efivars;
+static struct efivar_operations ops;
+
 #define PSTORE_EFI_ATTRIBUTES \
 	(EFI_VARIABLE_NON_VOLATILE | \
 	 EFI_VARIABLE_BOOTSERVICE_ACCESS | \
@@ -629,14 +636,380 @@ static struct kobj_type efivar_ktype = {
 	.default_attrs = def_attrs,
 };
 
-static struct pstore_info efi_pstore_info;
-
 static inline void
 efivar_unregister(struct efivar_entry *var)
 {
 	kobject_put(&var->kobj);
 }
 
+static int efivarfs_file_open(struct inode *inode, struct file *file)
+{
+	file->private_data = inode->i_private;
+	return 0;
+}
+
+static ssize_t efivarfs_file_write(struct file *file,
+		const char __user *userbuf, size_t count, loff_t *ppos)
+{
+	struct efivar_entry *var = file->private_data;
+	struct efivars *efivars;
+	efi_status_t status;
+	void *data;
+	u32 attributes;
+	struct inode *inode = file->f_mapping->host;
+	int datasize = count - sizeof(attributes);
+
+	if (count < sizeof(attributes))
+		return -EINVAL;
+
+	data = kmalloc(datasize, GFP_KERNEL);
+
+	if (!data)
+		return -ENOMEM;
+
+	efivars = var->efivars;
+
+	if (copy_from_user(&attributes, userbuf, sizeof(attributes))) {
+		count = -EFAULT;
+		goto out;
+	}
+
+	if (attributes & ~(EFI_VARIABLE_MASK)) {
+		count = -EINVAL;
+		goto out;
+	}
+
+	if (copy_from_user(data, userbuf + sizeof(attributes), datasize)) {
+		count = -EFAULT;
+		goto out;
+	}
+
+	if (validate_var(&var->var, data, datasize) == false) {
+		count = -EINVAL;
+		goto out;
+	}
+
+	status = efivars->ops->set_variable(var->var.VariableName,
+					    &var->var.VendorGuid,
+					    attributes, datasize,
+					    data);
+
+	switch (status) {
+	case EFI_SUCCESS:
+		mutex_lock(&inode->i_mutex);
+		i_size_write(inode, count);
+		mutex_unlock(&inode->i_mutex);
+		break;
+	case EFI_INVALID_PARAMETER:
+		count = -EINVAL;
+		break;
+	case EFI_OUT_OF_RESOURCES:
+		count = -ENOSPC;
+		break;
+	case EFI_DEVICE_ERROR:
+		count = -EIO;
+		break;
+	case EFI_WRITE_PROTECTED:
+		count = -EROFS;
+		break;
+	case EFI_SECURITY_VIOLATION:
+		count = -EACCES;
+		break;
+	case EFI_NOT_FOUND:
+		count = -ENOENT;
+		break;
+	default:
+		count = -EINVAL;
+		break;
+	}
+out:
+	kfree(data);
+
+	return count;
+}
+
+static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
+		size_t count, loff_t *ppos)
+{
+	struct efivar_entry *var = file->private_data;
+	struct efivars *efivars = var->efivars;
+	efi_status_t status;
+	unsigned long datasize = 0;
+	u32 attributes;
+	void *data;
+	ssize_t size;
+
+	status = efivars->ops->get_variable(var->var.VariableName,
+					    &var->var.VendorGuid,
+					    &attributes, &datasize, NULL);
+
+	if (status != EFI_BUFFER_TOO_SMALL)
+		return 0;
+
+	data = kmalloc(datasize + 4, GFP_KERNEL);
+
+	if (!data)
+		return 0;
+
+	status = efivars->ops->get_variable(var->var.VariableName,
+					    &var->var.VendorGuid,
+					    &attributes, &datasize,
+					    (data + 4));
+
+	if (status != EFI_SUCCESS)
+		return 0;
+
+	memcpy(data, &attributes, 4);
+	size = simple_read_from_buffer(userbuf, count, ppos,
+					data, datasize + 4);
+	kfree(data);
+
+	return size;
+}
+
+static void efivarfs_evict_inode(struct inode *inode)
+{
+	clear_inode(inode);
+}
+
+static const struct super_operations efivarfs_ops = {
+	.statfs = simple_statfs,
+	.drop_inode = generic_delete_inode,
+	.evict_inode = efivarfs_evict_inode,
+	.show_options = generic_show_options,
+};
+
+static struct super_block *efivarfs_sb;
+
+static const struct inode_operations efivarfs_dir_inode_operations;
+
+static const struct file_operations efivarfs_file_operations = {
+	.open	= efivarfs_file_open,
+	.read	= efivarfs_file_read,
+	.write	= efivarfs_file_write,
+	.llseek	= default_llseek,
+};
+
+static struct inode *efivarfs_get_inode(struct super_block *sb,
+				const struct inode *dir, int mode, dev_t dev)
+{
+	struct inode *inode = new_inode(sb);
+
+	if (inode) {
+		inode->i_ino = get_next_ino();
+		inode->i_uid = inode->i_gid = 0;
+		inode->i_mode = mode;
+		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+		switch (mode & S_IFMT) {
+		case S_IFREG:
+			inode->i_fop = &efivarfs_file_operations;
+			break;
+		case S_IFDIR:
+			inode->i_op = &efivarfs_dir_inode_operations;
+			inode->i_fop = &simple_dir_operations;
+			inc_nlink(inode);
+			break;
+		}
+	}
+	return inode;
+}
+
+static void efivarfs_hex_to_guid(const char *str, efi_guid_t *guid)
+{
+	guid->b[0] = hex_to_bin(str[6]) << 4 | hex_to_bin(str[7]);
+	guid->b[1] = hex_to_bin(str[4]) << 4 | hex_to_bin(str[5]);
+	guid->b[2] = hex_to_bin(str[2]) << 4 | hex_to_bin(str[3]);
+	guid->b[3] = hex_to_bin(str[0]) << 4 | hex_to_bin(str[1]);
+	guid->b[4] = hex_to_bin(str[11]) << 4 | hex_to_bin(str[12]);
+	guid->b[5] = hex_to_bin(str[9]) << 4 | hex_to_bin(str[10]);
+	guid->b[6] = hex_to_bin(str[16]) << 4 | hex_to_bin(str[17]);
+	guid->b[7] = hex_to_bin(str[14]) << 4 | hex_to_bin(str[15]);
+	guid->b[8] = hex_to_bin(str[19]) << 4 | hex_to_bin(str[20]);
+	guid->b[9] = hex_to_bin(str[21]) << 4 | hex_to_bin(str[22]);
+	guid->b[10] = hex_to_bin(str[24]) << 4 | hex_to_bin(str[25]);
+	guid->b[11] = hex_to_bin(str[26]) << 4 | hex_to_bin(str[27]);
+	guid->b[12] = hex_to_bin(str[28]) << 4 | hex_to_bin(str[29]);
+	guid->b[13] = hex_to_bin(str[30]) << 4 | hex_to_bin(str[31]);
+	guid->b[14] = hex_to_bin(str[32]) << 4 | hex_to_bin(str[33]);
+	guid->b[15] = hex_to_bin(str[34]) << 4 | hex_to_bin(str[35]);
+}
+
+static int efivarfs_create(struct inode *dir, struct dentry *dentry,
+			  umode_t mode, bool excl)
+{
+	struct inode *inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
+	struct efivars *efivars = &__efivars;
+	struct efivar_entry *var;
+	int namelen, i = 0, err = 0;
+
+	if (dentry->d_name.len < 38)
+		return -EINVAL;
+
+	if (!inode)
+		return -ENOSPC;
+
+	var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
+
+	if (!var)
+		return -ENOMEM;
+
+	namelen = dentry->d_name.len - GUID_LEN;
+
+	efivarfs_hex_to_guid(dentry->d_name.name + namelen + 1,
+			&var->var.VendorGuid);
+
+	for (i = 0; i < namelen; i++)
+		var->var.VariableName[i] = dentry->d_name.name[i];
+
+	var->var.VariableName[i] = '\0';
+
+	inode->i_private = var;
+	var->efivars = efivars;
+	var->kobj.kset = efivars->kset;
+
+	err = kobject_init_and_add(&var->kobj, &efivar_ktype, NULL, "%s",
+			     dentry->d_name.name);
+	if (err)
+		goto out;
+
+	kobject_uevent(&var->kobj, KOBJ_ADD);
+	spin_lock(&efivars->lock);
+	list_add(&var->list, &efivars->list);
+	spin_unlock(&efivars->lock);
+	d_instantiate(dentry, inode);
+	dget(dentry);
+out:
+	if (err)
+		kfree(var);
+	return err;
+}
+
+static int efivarfs_unlink(struct inode *dir, struct dentry *dentry)
+{
+	struct efivar_entry *var = dentry->d_inode->i_private;
+	struct efivars *efivars = var->efivars;
+	efi_status_t status;
+
+	spin_lock(&efivars->lock);
+
+	status = efivars->ops->set_variable(var->var.VariableName,
+					    &var->var.VendorGuid,
+					    0, 0, NULL);
+
+	if (status == EFI_SUCCESS || status == EFI_NOT_FOUND) {
+		list_del(&var->list);
+		spin_unlock(&efivars->lock);
+		efivar_unregister(var);
+		drop_nlink(dir);
+		dput(dentry);
+		return 0;
+	}
+
+	spin_unlock(&efivars->lock);
+	return -EINVAL;
+};
+
+int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
+{
+	struct inode *inode = NULL;
+	struct dentry *root;
+	struct efivar_entry *entry, *n;
+	struct efivars *efivars = &__efivars;
+	int err;
+
+	efivarfs_sb = sb;
+
+	sb->s_maxbytes          = MAX_LFS_FILESIZE;
+	sb->s_blocksize         = PAGE_CACHE_SIZE;
+	sb->s_blocksize_bits    = PAGE_CACHE_SHIFT;
+	sb->s_magic             = PSTOREFS_MAGIC;
+	sb->s_op                = &efivarfs_ops;
+	sb->s_time_gran         = 1;
+
+	inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0);
+	if (!inode) {
+		err = -ENOMEM;
+		goto fail;
+	}
+	inode->i_op = &efivarfs_dir_inode_operations;
+
+	root = d_make_root(inode);
+	sb->s_root = root;
+	if (!root) {
+		err = -ENOMEM;
+		goto fail;
+	}
+
+	list_for_each_entry_safe(entry, n, &efivars->list, list) {
+		struct inode *inode;
+		struct dentry *dentry, *root = efivarfs_sb->s_root;
+		char *name;
+		unsigned long size = 0;
+		int len, i;
+
+		len = utf16_strlen(entry->var.VariableName);
+
+		/* GUID plus trailing NULL */
+		name = kmalloc(len + 38, GFP_ATOMIC);
+
+		for (i = 0; i < len; i++)
+			name[i] = entry->var.VariableName[i] & 0xFF;
+
+		name[len] = '-';
+
+		efi_guid_unparse(&entry->var.VendorGuid, name + len + 1);
+
+		name[len+GUID_LEN] = '\0';
+
+		inode = efivarfs_get_inode(efivarfs_sb, root->d_inode,
+					  S_IFREG | 0644, 0);
+		dentry = d_alloc_name(root, name);
+
+		efivars->ops->get_variable(entry->var.VariableName,
+					   &entry->var.VendorGuid,
+					   &entry->var.Attributes,
+					   &size,
+					   NULL);
+
+		mutex_lock(&inode->i_mutex);
+		inode->i_private = entry;
+		i_size_write(inode, size+4);
+		mutex_unlock(&inode->i_mutex);
+		d_add(dentry, inode);
+	}
+
+	return 0;
+fail:
+	iput(inode);
+	return err;
+}
+
+static struct dentry *efivarfs_mount(struct file_system_type *fs_type,
+				    int flags, const char *dev_name, void *data)
+{
+	return mount_single(fs_type, flags, data, efivarfs_fill_super);
+}
+
+static void efivarfs_kill_sb(struct super_block *sb)
+{
+	kill_litter_super(sb);
+	efivarfs_sb = NULL;
+}
+
+static struct file_system_type efivarfs_type = {
+	.name    = "efivarfs",
+	.mount   = efivarfs_mount,
+	.kill_sb = efivarfs_kill_sb,
+};
+
+static const struct inode_operations efivarfs_dir_inode_operations = {
+	.lookup = simple_lookup,
+	.unlink = efivarfs_unlink,
+	.create = efivarfs_create,
+};
+
+static struct pstore_info efi_pstore_info;
+
 #ifdef CONFIG_PSTORE
 
 static int efi_pstore_open(struct pstore_info *psi)
@@ -1198,6 +1571,8 @@ int register_efivars(struct efivars *efivars,
 		pstore_register(&efivars->efi_pstore_info);
 	}
 
+	register_filesystem(&efivarfs_type);
+
 out:
 	kfree(variable_name);
 
@@ -1205,9 +1580,6 @@ out:
 }
 EXPORT_SYMBOL_GPL(register_efivars);
 
-static struct efivars __efivars;
-static struct efivar_operations ops;
-
 /*
  * For now we register the efi subsystem with the firmware subsystem
  * and the vars subsystem with the efi subsystem.  In the future, it
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8670eb1..b2af157 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -29,7 +29,12 @@
 #define EFI_UNSUPPORTED		( 3 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_BAD_BUFFER_SIZE     ( 4 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_BUFFER_TOO_SMALL	( 5 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_NOT_READY		( 6 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_DEVICE_ERROR	( 7 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_WRITE_PROTECTED	( 8 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_OUT_OF_RESOURCES	( 9 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_NOT_FOUND		(14 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_SECURITY_VIOLATION	(26 | (1UL << (BITS_PER_LONG-1)))
 
 typedef unsigned long efi_status_t;
 typedef u8 efi_bool_t;
-- 
1.7.11.7

[PATCH 02/20] efi: Handle deletions and size changes in efivarfs_write_file

[Matt Fleming]

From: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

A write to an efivarfs file will not always result in a variable of
'count' size after the EFI SetVariable() call. We may have appended to
the existing data (ie, with the EFI_VARIABLE_APPEND_WRITE attribute), or
even have deleted the variable (with an authenticated variable update,
with a zero datasize).

This change re-reads the updated variable from firmware, to check for
size changes and deletions. In the latter case, we need to drop the
dentry.

Signed-off-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 49 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index a1d6940..6d43bbd 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -658,6 +658,7 @@ static ssize_t efivarfs_file_write(struct file *file,
 	u32 attributes;
 	struct inode *inode = file->f_mapping->host;
 	int datasize = count - sizeof(attributes);
+	unsigned long newdatasize;
 
 	if (count < sizeof(attributes))
 		return -EINVAL;
@@ -696,32 +697,60 @@ static ssize_t efivarfs_file_write(struct file *file,
 
 	switch (status) {
 	case EFI_SUCCESS:
-		mutex_lock(&inode->i_mutex);
-		i_size_write(inode, count);
-		mutex_unlock(&inode->i_mutex);
 		break;
 	case EFI_INVALID_PARAMETER:
 		count = -EINVAL;
-		break;
+		goto out;
 	case EFI_OUT_OF_RESOURCES:
 		count = -ENOSPC;
-		break;
+		goto out;
 	case EFI_DEVICE_ERROR:
 		count = -EIO;
-		break;
+		goto out;
 	case EFI_WRITE_PROTECTED:
 		count = -EROFS;
-		break;
+		goto out;
 	case EFI_SECURITY_VIOLATION:
 		count = -EACCES;
-		break;
+		goto out;
 	case EFI_NOT_FOUND:
 		count = -ENOENT;
-		break;
+		goto out;
 	default:
 		count = -EINVAL;
-		break;
+		goto out;
 	}
+
+	/*
+	 * Writing to the variable may have caused a change in size (which
+	 * could either be an append or an overwrite), or the variable to be
+	 * deleted. Perform a GetVariable() so we can tell what actually
+	 * happened.
+	 */
+	newdatasize = 0;
+	status = efivars->ops->get_variable(var->var.VariableName,
+					    &var->var.VendorGuid,
+					    NULL, &newdatasize,
+					    NULL);
+
+	if (status == EFI_BUFFER_TOO_SMALL) {
+		mutex_lock(&inode->i_mutex);
+		i_size_write(inode, newdatasize + sizeof(attributes));
+		mutex_unlock(&inode->i_mutex);
+
+	} else if (status == EFI_NOT_FOUND) {
+		spin_lock(&efivars->lock);
+		list_del(&var->list);
+		spin_unlock(&efivars->lock);
+		efivar_unregister(var);
+		drop_nlink(inode);
+		dput(file->f_dentry);
+
+	} else {
+		pr_warn("efivarfs: inconsistent EFI variable implementation? "
+				"status = %lx\n", status);
+	}
+
 out:
 	kfree(data);
 
-- 
1.7.11.7

[PATCH 03/20] efi: add efivars kobject to efi sysfs folder

[Matt Fleming]

From: "Lee, Chun-Yi" <joeyli.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

UEFI variable filesystem need a new mount point, so this patch add
efivars kobject to efi_kobj for create a /sys/firmware/efi/efivars
folder.

Cc: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Signed-off-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 11 +++++++++++
 include/linux/efi.h        |  1 +
 2 files changed, 12 insertions(+)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 6d43bbd..4b12a8fd 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1527,6 +1527,7 @@ void unregister_efivars(struct efivars *efivars)
 		sysfs_remove_bin_file(&efivars->kset->kobj, efivars->del_var);
 	kfree(efivars->new_var);
 	kfree(efivars->del_var);
+	kobject_put(efivars->kobject);
 	kset_unregister(efivars->kset);
 }
 EXPORT_SYMBOL_GPL(unregister_efivars);
@@ -1558,6 +1559,13 @@ int register_efivars(struct efivars *efivars,
 		goto out;
 	}
 
+	efivars->kobject = kobject_create_and_add("efivars", parent_kobj);
+	if (!efivars->kobject) {
+		pr_err("efivars: Subsystem registration failed.\n");
+		error = -ENOMEM;
+		goto err_unreg_vars;
+	}
+
 	/*
 	 * Per EFI spec, the maximum storage allocated for both
 	 * the variable name and variable data is 1024 bytes.
@@ -1602,6 +1610,9 @@ int register_efivars(struct efivars *efivars,
 
 	register_filesystem(&efivarfs_type);
 
+err_unreg_vars:
+	kset_unregister(efivars->kset);
+
 out:
 	kfree(variable_name);
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index b2af157..337aefb 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -662,6 +662,7 @@ struct efivars {
 	spinlock_t lock;
 	struct list_head list;
 	struct kset *kset;
+	struct kobject *kobject;
 	struct bin_attribute *new_var, *del_var;
 	const struct efivar_operations *ops;
 	struct efivar_entry *walk_entry;
-- 
1.7.11.7

[PATCH 04/20] efivarfs: Add documentation for the EFI variable filesystem

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 Documentation/filesystems/00-INDEX     |  2 ++
 Documentation/filesystems/efivarfs.txt | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)
 create mode 100644 Documentation/filesystems/efivarfs.txt

diff --git a/Documentation/filesystems/00-INDEX b/Documentation/filesystems/00-INDEX
index 8c624a1..7b52ba7 100644
--- a/Documentation/filesystems/00-INDEX
+++ b/Documentation/filesystems/00-INDEX
@@ -38,6 +38,8 @@ dnotify_test.c
 	- example program for dnotify
 ecryptfs.txt
 	- docs on eCryptfs: stacked cryptographic filesystem for Linux.
+efivarfs.txt
+	- info for the efivarfs filesystem.
 exofs.txt
 	- info, usage, mount options, design about EXOFS.
 ext2.txt
diff --git a/Documentation/filesystems/efivarfs.txt b/Documentation/filesystems/efivarfs.txt
new file mode 100644
index 0000000..c477af0
--- /dev/null
+++ b/Documentation/filesystems/efivarfs.txt
@@ -0,0 +1,16 @@
+
+efivarfs - a (U)EFI variable filesystem
+
+The efivarfs filesystem was created to address the shortcomings of
+using entries in sysfs to maintain EFI variables. The old sysfs EFI
+variables code only supported variables of up to 1024 bytes. This
+limitation existed in version 0.99 of the EFI specification, but was
+removed before any full releases. Since variables can now be larger
+than a single page, sysfs isn't the best interface for this.
+
+Variables can be created, deleted and modified with the efivarfs
+filesystem.
+
+efivarfs is typically mounted like this,
+
+	mount -t efivarfs none /sys/firmware/efi/efivars
-- 
1.7.11.7

[PATCH 05/20] x86, mm: Include the entire kernel memory map in trampoline_pgd

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

There are various pieces of code in arch/x86 that require a page table
with an identity mapping. Make trampoline_pgd a proper kernel page
table, it currently only includes the kernel text and module space
mapping.

One new feature of trampoline_pgd is that it now has mappings for the
physical I/O device addresses, which are inserted at ioremap()
time. Some broken implementations of EFI firmware require these
mappings to always be around.

Acked-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/mm/init_64.c    |   9 +++-
 arch/x86/mm/ioremap.c    | 105 +++++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/realmode/init.c |  17 +++++++-
 3 files changed, 128 insertions(+), 3 deletions(-)

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 2b6b4a3..fd4404f 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -108,13 +108,13 @@ void sync_global_pgds(unsigned long start, unsigned long end)
 	for (address = start; address <= end; address += PGDIR_SIZE) {
 		const pgd_t *pgd_ref = pgd_offset_k(address);
 		struct page *page;
+		pgd_t *pgd;
 
 		if (pgd_none(*pgd_ref))
 			continue;
 
 		spin_lock(&pgd_lock);
 		list_for_each_entry(page, &pgd_list, lru) {
-			pgd_t *pgd;
 			spinlock_t *pgt_lock;
 
 			pgd = (pgd_t *)page_address(page) + pgd_index(address);
@@ -130,6 +130,13 @@ void sync_global_pgds(unsigned long start, unsigned long end)
 
 			spin_unlock(pgt_lock);
 		}
+
+		pgd = __va(real_mode_header->trampoline_pgd);
+		pgd += pgd_index(address);
+
+		if (pgd_none(*pgd))
+			set_pgd(pgd, *pgd_ref);
+
 		spin_unlock(&pgd_lock);
 	}
 }
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 78fe3f1..e190f7b 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -50,6 +50,107 @@ int ioremap_change_attr(unsigned long vaddr, unsigned long size,
 	return err;
 }
 
+#ifdef CONFIG_X86_64
+static void ident_pte_range(unsigned long paddr, unsigned long vaddr,
+			    pmd_t *ppmd, pmd_t *vpmd, unsigned long end)
+{
+	pte_t *ppte = pte_offset_kernel(ppmd, paddr);
+	pte_t *vpte = pte_offset_kernel(vpmd, vaddr);
+
+	do {
+		set_pte(ppte, *vpte);
+	} while (ppte++, vpte++, vaddr += PAGE_SIZE, vaddr != end);
+}
+
+static int ident_pmd_range(unsigned long paddr, unsigned long vaddr,
+			    pud_t *ppud, pud_t *vpud, unsigned long end)
+{
+	pmd_t *ppmd = pmd_offset(ppud, paddr);
+	pmd_t *vpmd = pmd_offset(vpud, vaddr);
+	unsigned long next;
+
+	do {
+		next = pmd_addr_end(vaddr, end);
+
+		if (!pmd_present(*ppmd)) {
+			pte_t *ppte = (pte_t *)get_zeroed_page(GFP_KERNEL);
+			if (!ppte)
+				return 1;
+
+			set_pmd(ppmd, __pmd(_KERNPG_TABLE | __pa(ppte)));
+		}
+
+		ident_pte_range(paddr, vaddr, ppmd, vpmd, next);
+	} while (ppmd++, vpmd++, vaddr = next, vaddr != end);
+
+	return 0;
+}
+
+static int ident_pud_range(unsigned long paddr, unsigned long vaddr,
+			    pgd_t *ppgd, pgd_t *vpgd, unsigned long end)
+{
+	pud_t *ppud = pud_offset(ppgd, paddr);
+	pud_t *vpud = pud_offset(vpgd, vaddr);
+	unsigned long next;
+
+	do {
+		next = pud_addr_end(vaddr, end);
+
+		if (!pud_present(*ppud)) {
+			pmd_t *ppmd = (pmd_t *)get_zeroed_page(GFP_KERNEL);
+			if (!ppmd)
+				return 1;
+
+			set_pud(ppud, __pud(_KERNPG_TABLE | __pa(ppmd)));
+		}
+
+		if (ident_pmd_range(paddr, vaddr, ppud, vpud, next))
+			return 1;
+	} while (ppud++, vpud++, vaddr = next, vaddr != end);
+
+	return 0;
+}
+
+static int insert_identity_mapping(resource_size_t paddr, unsigned long vaddr,
+				    unsigned long size)
+{
+	unsigned long end = vaddr + size;
+	unsigned long next;
+	pgd_t *vpgd, *ppgd;
+
+	/* Don't map over the guard hole. */
+	if (paddr >= 0x800000000000 || paddr + size > 0x800000000000)
+		return 1;
+
+	ppgd = __va(real_mode_header->trampoline_pgd) + pgd_index(paddr);
+
+	vpgd = pgd_offset_k(vaddr);
+	do {
+		next = pgd_addr_end(vaddr, end);
+
+		if (!pgd_present(*ppgd)) {
+			pud_t *ppud = (pud_t *)get_zeroed_page(GFP_KERNEL);
+			if (!ppud)
+				return 1;
+
+			set_pgd(ppgd, __pgd(_KERNPG_TABLE | __pa(ppud)));
+		}
+
+		if (ident_pud_range(paddr, vaddr, ppgd, vpgd, next))
+			return 1;
+	} while (ppgd++, vpgd++, vaddr = next, vaddr != end);
+
+	return 0;
+}
+#else
+static inline int insert_identity_mapping(resource_size_t paddr,
+					  unsigned long vaddr,
+					  unsigned long size)
+{
+	return 0;
+}
+#endif /* CONFIG_X86_64 */
+
 /*
  * Remap an arbitrary physical address space into the kernel virtual
  * address space. Needed when the kernel wants to access high addresses
@@ -163,6 +264,10 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 	ret_addr = (void __iomem *) (vaddr + offset);
 	mmiotrace_ioremap(unaligned_phys_addr, unaligned_size, ret_addr);
 
+	if (insert_identity_mapping(phys_addr, vaddr, size))
+		printk(KERN_WARNING "ioremap: unable to map 0x%llx in identity pagetable\n",
+					(unsigned long long)phys_addr);
+
 	/*
 	 * Check if the request spans more than any BAR in the iomem resource
 	 * tree.
diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
index cbca565..8e6ab61 100644
--- a/arch/x86/realmode/init.c
+++ b/arch/x86/realmode/init.c
@@ -78,8 +78,21 @@ void __init setup_real_mode(void)
 	*trampoline_cr4_features = read_cr4();
 
 	trampoline_pgd = (u64 *) __va(real_mode_header->trampoline_pgd);
-	trampoline_pgd[0] = __pa(level3_ident_pgt) + _KERNPG_TABLE;
-	trampoline_pgd[511] = __pa(level3_kernel_pgt) + _KERNPG_TABLE;
+
+	/*
+	 * Create an identity mapping for all of physical memory.
+	 */
+	for (i = 0; i <= pgd_index(max_pfn << PAGE_SHIFT); i++) {
+		int index = pgd_index(PAGE_OFFSET) + i;
+
+		trampoline_pgd[i] = (u64)pgd_val(swapper_pg_dir[index]);
+	}
+
+	/*
+	 * Copy the upper-half of the kernel pages tables.
+	 */
+	for (i = pgd_index(PAGE_OFFSET); i < PTRS_PER_PGD; i++)
+		trampoline_pgd[i] = (u64)pgd_val(swapper_pg_dir[i]);
 #endif
 }
 
-- 
1.7.11.7

[PATCH 06/20] x86, efi: 1:1 pagetable mapping for virtual EFI calls

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Some firmware still needs a 1:1 (virt->phys) mapping even after we've
called SetVirtualAddressMap(). So install the mapping alongside our
existing kernel mapping whenever we make EFI calls in virtual mode.

This bug was discovered on ASUS machines where the firmware
implementation of GetTime() accesses the RTC device via physical
addresses, even though that's bogus per the UEFI spec since we've
informed the firmware via SetVirtualAddressMap() that the boottime
memory map is no longer valid.

This bug seems to be present in a lot of consumer devices, so there's
not a lot we can do about this spec violation apart from workaround
it.

Cc: JérômeCarretero <cJ-ko-WRw03QTAyf3sq35pWSNszA@public.gmane.org>
Cc: Vasco Dias <rafa.vasco-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/include/asm/efi.h     | 28 +++++++++++++++++++++-------
 arch/x86/platform/efi/efi_64.c | 15 +++++++++++++++
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index c9dcc18..ae3bf3b 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -69,23 +69,37 @@ extern u64 efi_call6(void *fp, u64 arg1, u64 arg2, u64 arg3,
 	efi_call6((void *)(f), (u64)(a1), (u64)(a2), (u64)(a3),		\
 		  (u64)(a4), (u64)(a5), (u64)(a6))
 
+extern unsigned long efi_call_virt_prelog(void);
+extern void efi_call_virt_epilog(unsigned long);
+
+#define efi_callx(x, func, ...)					\
+	({							\
+		efi_status_t __status;				\
+		unsigned long __pgd;				\
+								\
+		__pgd = efi_call_virt_prelog();			\
+		__status = efi_call##x(func, __VA_ARGS__);	\
+		efi_call_virt_epilog(__pgd);			\
+		__status;					\
+	})
+
 #define efi_call_virt0(f)				\
-	efi_call0((void *)(efi.systab->runtime->f))
+	efi_callx(0, (void *)(efi.systab->runtime->f))
 #define efi_call_virt1(f, a1)					\
-	efi_call1((void *)(efi.systab->runtime->f), (u64)(a1))
+	efi_callx(1, (void *)(efi.systab->runtime->f), (u64)(a1))
 #define efi_call_virt2(f, a1, a2)					\
-	efi_call2((void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2))
+	efi_callx(2, (void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2))
 #define efi_call_virt3(f, a1, a2, a3)					\
-	efi_call3((void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2), \
+	efi_callx(3, (void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2), \
 		  (u64)(a3))
 #define efi_call_virt4(f, a1, a2, a3, a4)				\
-	efi_call4((void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2), \
+	efi_callx(4, (void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2), \
 		  (u64)(a3), (u64)(a4))
 #define efi_call_virt5(f, a1, a2, a3, a4, a5)				\
-	efi_call5((void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2), \
+	efi_callx(5, (void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2), \
 		  (u64)(a3), (u64)(a4), (u64)(a5))
 #define efi_call_virt6(f, a1, a2, a3, a4, a5, a6)			\
-	efi_call6((void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2), \
+	efi_callx(6, (void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2), \
 		  (u64)(a3), (u64)(a4), (u64)(a5), (u64)(a6))
 
 extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size,
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index ac3aa54..ddb0174 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -58,6 +58,21 @@ static void __init early_code_mapping_set_exec(int executable)
 	}
 }
 
+unsigned long efi_call_virt_prelog(void)
+{
+	unsigned long saved;
+
+	saved = read_cr3();
+	write_cr3(real_mode_header->trampoline_pgd);
+
+	return saved;
+}
+
+void efi_call_virt_epilog(unsigned long saved)
+{
+	write_cr3(saved);
+}
+
 void __init efi_call_phys_prelog(void)
 {
 	unsigned long vaddress;
-- 
1.7.11.7

[PATCH 07/20] x86/kernel: remove tboot 1:1 page table creation code

[Matt Fleming]

From: Xiaoyan Zhang <xiaoyan.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

For TXT boot, while Linux kernel trys to shutdown/S3/S4/reboot, it
need to jump back to tboot code and do TXT teardown work. Previously
kernel zapped all mem page identity mapping (va=pa) after booting, so
tboot code mem address was mapped again with identity mapping. Now
kernel didn't zap the identity mapping page table, so tboot related
code can remove the remapping code before trapping back now.

Signed-off-by: Xiaoyan Zhang <xiaoyan.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Acked-by: Gang Wei <gang.wei-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 arch/x86/kernel/tboot.c | 78 ++++---------------------------------------------
 1 file changed, 5 insertions(+), 73 deletions(-)

diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index f84fe00..d4f460f 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -103,71 +103,13 @@ void __init tboot_probe(void)
 	pr_debug("tboot_size: 0x%x\n", tboot->tboot_size);
 }
 
-static pgd_t *tboot_pg_dir;
-static struct mm_struct tboot_mm = {
-	.mm_rb          = RB_ROOT,
-	.pgd            = swapper_pg_dir,
-	.mm_users       = ATOMIC_INIT(2),
-	.mm_count       = ATOMIC_INIT(1),
-	.mmap_sem       = __RWSEM_INITIALIZER(init_mm.mmap_sem),
-	.page_table_lock =  __SPIN_LOCK_UNLOCKED(init_mm.page_table_lock),
-	.mmlist         = LIST_HEAD_INIT(init_mm.mmlist),
-};
-
 static inline void switch_to_tboot_pt(void)
 {
-	write_cr3(virt_to_phys(tboot_pg_dir));
-}
-
-static int map_tboot_page(unsigned long vaddr, unsigned long pfn,
-			  pgprot_t prot)
-{
-	pgd_t *pgd;
-	pud_t *pud;
-	pmd_t *pmd;
-	pte_t *pte;
-
-	pgd = pgd_offset(&tboot_mm, vaddr);
-	pud = pud_alloc(&tboot_mm, pgd, vaddr);
-	if (!pud)
-		return -1;
-	pmd = pmd_alloc(&tboot_mm, pud, vaddr);
-	if (!pmd)
-		return -1;
-	pte = pte_alloc_map(&tboot_mm, NULL, pmd, vaddr);
-	if (!pte)
-		return -1;
-	set_pte_at(&tboot_mm, vaddr, pte, pfn_pte(pfn, prot));
-	pte_unmap(pte);
-	return 0;
-}
-
-static int map_tboot_pages(unsigned long vaddr, unsigned long start_pfn,
-			   unsigned long nr)
-{
-	/* Reuse the original kernel mapping */
-	tboot_pg_dir = pgd_alloc(&tboot_mm);
-	if (!tboot_pg_dir)
-		return -1;
-
-	for (; nr > 0; nr--, vaddr += PAGE_SIZE, start_pfn++) {
-		if (map_tboot_page(vaddr, start_pfn, PAGE_KERNEL_EXEC))
-			return -1;
-	}
-
-	return 0;
-}
-
-static void tboot_create_trampoline(void)
-{
-	u32 map_base, map_size;
-
-	/* Create identity map for tboot shutdown code. */
-	map_base = PFN_DOWN(tboot->tboot_base);
-	map_size = PFN_UP(tboot->tboot_size);
-	if (map_tboot_pages(map_base << PAGE_SHIFT, map_base, map_size))
-		panic("tboot: Error mapping tboot pages (mfns) @ 0x%x, 0x%x\n",
-		      map_base, map_size);
+#ifdef CONFIG_X86_32
+	load_cr3(initial_page_table);
+#else
+	write_cr3(real_mode_header->trampoline_pgd);
+#endif
 }
 
 #ifdef CONFIG_ACPI_SLEEP
@@ -225,14 +167,6 @@ void tboot_shutdown(u32 shutdown_type)
 	if (!tboot_enabled())
 		return;
 
-	/*
-	 * if we're being called before the 1:1 mapping is set up then just
-	 * return and let the normal shutdown happen; this should only be
-	 * due to very early panic()
-	 */
-	if (!tboot_pg_dir)
-		return;
-
 	/* if this is S3 then set regions to MAC */
 	if (shutdown_type == TB_SHUTDOWN_S3)
 		if (tboot_setup_sleep())
@@ -343,8 +277,6 @@ static __init int tboot_late_init(void)
 	if (!tboot_enabled())
 		return 0;
 
-	tboot_create_trampoline();
-
 	atomic_set(&ap_wfs_count, 0);
 	register_hotcpu_notifier(&tboot_cpu_notifier);
 
-- 
1.7.11.7

[PATCH 08/20] x86-64/efi: Use EFI to deal with platform wall clock (again)

[Matt Fleming]

From: Jan Beulich <JBeulich-IBi9RG/b67k@public.gmane.org>

Other than ix86, x86-64 on EFI so far didn't set the
{g,s}et_wallclock accessors to the EFI routines, thus
incorrectly using raw RTC accesses instead.

Simply removing the #ifdef around the respective code isn't
enough, however: While so far early get-time calls were done in
physical mode, this doesn't work properly for x86-64, as virtual
addresses would still need to be set up for all runtime regions
(which wasn't the case on the system I have access to), so
instead the patch moves the call to efi_enter_virtual_mode()
ahead (which in turn allows to drop all code related to calling
efi-get-time in physical mode).

Additionally the earlier calling of efi_set_executable()
requires the CPA code to cope, i.e. during early boot it must be
avoided to call cpa_flush_array(), as the first thing this
function does is a BUG_ON(irqs_disabled()).

Also make the two EFI functions in question here static -
they're not being referenced elsewhere.

History:

    This commit was originally merged as bacef661acdb ("x86-64/efi:
    Use EFI to deal with platform wall clock") but it resulted in some
    ASUS machines no longer booting due to a firmware bug, and so was
    reverted in f026cfa82f62. A pre-emptive fix for the buggy ASUS
    firmware was merged in 03a1c254975e ("x86, efi: 1:1 pagetable
    mapping for virtual EFI calls") so now this patch can be
    reapplied.

Signed-off-by: Jan Beulich <jbeulich-IBi9RG/b67k@public.gmane.org>
Tested-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Acked-by: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Peter Zijlstra <a.p.zijlstra-/NLkJaSkS4VmR6Xm/wNWPw@public.gmane.org>
Cc: H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> [added commit history]
---
 arch/x86/mm/pageattr.c      | 10 ++++++----
 arch/x86/platform/efi/efi.c | 30 ++++--------------------------
 include/linux/efi.h         |  2 --
 init/main.c                 |  8 ++++----
 4 files changed, 14 insertions(+), 36 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index a718e0d..931930a 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -919,11 +919,13 @@ static int change_page_attr_set_clr(unsigned long *addr, int numpages,
 
 	/*
 	 * On success we use clflush, when the CPU supports it to
-	 * avoid the wbindv. If the CPU does not support it and in the
-	 * error case we fall back to cpa_flush_all (which uses
-	 * wbindv):
+	 * avoid the wbindv. If the CPU does not support it, in the
+	 * error case, and during early boot (for EFI) we fall back
+	 * to cpa_flush_all (which uses wbinvd):
 	 */
-	if (!ret && cpu_has_clflush) {
+	if (early_boot_irqs_disabled)
+		__cpa_flush_all((void *)(long)cache);
+	else if (!ret && cpu_has_clflush) {
 		if (cpa.flags & (CPA_PAGES_ARRAY | CPA_ARRAY)) {
 			cpa_flush_array(addr, numpages, cache,
 					cpa.flags, pages);
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index aded2a9..7578344 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -235,22 +235,7 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
 	return status;
 }
 
-static efi_status_t __init phys_efi_get_time(efi_time_t *tm,
-					     efi_time_cap_t *tc)
-{
-	unsigned long flags;
-	efi_status_t status;
-
-	spin_lock_irqsave(&rtc_lock, flags);
-	efi_call_phys_prelog();
-	status = efi_call_phys2(efi_phys.get_time, virt_to_phys(tm),
-				virt_to_phys(tc));
-	efi_call_phys_epilog();
-	spin_unlock_irqrestore(&rtc_lock, flags);
-	return status;
-}
-
-int efi_set_rtc_mmss(unsigned long nowtime)
+static int efi_set_rtc_mmss(unsigned long nowtime)
 {
 	int real_seconds, real_minutes;
 	efi_status_t 	status;
@@ -279,7 +264,7 @@ int efi_set_rtc_mmss(unsigned long nowtime)
 	return 0;
 }
 
-unsigned long efi_get_time(void)
+static unsigned long efi_get_time(void)
 {
 	efi_status_t status;
 	efi_time_t eft;
@@ -635,18 +620,13 @@ static int __init efi_runtime_init(void)
 	}
 	/*
 	 * We will only need *early* access to the following
-	 * two EFI runtime services before set_virtual_address_map
+	 * EFI runtime service before set_virtual_address_map
 	 * is invoked.
 	 */
-	efi_phys.get_time = (efi_get_time_t *)runtime->get_time;
 	efi_phys.set_virtual_address_map =
 		(efi_set_virtual_address_map_t *)
 		runtime->set_virtual_address_map;
-	/*
-	 * Make efi_get_time can be called before entering
-	 * virtual mode.
-	 */
-	efi.get_time = phys_efi_get_time;
+
 	early_iounmap(runtime, sizeof(efi_runtime_services_t));
 
 	return 0;
@@ -734,12 +714,10 @@ void __init efi_init(void)
 		efi_enabled = 0;
 		return;
 	}
-#ifdef CONFIG_X86_32
 	if (efi_native) {
 		x86_platform.get_wallclock = efi_get_time;
 		x86_platform.set_wallclock = efi_set_rtc_mmss;
 	}
-#endif
 
 #if EFI_DEBUG
 	print_efi_memmap();
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 337aefb..5e2308d 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -516,8 +516,6 @@ extern u64 efi_mem_attribute (unsigned long phys_addr, unsigned long size);
 extern int __init efi_uart_console_only (void);
 extern void efi_initialize_iomem_resources(struct resource *code_resource,
 		struct resource *data_resource, struct resource *bss_resource);
-extern unsigned long efi_get_time(void);
-extern int efi_set_rtc_mmss(unsigned long nowtime);
 extern void efi_reserve_boot_services(void);
 extern struct efi_memory_map memmap;
 
diff --git a/init/main.c b/init/main.c
index 9cf77ab..ae70b64 100644
--- a/init/main.c
+++ b/init/main.c
@@ -461,6 +461,10 @@ static void __init mm_init(void)
 	percpu_init_late();
 	pgtable_cache_init();
 	vmalloc_init();
+#ifdef CONFIG_X86
+	if (efi_enabled)
+		efi_enter_virtual_mode();
+#endif
 }
 
 asmlinkage void __init start_kernel(void)
@@ -601,10 +605,6 @@ asmlinkage void __init start_kernel(void)
 	calibrate_delay();
 	pidmap_init();
 	anon_vma_init();
-#ifdef CONFIG_X86
-	if (efi_enabled)
-		efi_enter_virtual_mode();
-#endif
 	thread_info_cache_init();
 	cred_init();
 	fork_init(totalram_pages);
-- 
1.7.11.7

[PATCH 09/20] efivarfs: efivarfs_file_read ensure we free data in error paths

[Matt Fleming]

From: Andy Whitcroft <apw-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

Signed-off-by: Andy Whitcroft <apw-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Acked-by: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 4b12a8fd..ae50d2f 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -766,7 +766,7 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	unsigned long datasize = 0;
 	u32 attributes;
 	void *data;
-	ssize_t size;
+	ssize_t size = 0;
 
 	status = efivars->ops->get_variable(var->var.VariableName,
 					    &var->var.VendorGuid,
@@ -784,13 +784,13 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 					    &var->var.VendorGuid,
 					    &attributes, &datasize,
 					    (data + 4));
-
 	if (status != EFI_SUCCESS)
-		return 0;
+		goto out_free;
 
 	memcpy(data, &attributes, 4);
 	size = simple_read_from_buffer(userbuf, count, ppos,
 					data, datasize + 4);
+out_free:
 	kfree(data);
 
 	return size;
-- 
1.7.11.7

[PATCH 10/20] efivarfs: efivarfs_create() ensure we drop our reference on inode on error

[Matt Fleming]

From: Andy Whitcroft <apw-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

Signed-off-by: Andy Whitcroft <apw-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Acked-by: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index ae50d2f..0bbf742 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -866,7 +866,7 @@ 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 = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
+	struct inode *inode;
 	struct efivars *efivars = &__efivars;
 	struct efivar_entry *var;
 	int namelen, i = 0, err = 0;
@@ -874,13 +874,15 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 	if (dentry->d_name.len < 38)
 		return -EINVAL;
 
+	inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
 	if (!inode)
 		return -ENOSPC;
 
 	var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
-
-	if (!var)
-		return -ENOMEM;
+	if (!var) {
+		err = -ENOMEM;
+		goto out;
+	}
 
 	namelen = dentry->d_name.len - GUID_LEN;
 
@@ -908,8 +910,10 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 	d_instantiate(dentry, inode);
 	dget(dentry);
 out:
-	if (err)
+	if (err) {
 		kfree(var);
+		iput(inode);
+	}
 	return err;
 }
 
-- 
1.7.11.7

[PATCH 11/20] efivarfs: efivarfs_fill_super() fix inode reference counts

[Matt Fleming]

From: Andy Whitcroft <apw-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

When d_make_root() fails it will automatically drop the reference
on the root inode.  We should not be doing so as well.

Signed-off-by: Andy Whitcroft <apw-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Acked-by: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 0bbf742..79f3e4e 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -948,7 +948,6 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 	struct dentry *root;
 	struct efivar_entry *entry, *n;
 	struct efivars *efivars = &__efivars;
-	int err;
 
 	efivarfs_sb = sb;
 
@@ -960,18 +959,14 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_time_gran         = 1;
 
 	inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0);
-	if (!inode) {
-		err = -ENOMEM;
-		goto fail;
-	}
+	if (!inode)
+		return -ENOMEM;
 	inode->i_op = &efivarfs_dir_inode_operations;
 
 	root = d_make_root(inode);
 	sb->s_root = root;
-	if (!root) {
-		err = -ENOMEM;
-		goto fail;
-	}
+	if (!root)
+		return -ENOMEM;
 
 	list_for_each_entry_safe(entry, n, &efivars->list, list) {
 		struct inode *inode;
@@ -1012,9 +1007,6 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	return 0;
-fail:
-	iput(inode);
-	return err;
 }
 
 static struct dentry *efivarfs_mount(struct file_system_type *fs_type,
-- 
1.7.11.7

[PATCH 12/20] efivarfs: efivarfs_fill_super() ensure we free our temporary name

[Matt Fleming]

From: Andy Whitcroft <apw-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

d_alloc_name() copies the passed name to new storage, once complete we
no longer need our name.

Signed-off-by: Andy Whitcroft <apw-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Acked-by: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 79f3e4e..dcb34ae 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -992,6 +992,8 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 		inode = efivarfs_get_inode(efivarfs_sb, root->d_inode,
 					  S_IFREG | 0644, 0);
 		dentry = d_alloc_name(root, name);
+		/* copied by the above to local storage in the dentry. */
+		kfree(name);
 
 		efivars->ops->get_variable(entry->var.VariableName,
 					   &entry->var.VendorGuid,
-- 
1.7.11.7

[PATCH 13/20] efivarfs: efivarfs_fill_super() ensure we clean up correctly on error

[Matt Fleming]

From: Andy Whitcroft <apw-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

Ensure we free both the name and inode on error when building the
individual variables.

Signed-off-by: Andy Whitcroft <apw-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Acked-by: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index dcb34ae..2d67781 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -948,6 +948,7 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 	struct dentry *root;
 	struct efivar_entry *entry, *n;
 	struct efivars *efivars = &__efivars;
+	char *name;
 
 	efivarfs_sb = sb;
 
@@ -969,16 +970,18 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 		return -ENOMEM;
 
 	list_for_each_entry_safe(entry, n, &efivars->list, list) {
-		struct inode *inode;
 		struct dentry *dentry, *root = efivarfs_sb->s_root;
-		char *name;
 		unsigned long size = 0;
 		int len, i;
 
+		inode = NULL;
+
 		len = utf16_strlen(entry->var.VariableName);
 
 		/* GUID plus trailing NULL */
 		name = kmalloc(len + 38, GFP_ATOMIC);
+		if (!name)
+			goto fail;
 
 		for (i = 0; i < len; i++)
 			name[i] = entry->var.VariableName[i] & 0xFF;
@@ -991,7 +994,13 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 
 		inode = efivarfs_get_inode(efivarfs_sb, root->d_inode,
 					  S_IFREG | 0644, 0);
+		if (!inode)
+			goto fail_name;
+
 		dentry = d_alloc_name(root, name);
+		if (!dentry)
+			goto fail_inode;
+
 		/* copied by the above to local storage in the dentry. */
 		kfree(name);
 
@@ -1009,6 +1018,13 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	return 0;
+
+fail_inode:
+	iput(inode);
+fail_name:
+	kfree(name);
+fail:
+	return -ENOMEM;
 }
 
 static struct dentry *efivarfs_mount(struct file_system_type *fs_type,
-- 
1.7.11.7

[PATCH 14/20] efivarfs: Implement exclusive access for {get,set}_variable

[Matt Fleming]

From: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

Currently, efivarfs does not enforce exclusion over the get_variable and
set_variable operations. Section 7.1 of UEFI requires us to only allow a
single processor to enter {get,set}_variable services at once.

This change acquires the efivars->lock over calls to these operations
from the efivarfs paths.

Signed-off-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 68 +++++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 25 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 2d67781..03e0df2 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -690,35 +690,45 @@ static ssize_t efivarfs_file_write(struct file *file,
 		goto out;
 	}
 
+	/*
+	 * The lock here protects the get_variable call, the conditional
+	 * set_variable call, and removal of the variable from the efivars
+	 * list (in the case of an authenticated delete).
+	 */
+	spin_lock(&efivars->lock);
+
 	status = efivars->ops->set_variable(var->var.VariableName,
 					    &var->var.VendorGuid,
 					    attributes, datasize,
 					    data);
 
-	switch (status) {
-	case EFI_SUCCESS:
-		break;
-	case EFI_INVALID_PARAMETER:
-		count = -EINVAL;
-		goto out;
-	case EFI_OUT_OF_RESOURCES:
-		count = -ENOSPC;
-		goto out;
-	case EFI_DEVICE_ERROR:
-		count = -EIO;
-		goto out;
-	case EFI_WRITE_PROTECTED:
-		count = -EROFS;
-		goto out;
-	case EFI_SECURITY_VIOLATION:
-		count = -EACCES;
-		goto out;
-	case EFI_NOT_FOUND:
-		count = -ENOENT;
-		goto out;
-	default:
-		count = -EINVAL;
-		goto out;
+	if (status != EFI_SUCCESS) {
+		spin_unlock(&efivars->lock);
+		kfree(data);
+
+		switch (status) {
+		case EFI_INVALID_PARAMETER:
+			count = -EINVAL;
+			break;
+		case EFI_OUT_OF_RESOURCES:
+			count = -ENOSPC;
+			break;
+		case EFI_DEVICE_ERROR:
+			count = -EIO;
+			break;
+		case EFI_WRITE_PROTECTED:
+			count = -EROFS;
+			break;
+		case EFI_SECURITY_VIOLATION:
+			count = -EACCES;
+			break;
+		case EFI_NOT_FOUND:
+			count = -ENOENT;
+			break;
+		default:
+			count = -EINVAL;
+		}
+		return count;
 	}
 
 	/*
@@ -734,12 +744,12 @@ static ssize_t efivarfs_file_write(struct file *file,
 					    NULL);
 
 	if (status == EFI_BUFFER_TOO_SMALL) {
+		spin_unlock(&efivars->lock);
 		mutex_lock(&inode->i_mutex);
 		i_size_write(inode, newdatasize + sizeof(attributes));
 		mutex_unlock(&inode->i_mutex);
 
 	} else if (status == EFI_NOT_FOUND) {
-		spin_lock(&efivars->lock);
 		list_del(&var->list);
 		spin_unlock(&efivars->lock);
 		efivar_unregister(var);
@@ -747,6 +757,7 @@ static ssize_t efivarfs_file_write(struct file *file,
 		dput(file->f_dentry);
 
 	} else {
+		spin_unlock(&efivars->lock);
 		pr_warn("efivarfs: inconsistent EFI variable implementation? "
 				"status = %lx\n", status);
 	}
@@ -768,9 +779,11 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	void *data;
 	ssize_t size = 0;
 
+	spin_lock(&efivars->lock);
 	status = efivars->ops->get_variable(var->var.VariableName,
 					    &var->var.VendorGuid,
 					    &attributes, &datasize, NULL);
+	spin_unlock(&efivars->lock);
 
 	if (status != EFI_BUFFER_TOO_SMALL)
 		return 0;
@@ -780,10 +793,13 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	if (!data)
 		return 0;
 
+	spin_lock(&efivars->lock);
 	status = efivars->ops->get_variable(var->var.VariableName,
 					    &var->var.VendorGuid,
 					    &attributes, &datasize,
 					    (data + 4));
+	spin_unlock(&efivars->lock);
+
 	if (status != EFI_SUCCESS)
 		goto out_free;
 
@@ -1004,11 +1020,13 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 		/* copied by the above to local storage in the dentry. */
 		kfree(name);
 
+		spin_lock(&efivars->lock);
 		efivars->ops->get_variable(entry->var.VariableName,
 					   &entry->var.VendorGuid,
 					   &entry->var.Attributes,
 					   &size,
 					   NULL);
+		spin_unlock(&efivars->lock);
 
 		mutex_lock(&inode->i_mutex);
 		inode->i_private = entry;
-- 
1.7.11.7

[PATCH 15/20] efi: Clarify GUID length calculations

[Matt Fleming]

From: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

At present, the handling of GUIDs in efivar file names isn't consistent.
We use GUID_LEN in some places, and 38 in others (GUID_LEN plus
separator), and implicitly use the presence of the trailing NUL.

This change removes the trailing NUL from GUID_LEN, so that we're
explicitly adding it when required. We also replace magic numbers
with GUID_LEN, and clarify the comments where appropriate.

We also fix the allocation size in efivar_create_sysfs_entry, where
we're allocating one byte too much, due to counting the trailing NUL
twice - once when calculating short_name_size, and once in the kzalloc.

Signed-off-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 03e0df2..c532854 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -95,7 +95,12 @@ MODULE_LICENSE("GPL");
 MODULE_VERSION(EFIVARS_VERSION);
 
 #define DUMP_NAME_LEN 52
-#define GUID_LEN 37
+
+/*
+ * Length of a GUID string (strlen("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"))
+ * not including trailing NUL
+ */
+#define GUID_LEN 36
 
 /*
  * The maximum size of VariableName + Data = 1024
@@ -887,7 +892,11 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 	struct efivar_entry *var;
 	int namelen, i = 0, err = 0;
 
-	if (dentry->d_name.len < 38)
+	/*
+	 * We need a GUID, plus at least one letter for the variable name,
+	 * plus the '-' separator
+	 */
+	if (dentry->d_name.len < GUID_LEN + 2)
 		return -EINVAL;
 
 	inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
@@ -900,7 +909,8 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 		goto out;
 	}
 
-	namelen = dentry->d_name.len - GUID_LEN;
+	/* length of the variable name itself: remove GUID and separator */
+	namelen = dentry->d_name.len - GUID_LEN - 1;
 
 	efivarfs_hex_to_guid(dentry->d_name.name + namelen + 1,
 			&var->var.VendorGuid);
@@ -994,8 +1004,8 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 
 		len = utf16_strlen(entry->var.VariableName);
 
-		/* GUID plus trailing NULL */
-		name = kmalloc(len + 38, GFP_ATOMIC);
+		/* name, plus '-', plus GUID, plus NUL*/
+		name = kmalloc(len + 1 + GUID_LEN + 1, GFP_ATOMIC);
 		if (!name)
 			goto fail;
 
@@ -1006,7 +1016,7 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 
 		efi_guid_unparse(&entry->var.VendorGuid, name + len + 1);
 
-		name[len+GUID_LEN] = '\0';
+		name[len+GUID_LEN+1] = '\0';
 
 		inode = efivarfs_get_inode(efivarfs_sb, root->d_inode,
 					  S_IFREG | 0644, 0);
@@ -1435,11 +1445,18 @@ efivar_create_sysfs_entry(struct efivars *efivars,
 			  efi_char16_t *variable_name,
 			  efi_guid_t *vendor_guid)
 {
-	int i, short_name_size = variable_name_size / sizeof(efi_char16_t) + 38;
+	int i, short_name_size;
 	char *short_name;
 	struct efivar_entry *new_efivar;
 
-	short_name = kzalloc(short_name_size + 1, GFP_KERNEL);
+	/*
+	 * Length of the variable bytes in ASCII, plus the '-' separator,
+	 * plus the GUID, plus trailing NUL
+	 */
+	short_name_size = variable_name_size / sizeof(efi_char16_t)
+				+ 1 + GUID_LEN + 1;
+
+	short_name = kzalloc(short_name_size, GFP_KERNEL);
 	new_efivar = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
 
 	if (!short_name || !new_efivar)  {
-- 
1.7.11.7

[PATCH 16/20] efivarfs: Return an error if we fail to read a variable

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Instead of always returning 0 in efivarfs_file_read(), even when we
fail to successfully read the variable, convert the EFI status to
something meaningful and return that to the caller. This way the user
will have some hint as to why the read failed.

Acked-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 58 +++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index c532854..3c31215 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -653,6 +653,36 @@ static int efivarfs_file_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int efi_status_to_err(efi_status_t status)
+{
+	int err;
+
+	switch (status) {
+	case EFI_INVALID_PARAMETER:
+		err = -EINVAL;
+		break;
+	case EFI_OUT_OF_RESOURCES:
+		err = -ENOSPC;
+		break;
+	case EFI_DEVICE_ERROR:
+		err = -EIO;
+		break;
+	case EFI_WRITE_PROTECTED:
+		err = -EROFS;
+		break;
+	case EFI_SECURITY_VIOLATION:
+		err = -EACCES;
+		break;
+	case EFI_NOT_FOUND:
+		err = -ENOENT;
+		break;
+	default:
+		err = -EINVAL;
+	}
+
+	return err;
+}
+
 static ssize_t efivarfs_file_write(struct file *file,
 		const char __user *userbuf, size_t count, loff_t *ppos)
 {
@@ -711,29 +741,7 @@ static ssize_t efivarfs_file_write(struct file *file,
 		spin_unlock(&efivars->lock);
 		kfree(data);
 
-		switch (status) {
-		case EFI_INVALID_PARAMETER:
-			count = -EINVAL;
-			break;
-		case EFI_OUT_OF_RESOURCES:
-			count = -ENOSPC;
-			break;
-		case EFI_DEVICE_ERROR:
-			count = -EIO;
-			break;
-		case EFI_WRITE_PROTECTED:
-			count = -EROFS;
-			break;
-		case EFI_SECURITY_VIOLATION:
-			count = -EACCES;
-			break;
-		case EFI_NOT_FOUND:
-			count = -ENOENT;
-			break;
-		default:
-			count = -EINVAL;
-		}
-		return count;
+		return efi_status_to_err(status);
 	}
 
 	/*
@@ -805,8 +813,10 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 					    (data + 4));
 	spin_unlock(&efivars->lock);
 
-	if (status != EFI_SUCCESS)
+	if (status != EFI_SUCCESS) {
+		size = efi_status_to_err(status);
 		goto out_free;
+	}
 
 	memcpy(data, &attributes, 4);
 	size = simple_read_from_buffer(userbuf, count, ppos,
-- 
1.7.11.7

[PATCH 17/20] efivarfs: Replace magic number with sizeof(attributes)

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Seeing "+ 4" littered throughout the functions gets a bit
confusing. Use "sizeof(attributes)" which clearly explains what
quantity we're adding.

Acked-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 3c31215..a6cec2f 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -801,7 +801,7 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	if (status != EFI_BUFFER_TOO_SMALL)
 		return 0;
 
-	data = kmalloc(datasize + 4, GFP_KERNEL);
+	data = kmalloc(datasize + sizeof(attributes), GFP_KERNEL);
 
 	if (!data)
 		return 0;
@@ -810,7 +810,7 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	status = efivars->ops->get_variable(var->var.VariableName,
 					    &var->var.VendorGuid,
 					    &attributes, &datasize,
-					    (data + 4));
+					    (data + sizeof(attributes)));
 	spin_unlock(&efivars->lock);
 
 	if (status != EFI_SUCCESS) {
@@ -818,9 +818,9 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 		goto out_free;
 	}
 
-	memcpy(data, &attributes, 4);
+	memcpy(data, &attributes, sizeof(attributes));
 	size = simple_read_from_buffer(userbuf, count, ppos,
-					data, datasize + 4);
+				       data, datasize + sizeof(attributes));
 out_free:
 	kfree(data);
 
-- 
1.7.11.7

[PATCH 18/20] efivarfs: Add unique magic number

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Using pstore's superblock magic number is no doubt going to cause
problems in the future. Give efivarfs its own magic number.

Acked-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 2 +-
 include/uapi/linux/magic.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index a6cec2f..057a9c8 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -991,7 +991,7 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_maxbytes          = MAX_LFS_FILESIZE;
 	sb->s_blocksize         = PAGE_CACHE_SIZE;
 	sb->s_blocksize_bits    = PAGE_CACHE_SHIFT;
-	sb->s_magic             = PSTOREFS_MAGIC;
+	sb->s_magic             = EFIVARFS_MAGIC;
 	sb->s_op                = &efivarfs_ops;
 	sb->s_time_gran         = 1;
 
diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h
index e15192c..12f68c7 100644
--- a/include/uapi/linux/magic.h
+++ b/include/uapi/linux/magic.h
@@ -27,6 +27,7 @@
 #define ISOFS_SUPER_MAGIC	0x9660
 #define JFFS2_SUPER_MAGIC	0x72b6
 #define PSTOREFS_MAGIC		0x6165676C
+#define EFIVARFS_MAGIC		0xde5e81e4
 
 #define MINIX_SUPER_MAGIC	0x137F		/* minix v1 fs, 14 char names */
 #define MINIX_SUPER_MAGIC2	0x138F		/* minix v1 fs, 30 char names */
-- 
1.7.11.7

[PATCH 19/20] efivarfs: Make 'datasize' unsigned long

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

There's no reason to declare 'datasize' as an int, since the majority
of the functions it's passed to expect an unsigned long anyway. Plus,
this way we avoid any sign problems during arithmetic.

Acked-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 057a9c8..5e4d120 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -692,7 +692,7 @@ static ssize_t efivarfs_file_write(struct file *file,
 	void *data;
 	u32 attributes;
 	struct inode *inode = file->f_mapping->host;
-	int datasize = count - sizeof(attributes);
+	unsigned long datasize = count - sizeof(attributes);
 	unsigned long newdatasize;
 
 	if (count < sizeof(attributes))
-- 
1.7.11.7

[PATCH 20/20] efivarfs: Return a consistent error when efivarfs_get_inode() fails

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Instead of returning -ENOSPC if efivarfs_get_inode() fails we should
be returning -ENOMEM, since running out of memory is the only reason
it can fail.  Furthermore, that's the error value used everywhere else
in this file. It's also less likely to confuse users that hit this
error case.

Acked-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 5e4d120..73f22d1 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -911,7 +911,7 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
 
 	inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
 	if (!inode)
-		return -ENOSPC;
+		return -ENOMEM;
 
 	var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
 	if (!var) {
-- 
1.7.11.7

Re: [PATCH 01/20] efi: Add support for a UEFI variable filesystem

[Alan Cox]

> +static ssize_t efivarfs_file_write(struct file *file,
> +		const char __user *userbuf, size_t count, loff_t *ppos)
> +{
> +	struct efivar_entry *var = file->private_data;
> +	struct efivars *efivars;
> +	efi_status_t status;
> +	void *data;
> +	u32 attributes;
> +	struct inode *inode = file->f_mapping->host;
> +	int datasize = count - sizeof(attributes);

long ?

> +
> +	if (count < sizeof(attributes))
> +		return -EINVAL;
> +
> +	data = kmalloc(datasize, GFP_KERNEL);

This allows anyone who can open the file to allocate enormous amounts of
memory ... there should probably be a sanity check size here ?

> +	if (!data)
> +		return -ENOMEM;
> +
> +	efivars = var->efivars;
> +
> +	if (copy_from_user(&attributes, userbuf, sizeof(attributes))) {
> +		count = -EFAULT;

Nope.. size_t is not necessarily signed. ssize_t is. This probably
happens to do the right thing but it's not really right.

> +	switch (status) {
> +	case EFI_SUCCESS:

Would it make sense for EFI to have a generic 'efi_to_errno' ?

> +static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
> +		size_t count, loff_t *ppos)
> +{
> +	struct efivar_entry *var = file->private_data;
> +	struct efivars *efivars = var->efivars;
> +	efi_status_t status;
> +	unsigned long datasize = 0;
> +	u32 attributes;
> +	void *data;
> +	ssize_t size;
> +
> +	status = efivars->ops->get_variable(var->var.VariableName,
> +					    &var->var.VendorGuid,
> +					    &attributes, &datasize, NULL);
> +
> +	if (status != EFI_BUFFER_TOO_SMALL)
> +		return 0;

This seems odd - the writer translates error codes this doesnt.
> +
> +	data = kmalloc(datasize + 4, GFP_KERNEL);

Are you sure the firmware will never hand back insane datasize values ?

> +static const struct file_operations efivarfs_file_operations = {
> +	.open	= efivarfs_file_open,
> +	.read	= efivarfs_file_read,
> +	.write	= efivarfs_file_write,
> +	.llseek	= default_llseek,

But you don't implement llseek in your read/write methods ?

> +		/* GUID plus trailing NULL */
> +		name = kmalloc(len + 38, GFP_ATOMIC);
> +
> +		for (i = 0; i < len; i++)
> +			name[i] = entry->var.VariableName[i] & 0xFF;

Unchecked kmalloc - and an atomic one at that.

> +		dentry = d_alloc_name(root, name);

...

Re: [PATCH 03/20] efi: add efivars kobject to efi sysfs folder

[Alan Cox]

On Fri, 26 Oct 2012 08:51:46 +0100
Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:

> From: "Lee, Chun-Yi" <joeyli.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> UEFI variable filesystem need a new mount point, so this patch add
> efivars kobject to efi_kobj for create a /sys/firmware/efi/efivars
> folder.
> 
> Cc: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
> Signed-off-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efivars.c | 11 +++++++++++
>  include/linux/efi.h        |  1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 6d43bbd..4b12a8fd 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -1527,6 +1527,7 @@ void unregister_efivars(struct efivars *efivars)
>  		sysfs_remove_bin_file(&efivars->kset->kobj, efivars->del_var);
>  	kfree(efivars->new_var);
>  	kfree(efivars->del_var);
> +	kobject_put(efivars->kobject);

This makes no sense - you already always unregister the kset in
register_efivars ?

Re: [PATCH 01/20] efi: Add support for a UEFI variable filesystem

[Matt Fleming]

On Fri, 2012-10-26 at 11:10 +0100, Alan Cox wrote:
> > +static ssize_t efivarfs_file_write(struct file *file,
> > +		const char __user *userbuf, size_t count, loff_t *ppos)
> > +{
> > +	struct efivar_entry *var = file->private_data;
> > +	struct efivars *efivars;
> > +	efi_status_t status;
> > +	void *data;
> > +	u32 attributes;
> > +	struct inode *inode = file->f_mapping->host;
> > +	int datasize = count - sizeof(attributes);
> 
> long ?

unsigned long, but yes. This is corrected in [PATCH 19/20].

> > +
> > +	if (count < sizeof(attributes))
> > +		return -EINVAL;
> > +
> > +	data = kmalloc(datasize, GFP_KERNEL);
> 
> This allows anyone who can open the file to allocate enormous amounts of
> memory ... there should probably be a sanity check size here ?

Yeah probably. I'm not sure what a good upper limit for the allocation
would be though. Anyone got any ideas?

> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	efivars = var->efivars;
> > +
> > +	if (copy_from_user(&attributes, userbuf, sizeof(attributes))) {
> > +		count = -EFAULT;
> 
> Nope.. size_t is not necessarily signed. ssize_t is. This probably
> happens to do the right thing but it's not really right.

Good catch. Needs fixing.

> > +	switch (status) {
> > +	case EFI_SUCCESS:
> 
> Would it make sense for EFI to have a generic 'efi_to_errno' ?

Yep, that function appears in [PATCH 16/20] and gets used here.

> > +static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
> > +		size_t count, loff_t *ppos)
> > +{
> > +	struct efivar_entry *var = file->private_data;
> > +	struct efivars *efivars = var->efivars;
> > +	efi_status_t status;
> > +	unsigned long datasize = 0;
> > +	u32 attributes;
> > +	void *data;
> > +	ssize_t size;
> > +
> > +	status = efivars->ops->get_variable(var->var.VariableName,
> > +					    &var->var.VendorGuid,
> > +					    &attributes, &datasize, NULL);
> > +
> > +	if (status != EFI_BUFFER_TOO_SMALL)
> > +		return 0;
> 
> This seems odd - the writer translates error codes this doesnt.

Bah, I missed fixing this in [PATCH 16/20]. Thanks.

> > +
> > +	data = kmalloc(datasize + 4, GFP_KERNEL);
> 
> Are you sure the firmware will never hand back insane datasize values ?

Possibly yes, but it's difficult to know what is a "sane" value. See the
question above about upper limits for allocations.

> > +static const struct file_operations efivarfs_file_operations = {
> > +	.open	= efivarfs_file_open,
> > +	.read	= efivarfs_file_read,
> > +	.write	= efivarfs_file_write,
> > +	.llseek	= default_llseek,
> 
> But you don't implement llseek in your read/write methods ?

Jeremy? Do you want to take this one?

> > +		/* GUID plus trailing NULL */
> > +		name = kmalloc(len + 38, GFP_ATOMIC);
> > +
> > +		for (i = 0; i < len; i++)
> > +			name[i] = entry->var.VariableName[i] & 0xFF;
> 
> Unchecked kmalloc - and an atomic one at that.

Fixed in [PATCH 13/20].

> > +		dentry = d_alloc_name(root, name);
> 

Fixed in [PATCH 13/20].

Thanks for the review, Alan!

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 03/20] efi: add efivars kobject to efi sysfs folder

[Matt Fleming]

On Fri, 2012-10-26 at 11:13 +0100, Alan Cox wrote:
> On Fri, 26 Oct 2012 08:51:46 +0100
> Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
> 
> > From: "Lee, Chun-Yi" <joeyli.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 
> > UEFI variable filesystem need a new mount point, so this patch add
> > efivars kobject to efi_kobj for create a /sys/firmware/efi/efivars
> > folder.
> > 
> > Cc: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
> > Signed-off-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> > Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/firmware/efivars.c | 11 +++++++++++
> >  include/linux/efi.h        |  1 +
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> > index 6d43bbd..4b12a8fd 100644
> > --- a/drivers/firmware/efivars.c
> > +++ b/drivers/firmware/efivars.c
> > @@ -1527,6 +1527,7 @@ void unregister_efivars(struct efivars *efivars)
> >  		sysfs_remove_bin_file(&efivars->kset->kobj, efivars->del_var);
> >  	kfree(efivars->new_var);
> >  	kfree(efivars->del_var);
> > +	kobject_put(efivars->kobject);
> 
> This makes no sense - you already always unregister the kset in
> register_efivars ?

Crap. Yes, you're completely right. Thanks Alan.

Joey, this chunk of your patch is incorrect,

@@ -1558,6 +1559,13 @@ int register_efivars(struct efivars *efivars,
 		goto out;
 	}
 
+	efivars->kobject = kobject_create_and_add("efivars", parent_kobj);
+	if (!efivars->kobject) {
+		pr_err("efivars: Subsystem registration failed.\n");
+		error = -ENOMEM;
+		goto err_unreg_vars;
+	}
+
 	/*
 	 * Per EFI spec, the maximum storage allocated for both
 	 * the variable name and variable data is 1024 bytes.
@@ -1602,6 +1610,9 @@ int register_efivars(struct efivars *efivars,
 
 	register_filesystem(&efivarfs_type);
 
+err_unreg_vars:
+	kset_unregister(efivars->kset);
+
 out:
 	kfree(variable_name);
 

because err_unreg_vars is in the *success* path, not the error path. So
even if we successfully register efivarfs, we still call
kset_unregister().

Could you please fix this and resubmit?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 03/20] efi: add efivars kobject to efi sysfs folder

[joeyli]

於 五，2012-10-26 於 12:13 +0100，Matt Fleming 提到：
> On Fri, 2012-10-26 at 11:13 +0100, Alan Cox wrote:
> > On Fri, 26 Oct 2012 08:51:46 +0100
> > Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org> wrote:
> > 
> > > From: "Lee, Chun-Yi" <joeyli.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > 
> > > UEFI variable filesystem need a new mount point, so this patch add
> > > efivars kobject to efi_kobj for create a /sys/firmware/efi/efivars
> > > folder.
> > > 
> > > Cc: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > Cc: H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
> > > Signed-off-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> > > Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  drivers/firmware/efivars.c | 11 +++++++++++
> > >  include/linux/efi.h        |  1 +
> > >  2 files changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> > > index 6d43bbd..4b12a8fd 100644
> > > --- a/drivers/firmware/efivars.c
> > > +++ b/drivers/firmware/efivars.c
> > > @@ -1527,6 +1527,7 @@ void unregister_efivars(struct efivars *efivars)
> > >  		sysfs_remove_bin_file(&efivars->kset->kobj, efivars->del_var);
> > >  	kfree(efivars->new_var);
> > >  	kfree(efivars->del_var);
> > > +	kobject_put(efivars->kobject);
> > 
> > This makes no sense - you already always unregister the kset in
> > register_efivars ?
> 
> Crap. Yes, you're completely right. Thanks Alan.
> 
> Joey, this chunk of your patch is incorrect,
> 
> @@ -1558,6 +1559,13 @@ int register_efivars(struct efivars *efivars,
>  		goto out;
>  	}
>  
> +	efivars->kobject = kobject_create_and_add("efivars", parent_kobj);
> +	if (!efivars->kobject) {
> +		pr_err("efivars: Subsystem registration failed.\n");
> +		error = -ENOMEM;
> +		goto err_unreg_vars;
> +	}
> +
>  	/*
>  	 * Per EFI spec, the maximum storage allocated for both
>  	 * the variable name and variable data is 1024 bytes.
> @@ -1602,6 +1610,9 @@ int register_efivars(struct efivars *efivars,
>  
>  	register_filesystem(&efivarfs_type);
>  
> +err_unreg_vars:
> +	kset_unregister(efivars->kset);
> +
>  out:
>  	kfree(variable_name);
>  
> 
> because err_unreg_vars is in the *success* path, not the error path. So
> even if we successfully register efivarfs, we still call
> kset_unregister().
> 
> Could you please fix this and resubmit?
> 

Sorry for my mistake and thanks for Alan's review.

I just sent the following patch to fix the problem.


Thanks
Joey Lee


>From 49f036dd4e5e702a344f115177a58952c8aa5b0d Mon Sep 17 00:00:00 2001
From: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Date: Mon, 29 Oct 2012 13:42:33 +0800
Subject: [PATCH] efi: fix always unregister kset of efivars in register_efivars

The cc9c97b76aa7235d5a1de0f74f990097f77008b4 patch introduced a
issue that always unregister kset of efivars in register_efivars.

This patch moved the kset_unregister function call to the kobject create check
block for fix issue.

Cc: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
Cc: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Cc: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
---
 drivers/firmware/efivars.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 03e0df2..8870907 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1595,7 +1595,8 @@ int register_efivars(struct efivars *efivars,
 	if (!efivars->kobject) {
 		pr_err("efivars: Subsystem registration failed.\n");
 		error = -ENOMEM;
-		goto err_unreg_vars;
+		kset_unregister(efivars->kset);
+		goto out;
 	}
 
 	/*
@@ -1642,9 +1643,6 @@ int register_efivars(struct efivars *efivars,
 
 	register_filesystem(&efivarfs_type);
 
-err_unreg_vars:
-	kset_unregister(efivars->kset);
-
 out:
 	kfree(variable_name);
 
-- 
1.6.0.2

[PATCH v2 01/20] efi: Add support for a UEFI variable filesystem

[Matt Fleming]

From: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

The existing EFI variables code only supports variables of up to 1024
bytes. This limitation existed in version 0.99 of the EFI specification,
but was removed before any full releases. Since variables can now be
larger than a single page, sysfs isn't the best interface for this. So,
instead, let's add a filesystem. Variables can be read, written and
created, with the first 4 bytes of each variable representing its UEFI
attributes. The create() method doesn't actually commit to flash since
zero-length variables can't exist per-spec.

Updates from Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>.

Signed-off-by: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---

v2: Address some comments from Alan Cox regarding llseek behaviour by
    explicitly not implementing llseek instead of providing half of an
    implementation.

 drivers/firmware/efivars.c | 384 ++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/efi.h        |   5 +
 2 files changed, 383 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index d10c987..b605c48 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -80,6 +80,10 @@
 #include <linux/slab.h>
 #include <linux/pstore.h>
 
+#include <linux/fs.h>
+#include <linux/ramfs.h>
+#include <linux/pagemap.h>
+
 #include <asm/uaccess.h>
 
 #define EFIVARS_VERSION "0.08"
@@ -91,6 +95,7 @@ MODULE_LICENSE("GPL");
 MODULE_VERSION(EFIVARS_VERSION);
 
 #define DUMP_NAME_LEN 52
+#define GUID_LEN 37
 
 /*
  * The maximum size of VariableName + Data = 1024
@@ -108,7 +113,6 @@ struct efi_variable {
 	__u32         Attributes;
 } __attribute__((packed));
 
-
 struct efivar_entry {
 	struct efivars *efivars;
 	struct efi_variable var;
@@ -122,6 +126,9 @@ struct efivar_attribute {
 	ssize_t (*store)(struct efivar_entry *entry, const char *buf, size_t count);
 };
 
+static struct efivars __efivars;
+static struct efivar_operations ops;
+
 #define PSTORE_EFI_ATTRIBUTES \
 	(EFI_VARIABLE_NON_VOLATILE | \
 	 EFI_VARIABLE_BOOTSERVICE_ACCESS | \
@@ -629,14 +636,380 @@ static struct kobj_type efivar_ktype = {
 	.default_attrs = def_attrs,
 };
 
-static struct pstore_info efi_pstore_info;
-
 static inline void
 efivar_unregister(struct efivar_entry *var)
 {
 	kobject_put(&var->kobj);
 }
 
+static int efivarfs_file_open(struct inode *inode, struct file *file)
+{
+	file->private_data = inode->i_private;
+	return 0;
+}
+
+static ssize_t efivarfs_file_write(struct file *file,
+		const char __user *userbuf, size_t count, loff_t *ppos)
+{
+	struct efivar_entry *var = file->private_data;
+	struct efivars *efivars;
+	efi_status_t status;
+	void *data;
+	u32 attributes;
+	struct inode *inode = file->f_mapping->host;
+	int datasize = count - sizeof(attributes);
+
+	if (count < sizeof(attributes))
+		return -EINVAL;
+
+	data = kmalloc(datasize, GFP_KERNEL);
+
+	if (!data)
+		return -ENOMEM;
+
+	efivars = var->efivars;
+
+	if (copy_from_user(&attributes, userbuf, sizeof(attributes))) {
+		count = -EFAULT;
+		goto out;
+	}
+
+	if (attributes & ~(EFI_VARIABLE_MASK)) {
+		count = -EINVAL;
+		goto out;
+	}
+
+	if (copy_from_user(data, userbuf + sizeof(attributes), datasize)) {
+		count = -EFAULT;
+		goto out;
+	}
+
+	if (validate_var(&var->var, data, datasize) == false) {
+		count = -EINVAL;
+		goto out;
+	}
+
+	status = efivars->ops->set_variable(var->var.VariableName,
+					    &var->var.VendorGuid,
+					    attributes, datasize,
+					    data);
+
+	switch (status) {
+	case EFI_SUCCESS:
+		mutex_lock(&inode->i_mutex);
+		i_size_write(inode, count);
+		mutex_unlock(&inode->i_mutex);
+		break;
+	case EFI_INVALID_PARAMETER:
+		count = -EINVAL;
+		break;
+	case EFI_OUT_OF_RESOURCES:
+		count = -ENOSPC;
+		break;
+	case EFI_DEVICE_ERROR:
+		count = -EIO;
+		break;
+	case EFI_WRITE_PROTECTED:
+		count = -EROFS;
+		break;
+	case EFI_SECURITY_VIOLATION:
+		count = -EACCES;
+		break;
+	case EFI_NOT_FOUND:
+		count = -ENOENT;
+		break;
+	default:
+		count = -EINVAL;
+		break;
+	}
+out:
+	kfree(data);
+
+	return count;
+}
+
+static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
+		size_t count, loff_t *ppos)
+{
+	struct efivar_entry *var = file->private_data;
+	struct efivars *efivars = var->efivars;
+	efi_status_t status;
+	unsigned long datasize = 0;
+	u32 attributes;
+	void *data;
+	ssize_t size;
+
+	status = efivars->ops->get_variable(var->var.VariableName,
+					    &var->var.VendorGuid,
+					    &attributes, &datasize, NULL);
+
+	if (status != EFI_BUFFER_TOO_SMALL)
+		return 0;
+
+	data = kmalloc(datasize + 4, GFP_KERNEL);
+
+	if (!data)
+		return 0;
+
+	status = efivars->ops->get_variable(var->var.VariableName,
+					    &var->var.VendorGuid,
+					    &attributes, &datasize,
+					    (data + 4));
+
+	if (status != EFI_SUCCESS)
+		return 0;
+
+	memcpy(data, &attributes, 4);
+	size = simple_read_from_buffer(userbuf, count, ppos,
+					data, datasize + 4);
+	kfree(data);
+
+	return size;
+}
+
+static void efivarfs_evict_inode(struct inode *inode)
+{
+	clear_inode(inode);
+}
+
+static const struct super_operations efivarfs_ops = {
+	.statfs = simple_statfs,
+	.drop_inode = generic_delete_inode,
+	.evict_inode = efivarfs_evict_inode,
+	.show_options = generic_show_options,
+};
+
+static struct super_block *efivarfs_sb;
+
+static const struct inode_operations efivarfs_dir_inode_operations;
+
+static const struct file_operations efivarfs_file_operations = {
+	.open	= efivarfs_file_open,
+	.read	= efivarfs_file_read,
+	.write	= efivarfs_file_write,
+	.llseek	= no_llseek,
+};
+
+static struct inode *efivarfs_get_inode(struct super_block *sb,
+				const struct inode *dir, int mode, dev_t dev)
+{
+	struct inode *inode = new_inode(sb);
+
+	if (inode) {
+		inode->i_ino = get_next_ino();
+		inode->i_uid = inode->i_gid = 0;
+		inode->i_mode = mode;
+		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+		switch (mode & S_IFMT) {
+		case S_IFREG:
+			inode->i_fop = &efivarfs_file_operations;
+			break;
+		case S_IFDIR:
+			inode->i_op = &efivarfs_dir_inode_operations;
+			inode->i_fop = &simple_dir_operations;
+			inc_nlink(inode);
+			break;
+		}
+	}
+	return inode;
+}
+
+static void efivarfs_hex_to_guid(const char *str, efi_guid_t *guid)
+{
+	guid->b[0] = hex_to_bin(str[6]) << 4 | hex_to_bin(str[7]);
+	guid->b[1] = hex_to_bin(str[4]) << 4 | hex_to_bin(str[5]);
+	guid->b[2] = hex_to_bin(str[2]) << 4 | hex_to_bin(str[3]);
+	guid->b[3] = hex_to_bin(str[0]) << 4 | hex_to_bin(str[1]);
+	guid->b[4] = hex_to_bin(str[11]) << 4 | hex_to_bin(str[12]);
+	guid->b[5] = hex_to_bin(str[9]) << 4 | hex_to_bin(str[10]);
+	guid->b[6] = hex_to_bin(str[16]) << 4 | hex_to_bin(str[17]);
+	guid->b[7] = hex_to_bin(str[14]) << 4 | hex_to_bin(str[15]);
+	guid->b[8] = hex_to_bin(str[19]) << 4 | hex_to_bin(str[20]);
+	guid->b[9] = hex_to_bin(str[21]) << 4 | hex_to_bin(str[22]);
+	guid->b[10] = hex_to_bin(str[24]) << 4 | hex_to_bin(str[25]);
+	guid->b[11] = hex_to_bin(str[26]) << 4 | hex_to_bin(str[27]);
+	guid->b[12] = hex_to_bin(str[28]) << 4 | hex_to_bin(str[29]);
+	guid->b[13] = hex_to_bin(str[30]) << 4 | hex_to_bin(str[31]);
+	guid->b[14] = hex_to_bin(str[32]) << 4 | hex_to_bin(str[33]);
+	guid->b[15] = hex_to_bin(str[34]) << 4 | hex_to_bin(str[35]);
+}
+
+static int efivarfs_create(struct inode *dir, struct dentry *dentry,
+			  umode_t mode, bool excl)
+{
+	struct inode *inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
+	struct efivars *efivars = &__efivars;
+	struct efivar_entry *var;
+	int namelen, i = 0, err = 0;
+
+	if (dentry->d_name.len < 38)
+		return -EINVAL;
+
+	if (!inode)
+		return -ENOSPC;
+
+	var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
+
+	if (!var)
+		return -ENOMEM;
+
+	namelen = dentry->d_name.len - GUID_LEN;
+
+	efivarfs_hex_to_guid(dentry->d_name.name + namelen + 1,
+			&var->var.VendorGuid);
+
+	for (i = 0; i < namelen; i++)
+		var->var.VariableName[i] = dentry->d_name.name[i];
+
+	var->var.VariableName[i] = '\0';
+
+	inode->i_private = var;
+	var->efivars = efivars;
+	var->kobj.kset = efivars->kset;
+
+	err = kobject_init_and_add(&var->kobj, &efivar_ktype, NULL, "%s",
+			     dentry->d_name.name);
+	if (err)
+		goto out;
+
+	kobject_uevent(&var->kobj, KOBJ_ADD);
+	spin_lock(&efivars->lock);
+	list_add(&var->list, &efivars->list);
+	spin_unlock(&efivars->lock);
+	d_instantiate(dentry, inode);
+	dget(dentry);
+out:
+	if (err)
+		kfree(var);
+	return err;
+}
+
+static int efivarfs_unlink(struct inode *dir, struct dentry *dentry)
+{
+	struct efivar_entry *var = dentry->d_inode->i_private;
+	struct efivars *efivars = var->efivars;
+	efi_status_t status;
+
+	spin_lock(&efivars->lock);
+
+	status = efivars->ops->set_variable(var->var.VariableName,
+					    &var->var.VendorGuid,
+					    0, 0, NULL);
+
+	if (status == EFI_SUCCESS || status == EFI_NOT_FOUND) {
+		list_del(&var->list);
+		spin_unlock(&efivars->lock);
+		efivar_unregister(var);
+		drop_nlink(dir);
+		dput(dentry);
+		return 0;
+	}
+
+	spin_unlock(&efivars->lock);
+	return -EINVAL;
+};
+
+int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
+{
+	struct inode *inode = NULL;
+	struct dentry *root;
+	struct efivar_entry *entry, *n;
+	struct efivars *efivars = &__efivars;
+	int err;
+
+	efivarfs_sb = sb;
+
+	sb->s_maxbytes          = MAX_LFS_FILESIZE;
+	sb->s_blocksize         = PAGE_CACHE_SIZE;
+	sb->s_blocksize_bits    = PAGE_CACHE_SHIFT;
+	sb->s_magic             = PSTOREFS_MAGIC;
+	sb->s_op                = &efivarfs_ops;
+	sb->s_time_gran         = 1;
+
+	inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0);
+	if (!inode) {
+		err = -ENOMEM;
+		goto fail;
+	}
+	inode->i_op = &efivarfs_dir_inode_operations;
+
+	root = d_make_root(inode);
+	sb->s_root = root;
+	if (!root) {
+		err = -ENOMEM;
+		goto fail;
+	}
+
+	list_for_each_entry_safe(entry, n, &efivars->list, list) {
+		struct inode *inode;
+		struct dentry *dentry, *root = efivarfs_sb->s_root;
+		char *name;
+		unsigned long size = 0;
+		int len, i;
+
+		len = utf16_strlen(entry->var.VariableName);
+
+		/* GUID plus trailing NULL */
+		name = kmalloc(len + 38, GFP_ATOMIC);
+
+		for (i = 0; i < len; i++)
+			name[i] = entry->var.VariableName[i] & 0xFF;
+
+		name[len] = '-';
+
+		efi_guid_unparse(&entry->var.VendorGuid, name + len + 1);
+
+		name[len+GUID_LEN] = '\0';
+
+		inode = efivarfs_get_inode(efivarfs_sb, root->d_inode,
+					  S_IFREG | 0644, 0);
+		dentry = d_alloc_name(root, name);
+
+		efivars->ops->get_variable(entry->var.VariableName,
+					   &entry->var.VendorGuid,
+					   &entry->var.Attributes,
+					   &size,
+					   NULL);
+
+		mutex_lock(&inode->i_mutex);
+		inode->i_private = entry;
+		i_size_write(inode, size+4);
+		mutex_unlock(&inode->i_mutex);
+		d_add(dentry, inode);
+	}
+
+	return 0;
+fail:
+	iput(inode);
+	return err;
+}
+
+static struct dentry *efivarfs_mount(struct file_system_type *fs_type,
+				    int flags, const char *dev_name, void *data)
+{
+	return mount_single(fs_type, flags, data, efivarfs_fill_super);
+}
+
+static void efivarfs_kill_sb(struct super_block *sb)
+{
+	kill_litter_super(sb);
+	efivarfs_sb = NULL;
+}
+
+static struct file_system_type efivarfs_type = {
+	.name    = "efivarfs",
+	.mount   = efivarfs_mount,
+	.kill_sb = efivarfs_kill_sb,
+};
+
+static const struct inode_operations efivarfs_dir_inode_operations = {
+	.lookup = simple_lookup,
+	.unlink = efivarfs_unlink,
+	.create = efivarfs_create,
+};
+
+static struct pstore_info efi_pstore_info;
+
 #ifdef CONFIG_PSTORE
 
 static int efi_pstore_open(struct pstore_info *psi)
@@ -1198,6 +1571,8 @@ int register_efivars(struct efivars *efivars,
 		pstore_register(&efivars->efi_pstore_info);
 	}
 
+	register_filesystem(&efivarfs_type);
+
 out:
 	kfree(variable_name);
 
@@ -1205,9 +1580,6 @@ out:
 }
 EXPORT_SYMBOL_GPL(register_efivars);
 
-static struct efivars __efivars;
-static struct efivar_operations ops;
-
 /*
  * For now we register the efi subsystem with the firmware subsystem
  * and the vars subsystem with the efi subsystem.  In the future, it
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8670eb1..b2af157 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -29,7 +29,12 @@
 #define EFI_UNSUPPORTED		( 3 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_BAD_BUFFER_SIZE     ( 4 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_BUFFER_TOO_SMALL	( 5 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_NOT_READY		( 6 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_DEVICE_ERROR	( 7 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_WRITE_PROTECTED	( 8 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_OUT_OF_RESOURCES	( 9 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_NOT_FOUND		(14 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_SECURITY_VIOLATION	(26 | (1UL << (BITS_PER_LONG-1)))
 
 typedef unsigned long efi_status_t;
 typedef u8 efi_bool_t;
-- 
1.7.11.7

[PATCH v2 03/20] efi: add efivars kobject to efi sysfs folder

[Matt Fleming]

From: "Lee, Chun-Yi" <joeyli.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

UEFI variable filesystem need a new mount point, so this patch add
efivars kobject to efi_kobj for create a /sys/firmware/efi/efivars
folder.

Cc: Matthew Garrett <mjg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Signed-off-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---

v2: Address comments from Alan Cox where efivars->kset was unregistered
    in the success path, not the error path.

 drivers/firmware/efivars.c | 9 +++++++++
 include/linux/efi.h        | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index d7658b4..6793965 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1527,6 +1527,7 @@ void unregister_efivars(struct efivars *efivars)
 		sysfs_remove_bin_file(&efivars->kset->kobj, efivars->del_var);
 	kfree(efivars->new_var);
 	kfree(efivars->del_var);
+	kobject_put(efivars->kobject);
 	kset_unregister(efivars->kset);
 }
 EXPORT_SYMBOL_GPL(unregister_efivars);
@@ -1558,6 +1559,14 @@ int register_efivars(struct efivars *efivars,
 		goto out;
 	}
 
+	efivars->kobject = kobject_create_and_add("efivars", parent_kobj);
+	if (!efivars->kobject) {
+		pr_err("efivars: Subsystem registration failed.\n");
+		error = -ENOMEM;
+		kset_unregister(efivars->kset);
+		goto out;
+	}
+
 	/*
 	 * Per EFI spec, the maximum storage allocated for both
 	 * the variable name and variable data is 1024 bytes.
diff --git a/include/linux/efi.h b/include/linux/efi.h
index b2af157..337aefb 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -662,6 +662,7 @@ struct efivars {
 	spinlock_t lock;
 	struct list_head list;
 	struct kset *kset;
+	struct kobject *kobject;
 	struct bin_attribute *new_var, *del_var;
 	const struct efivar_operations *ops;
 	struct efivar_entry *walk_entry;
-- 
1.7.11.7

[PATCH v2 16/20] efivarfs: Return an error if we fail to read a variable

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Instead of always returning 0 in efivarfs_file_read(), even when we
fail to successfully read the variable, convert the EFI status to
something meaningful and return that to the caller. This way the user
will have some hint as to why the read failed.

Acked-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---

v2: Alan Cox noted that I missed a place to use efi_status_to_err() in
    the first version of this patch.

 drivers/firmware/efivars.c | 62 +++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 26 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index a93e401..277e426 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -653,6 +653,36 @@ static int efivarfs_file_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int efi_status_to_err(efi_status_t status)
+{
+	int err;
+
+	switch (status) {
+	case EFI_INVALID_PARAMETER:
+		err = -EINVAL;
+		break;
+	case EFI_OUT_OF_RESOURCES:
+		err = -ENOSPC;
+		break;
+	case EFI_DEVICE_ERROR:
+		err = -EIO;
+		break;
+	case EFI_WRITE_PROTECTED:
+		err = -EROFS;
+		break;
+	case EFI_SECURITY_VIOLATION:
+		err = -EACCES;
+		break;
+	case EFI_NOT_FOUND:
+		err = -ENOENT;
+		break;
+	default:
+		err = -EINVAL;
+	}
+
+	return err;
+}
+
 static ssize_t efivarfs_file_write(struct file *file,
 		const char __user *userbuf, size_t count, loff_t *ppos)
 {
@@ -711,29 +741,7 @@ static ssize_t efivarfs_file_write(struct file *file,
 		spin_unlock(&efivars->lock);
 		kfree(data);
 
-		switch (status) {
-		case EFI_INVALID_PARAMETER:
-			count = -EINVAL;
-			break;
-		case EFI_OUT_OF_RESOURCES:
-			count = -ENOSPC;
-			break;
-		case EFI_DEVICE_ERROR:
-			count = -EIO;
-			break;
-		case EFI_WRITE_PROTECTED:
-			count = -EROFS;
-			break;
-		case EFI_SECURITY_VIOLATION:
-			count = -EACCES;
-			break;
-		case EFI_NOT_FOUND:
-			count = -ENOENT;
-			break;
-		default:
-			count = -EINVAL;
-		}
-		return count;
+		return efi_status_to_err(status);
 	}
 
 	/*
@@ -791,12 +799,12 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	spin_unlock(&efivars->lock);
 
 	if (status != EFI_BUFFER_TOO_SMALL)
-		return 0;
+		return efi_status_to_err(status);
 
 	data = kmalloc(datasize + 4, GFP_KERNEL);
 
 	if (!data)
-		return 0;
+		return -ENOMEM;
 
 	spin_lock(&efivars->lock);
 	status = efivars->ops->get_variable(var->var.VariableName,
@@ -805,8 +813,10 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 					    (data + 4));
 	spin_unlock(&efivars->lock);
 
-	if (status != EFI_SUCCESS)
+	if (status != EFI_SUCCESS) {
+		size = efi_status_to_err(status);
 		goto out_free;
+	}
 
 	memcpy(data, &attributes, 4);
 	size = simple_read_from_buffer(userbuf, count, ppos,
-- 
1.7.11.7

[PATCH 21/20] efivarfs: Fix return value of efivarfs_file_write()

[Matt Fleming]

From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

We're stuffing a variable of type size_t (unsigned) into a ssize_t
(signed) which, even though both types should be the same number of
bits, it's just asking for sign issues to be introduced.

Cc: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Reported-by: Alan Cox <alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 58cec62..9ac9340 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -694,6 +694,7 @@ static ssize_t efivarfs_file_write(struct file *file,
 	struct inode *inode = file->f_mapping->host;
 	unsigned long datasize = count - sizeof(attributes);
 	unsigned long newdatasize;
+	ssize_t bytes = 0;
 
 	if (count < sizeof(attributes))
 		return -EINVAL;
@@ -706,22 +707,22 @@ static ssize_t efivarfs_file_write(struct file *file,
 	efivars = var->efivars;
 
 	if (copy_from_user(&attributes, userbuf, sizeof(attributes))) {
-		count = -EFAULT;
+		bytes = -EFAULT;
 		goto out;
 	}
 
 	if (attributes & ~(EFI_VARIABLE_MASK)) {
-		count = -EINVAL;
+		bytes = -EINVAL;
 		goto out;
 	}
 
 	if (copy_from_user(data, userbuf + sizeof(attributes), datasize)) {
-		count = -EFAULT;
+		bytes = -EFAULT;
 		goto out;
 	}
 
 	if (validate_var(&var->var, data, datasize) == false) {
-		count = -EINVAL;
+		bytes = -EINVAL;
 		goto out;
 	}
 
@@ -744,6 +745,8 @@ static ssize_t efivarfs_file_write(struct file *file,
 		return efi_status_to_err(status);
 	}
 
+	bytes = count;
+
 	/*
 	 * Writing to the variable may have caused a change in size (which
 	 * could either be an append or an overwrite), or the variable to be
@@ -778,7 +781,7 @@ static ssize_t efivarfs_file_write(struct file *file,
 out:
 	kfree(data);
 
-	return count;
+	return bytes;
 }
 
 static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
-- 
1.7.11.7

Re: [PATCH v2 01/20] efi: Add support for a UEFI variable filesystem

[Matthew Garrett]

On Sat, Nov 03, 2012 at 12:22:49AM +0000, Alan Cox wrote:

> This still attempts to allocate arbitary user provided values of memory.
> This is still broken. Even in the extreme case of 'root only, right thing
> to do' then this is broken as you don't pass __GFP_NOWARN. However if you
> can have very large strings remember that big values handled this way are
> actually effectively implemented as
> 
> 	"maybe set the value or randomly return -ENOMEM possibly until a
> 	reboot"
> 
> so if your real upper limit is huge then this isn't good. If on the other
> hand its things like 32K or so then you just want NOWARN.

Yeah. It's fair that this should be limited to whatever 
QueryVariableInfo() returns on systems that have that, and say 64K on 
older systems.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH v2 01/20] efi: Add support for a UEFI variable filesystem

[Alan Cox]

> +static ssize_t efivarfs_file_write(struct file *file,
> +		const char __user *userbuf, size_t count, loff_t *ppos)
> +{
> +	struct efivar_entry *var = file->private_data;
> +	struct efivars *efivars;
> +	efi_status_t status;
> +	void *data;
> +	u32 attributes;
> +	struct inode *inode = file->f_mapping->host;
> +	int datasize = count - sizeof(attributes);
> +
> +	if (count < sizeof(attributes))
> +		return -EINVAL;
> +
> +	data = kmalloc(datasize, GFP_KERNEL);

This still attempts to allocate arbitary user provided values of memory.
This is still broken. Even in the extreme case of 'root only, right thing
to do' then this is broken as you don't pass __GFP_NOWARN. However if you
can have very large strings remember that big values handled this way are
actually effectively implemented as

	"maybe set the value or randomly return -ENOMEM possibly until a
	reboot"

so if your real upper limit is huge then this isn't good. If on the other
hand its things like 32K or so then you just want NOWARN.

Alan

Re: [PATCH v2 01/20] efi: Add support for a UEFI variable filesystem

[Matt Fleming]

On Sat, 2012-11-03 at 00:21 +0000, Matthew Garrett wrote:
> Yeah. It's fair that this should be limited to whatever 
> QueryVariableInfo() returns on systems that have that, and say 64K on 
> older systems.

Folks how about something like this (compile-tested only)?

---

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 9ac9340..b75fdf2 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -694,28 +694,54 @@ static ssize_t efivarfs_file_write(struct file *file,
 	struct inode *inode = file->f_mapping->host;
 	unsigned long datasize = count - sizeof(attributes);
 	unsigned long newdatasize;
+	u64 storage_size, remaining_size, max_size;
 	ssize_t bytes = 0;
 
 	if (count < sizeof(attributes))
 		return -EINVAL;
 
-	data = kmalloc(datasize, GFP_KERNEL);
+	if (copy_from_user(&attributes, userbuf, sizeof(attributes)))
+		return -EFAULT;
 
-	if (!data)
-		return -ENOMEM;
+	if (attributes & ~(EFI_VARIABLE_MASK))
+		return -EINVAL;
 
 	efivars = var->efivars;
 
-	if (copy_from_user(&attributes, userbuf, sizeof(attributes))) {
-		bytes = -EFAULT;
-		goto out;
+	/*
+	 * Ensure that the user can't allocate arbitrarily large
+	 * amounts of memory. Pick a default size of 64K if
+	 * QueryVariableInfo() isn't supported by the firmware.
+	 */
+	spin_lock(&efivars->lock);
+
+	if (!efivars->ops->query_variable_info)
+		status = EFI_UNSUPPORTED;
+	else {
+		const struct efivar_operations *fops = efivars->ops;
+		status = fops->query_variable_info(attributes, &storage_size,
+						   &remaining_size, &max_size);
 	}
 
-	if (attributes & ~(EFI_VARIABLE_MASK)) {
-		bytes = -EINVAL;
-		goto out;
+	spin_unlock(&efivars->lock);
+
+	if (status != EFI_SUCCESS) {
+		if (status != EFI_UNSUPPORTED)
+			return efi_status_to_err(status);
+
+		remaining_size = 65536;
+	}
+
+	if (datasize > remaining_size) {
+		printk(KERN_ERR "efivars: Variable size too big\n");
+		return -ENOSPC;
 	}
 
+	data = kmalloc(datasize, GFP_KERNEL);
+
+	if (!data)
+		return -ENOMEM;
+
 	if (copy_from_user(data, userbuf + sizeof(attributes), datasize)) {
 		bytes = -EFAULT;
 		goto out;
@@ -1709,6 +1735,9 @@ efivars_init(void)
 	ops.get_variable = efi.get_variable;
 	ops.set_variable = efi.set_variable;
 	ops.get_next_variable = efi.get_next_variable;
+#ifdef CONFIG_X86
+	ops.query_variable_info = efi.query_variable_info;
+#endif
 	error = register_efivars(&__efivars, &ops, efi_kobj);
 	if (error)
 		goto err_put;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 5e2308d..f80079c 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -646,6 +646,7 @@ struct efivar_operations {
 	efi_get_variable_t *get_variable;
 	efi_get_next_variable_t *get_next_variable;
 	efi_set_variable_t *set_variable;
+	efi_query_variable_info_t *query_variable_info;
 };
 
 struct efivars {


-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 01/20] efi: Add support for a UEFI variable filesystem

[Matthew Garrett]

On Sun, Nov 04, 2012 at 08:27:58PM +0000, Matt Fleming wrote:

> +#ifdef CONFIG_X86
> +	ops.query_variable_info = efi.query_variable_info;
> +#endif

Why limit to x86? Otherwise, looks broadly good - I can try to test 
tomorrow.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH v2 01/20] efi: Add support for a UEFI variable filesystem

[Matt Fleming]

On Sun, 2012-11-04 at 20:34 +0000, Matthew Garrett wrote:
> On Sun, Nov 04, 2012 at 08:27:58PM +0000, Matt Fleming wrote:
> 
> > +#ifdef CONFIG_X86
> > +	ops.query_variable_info = efi.query_variable_info;
> > +#endif
> 
> Why limit to x86? Otherwise, looks broadly good - I can try to test 
> tomorrow.

Because from what I can see, not only does ia64 not fill out the
query_variable_info() pointer in struct efi, struct efi is in the data
segment, and so, contains an uninitialised pointer.

Now, the proper approach would be to either implement
query_variable_info() or initialise the pointer to NULL. I don't have
any ia64 hardware for the former, but would be happy to carry a patch
for the latter if everyone else is happy with that.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 01/20] efi: Add support for a UEFI variable filesystem

[Matthew Garrett]

On Sun, Nov 04, 2012 at 08:47:06PM +0000, Matt Fleming wrote:

> Because from what I can see, not only does ia64 not fill out the
> query_variable_info() pointer in struct efi, struct efi is in the data
> segment, and so, contains an uninitialised pointer.

Oh, right. Another argument for some unified EFI code. Sigh. My only 
concern in that case is that someone's going to fix this at some point 
and then be confused as to why it doesn't work on ARM.

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: [PATCH v2 01/20] efi: Add support for a UEFI variable filesystem

[Alan Cox]

> +	if (datasize > remaining_size) {
> +		printk(KERN_ERR "efivars: Variable size too big\n");
> +		return -ENOSPC;

Why the printk - you've handed back a sensible error code. If you do have
a logging result you need to rate limit it as its from the user. I don't
think you need one - -ENOSPC is pretty clear !

Re: [PATCH v2 01/20] efi: Add support for a UEFI variable filesystem

[Matt Fleming]

On Sun, 2012-11-04 at 21:06 +0000, Alan Cox wrote:
> > +	if (datasize > remaining_size) {
> > +		printk(KERN_ERR "efivars: Variable size too big\n");
> > +		return -ENOSPC;
> 
> Why the printk - you've handed back a sensible error code. If you do have
> a logging result you need to rate limit it as its from the user. I don't
> think you need one - -ENOSPC is pretty clear !

OK, I'll drop the printk.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 01/20] efi: Add support for a UEFI variable filesystem

[Matt Fleming]

On Sun, 2012-11-04 at 20:34 +0000, Matthew Garrett wrote:
> On Sun, Nov 04, 2012 at 08:27:58PM +0000, Matt Fleming wrote:
> 
> > +#ifdef CONFIG_X86
> > +	ops.query_variable_info = efi.query_variable_info;
> > +#endif
> 
> Why limit to x86? Otherwise, looks broadly good - I can try to test 
> tomorrow.

Did anyone get chance to test this out? I'd really like to get the
efivarfs code in a position for merging for v3.8.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 01/20] efi: Add support for a UEFI variable filesystem

[Lingzhu Xiang]

On 10/26/2012 03:51 PM, Matt Fleming wrote:
> +static int efivarfs_unlink(struct inode *dir, struct dentry *dentry)
> +{
> +	struct efivar_entry *var = dentry->d_inode->i_private;
> +	struct efivars *efivars = var->efivars;
> +	efi_status_t status;
> +
> +	spin_lock(&efivars->lock);
> +
> +	status = efivars->ops->set_variable(var->var.VariableName,
> +					&var->var.VendorGuid,
> +					    0, 0, NULL);
> +
> +	if (status == EFI_SUCCESS || status == EFI_NOT_FOUND) {
> +		list_del(&var->list);
> +		spin_unlock(&efivars->lock);
> +		efivar_unregister(var);
> +		drop_nlink(dir);

This should be  drop_nlink(dentry->d_inode);

I'm getting warnings when testing Fedora 18 kernel with efivarfs patch
backported. i_nlink underflowed.

[root@localhost ~]# mount -t efivarfs - /sys/firmware/efi/efivars/
[root@localhost ~]# ls -1 /sys/firmware/efi/efivars/ | wc -l
28
[root@localhost ~]# stat -c%h /sys/firmware/efi/efivars/
2
[root@localhost ~]# rm -f /sys/firmware/efi/efivars/*
[   41.690805] ------------[ cut here ]------------
[   41.692975] WARNING: at fs/inode.c:280 drop_nlink+0x46/0x50()
[   41.694546] Modules linked in: nf_conntrack_netbios_ns nf_conntrack_broadcast ipt_MASQUERADE ip6table_mangle ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter ip6_tables iptable_nat nf_nat iptable_mangle nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack vfat fat i2c_piix4 i2c_core microcode virtio_net
[   41.701583] Pid: 822, comm: rm Tainted: G        W    3.6.11-3.fc18.x86_64.debug #1
[   41.703008] Call Trace:
[   41.703499]  [<ffffffff8106781f>] warn_slowpath_common+0x7f/0xc0
[   41.704591]  [<ffffffff8106787a>] warn_slowpath_null+0x1a/0x20
[   41.705777]  [<ffffffff811eb266>] drop_nlink+0x46/0x50
[   41.706855]  [<ffffffff81553fca>] efivarfs_unlink+0x8a/0xd0
[   41.707952]  [<ffffffff811dd75e>] vfs_unlink+0x9e/0x110
[   41.710800]  [<ffffffff811dd90d>] do_unlinkat+0x13d/0x1a0
[   41.712096]  [<ffffffff810d6b6d>] ? trace_hardirqs_on_caller+0x10d/0x1a0
[   41.713808]  [<ffffffff8110181c>] ? __audit_syscall_entry+0xcc/0x300
[   41.714983]  [<ffffffff8134f50e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[   41.716548]  [<ffffffff811e025b>] sys_unlinkat+0x1b/0x50
[   41.717694]  [<ffffffff816e7fe9>] system_call_fastpath+0x16/0x1b
[   41.718995] ---[ end trace 33f799b901d1db9e ]---
[root@localhost ~]# ls -1 /sys/firmware/efi/efivars/ | wc -l
15
[root@localhost ~]# stat -c%h /sys/firmware/efi/efivars/
4294967285


> +		dput(dentry);
> +		return 0;
> +	}

Re: [PATCH v2 16/20] efivarfs: Return an error if we fail to read a variable

[Lingzhu Xiang]

On 11/02/2012 04:53 PM, Matt Fleming wrote:
> Instead of always returning 0 in efivarfs_file_read(), even when we
> fail to successfully read the variable, convert the EFI status to
> something meaningful and return that to the caller. This way the user
> will have some hint as to why the read failed.
> ...
>
> +	case EFI_NOT_FOUND:
> +		err = -ENOENT;

This will be ambiguous for userspace. Is it file not being found by
efivarfs or by firmware?

Actually I've been confused during testing like this:

# ll test-12341234-1234-1234-1234-123412341234
ls: cannot access test-12341234-1234-1234-1234-123412341234: No such 
file or directory
# echo -en "\0\0\0\0a" >test-12341234-1234-1234-1234-123412341234
-bash: echo: write error: No such file or directory
# ll test-12341234-1234-1234-1234-123412341234
-rw-r--r--. 1 root root 0 Dec 21 06:19 
test-12341234-1234-1234-1234-123412341234


--
Lingzhu Xiang

General protection fault in efivarfs

[Lingzhu Xiang]

The following reproducer triggers certain bugs in efivarfs_file_write.

#!/bin/bash
p=/sys/firmware/efi/efivars
mount -t efivarfs - $p
cat $p/Lang-* >$p/test-12341234-1234-1234-1234-123412341234
umount $p
mount -t efivarfs - $p
echo -en "\0\0\0\0" >$p/test-12341234-1234-1234-1234-123412341234
cat $p/Lang-* >$p/test-12341234-1234-1234-1234-123412341234

Without the umount and mount, it causes nothing. After reproduction,
umount and various filesystem operations become unstable.

3.6.11 and 3.7 kernels are from Fedora 18/Rawhide with efivarfs
backported by Josh Boyer on Nov 2.

It is always reproducible, but sometimes it hanged with no call trace.

IBM machine, 3.7:
[   59.978216] general protection fault: 0000 [#1] SMP
[   59.983214] Modules linked in: nf_conntrack_netbios_ns 
nf_conntrack_broadcast ipt_MASQUERADE ip6table_mangle ip6t_REJECT 
nf_conntrack_ipv6 nf_defrag_ipv6 iptable_nat nf_nat_ipv4 nf_nat 
iptable_mangle nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack 
nf_conntrack ebtable_filter ebtables ip6table_filter ip6_tables vfat fat 
iTCO_wdt shpchp vhost_net i7core_edac coretemp crc32c_intel 
iTCO_vendor_support serio_raw ioatdma edac_core lpc_ich i2c_i801 
cdc_ether microcode mfd_core dca usbnet tun mii bnx2 macvtap macvlan 
kvm_intel kvm mgag200 i2c_algo_bit mptsas drm_kms_helper ttm mptscsih 
drm mptbase i2c_core scsi_transport_sas
[   60.038660] CPU 9
[   60.040501] Pid: 1001, comm: cat Not tainted 3.7.0-2.fc19.x86_64 #1 
IBM System x3550 M3 -[7944I21]-/69Y4438
[   60.050840] RIP: 0010:[<ffffffff810d5d1e>]  [<ffffffff810d5d1e>] 
__lock_acquire+0x5e/0x1bb0
[   60.059198] RSP: 0018:ffff880270595ce8  EFLAGS: 00010046
[   60.064500] RAX: 0000000000000046 RBX: 0000000000000002 RCX: 
0000000000000000
[   60.071617] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 
6b6b6b6b6b6b6b83
[   60.078735] RBP: ffff880270595dd8 R08: 0000000000000002 R09: 
0000000000000000
[   60.085852] R10: 6b6b6b6b6b6b6b83 R11: 0000000000000000 R12: 
0000000000000000
[   60.092971] R13: ffff88027170cd20 R14: 0000000000000000 R15: 
0000000000000000
[   60.100091] FS:  00007fc0c8ff3740(0000) GS:ffff880277000000(0000) 
knlGS:0000000000000000
[   60.108164] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   60.113899] CR2: 0000000001520000 CR3: 000000026d594000 CR4: 
00000000000007e0
[   60.121016] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
0000000000000000
[   60.128135] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 
0000000000000400
[   60.135254] Process cat (pid: 1001, threadinfo ffff880270594000, task 
ffff88027170cd20)
[   60.143239] Stack:
[   60.145251]  ffff880270595cf8 ffffffff81021da3 ffff880270595d08 
ffffffff81021e19
[   60.152714]  ffff880270595d38 ffffffff810acdb5 ffff880200000168 
0000000000000086
[   60.160175]  ffff88027170d5e8 ffffffff810d25ed ffff880270595d58 
ffffffff810ace7f
[   60.167638] Call Trace:
[   60.170088]  [<ffffffff81021da3>] ? native_sched_clock+0x13/0x80
[   60.176085]  [<ffffffff81021e19>] ? sched_clock+0x9/0x10
[   60.181389]  [<ffffffff810acdb5>] ? sched_clock_cpu+0xc5/0x120
[   60.187211]  [<ffffffff810d25ed>] ? trace_hardirqs_off+0xd/0x10
[   60.193121]  [<ffffffff810ace7f>] ? local_clock+0x6f/0x80
[   60.198513]  [<ffffffff810d2f6f>] ? 
lock_release_holdtime.part.26+0xf/0x180
[   60.205465]  [<ffffffff810d7b57>] ? lock_release_non_nested+0x2e7/0x320
[   60.212073]  [<ffffffff815638bb>] ? efivarfs_file_write+0x5b/0x280
[   60.218242]  [<ffffffff810d7f41>] lock_acquire+0xa1/0x1f0
[   60.223633]  [<ffffffff81563971>] ? efivarfs_file_write+0x111/0x280
[   60.229892]  [<ffffffff8118b47c>] ? might_fault+0x5c/0xb0
[   60.235287]  [<ffffffff816f1bf6>] _raw_spin_lock+0x46/0x80
[   60.240762]  [<ffffffff81563971>] ? efivarfs_file_write+0x111/0x280
[   60.247018]  [<ffffffff81563971>] efivarfs_file_write+0x111/0x280
[   60.253103]  [<ffffffff811d307f>] vfs_write+0xaf/0x190
[   60.258233]  [<ffffffff811d33d5>] sys_write+0x55/0xa0
[   60.263278]  [<ffffffff816fbd19>] system_call_fastpath+0x16/0x1b
[   60.269271] Code: 41 0f 45 d8 4c 89 75 f0 4c 89 7d f8 85 c0 0f 84 09 
01 00 00 8b 05 a3 f9 ff 00 49 89 fa 41 89 f6 41 89 d3 85 c0 0f 84 12 01 
00 00 <49> 8b 02 ba 01 00 00 00 48 3d a0 07 14 82 0f 44 da 41 83 fe 01
[   60.289431] RIP  [<ffffffff810d5d1e>] __lock_acquire+0x5e/0x1bb0
[   60.295444]  RSP <ffff880270595ce8>
[   60.298928] ---[ end trace 1bbfd41a2cf6a0d8 ]---
(More pstore "scheduling while atomic" ensues if debug options are 
turned on.)
Segmentation fault

QEMU with OVMF, 3.6.11:
[   74.193012] BUG: unable to handle kernel NULL pointer dereference at 
           (null)
[   74.193016] IP: [<ffffffff81623b0e>] _raw_spin_lock+0xe/0x30
[   74.193016] PGD 3ff0d067 PUD 3fe41067 PMD 0
[   74.193016] Oops: 0002 [#1] SMP
[   74.193016] Modules linked in: nf_conntrack_netbios_ns 
nf_conntrack_broadcast ipt_MASQUERADE ip6table_mangle ip6t_REJECT 
nf_conntrack_ipv6 nf_defrag_ipv6 iptable_nat nf_nat iptable_mangle 
nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack 
ebtable_filter ebtables ip6table_filter ip6_tables vfat fat virtio_net 
i2c_piix4 i2c_core microcode
[   74.193016] CPU 1
[   74.193016] Pid: 782, comm: echo Tainted: G        W 
3.6.11-3.fc18.x86_64 #1
[   74.193016] RIP: 0010:[<ffffffff81623b0e>]  [<ffffffff81623b0e>] 
_raw_spin_lock+0xe/0x30
[   74.193016] RSP: 0018:ffff8800797f7e88  EFLAGS: 00010202
[   74.193016] RAX: 0000000000000100 RBX: ffff8800787db6a0 RCX: 
0000000000000074
[   74.193016] RDX: 0000000000000004 RSI: ffff8800787db6a0 RDI: 
0000000000000000
[   74.193016] RBP: ffff8800797f7e88 R08: 0000000000000050 R09: 
ffffffff814d558b
[   74.193016] R10: 00007fff02dff770 R11: ffffffff81a1c31c R12: 
0000000000000008
[   74.193016] R13: ffff880078297000 R14: 0000000000000004 R15: 
ffff880077eee000
[   74.193016] FS:  00007fc6dbbc4740(0000) GS:ffff88007d900000(0000) 
knlGS:0000000000000000
[   74.193016] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   74.193016] CR2: 0000000000000000 CR3: 000000003fc31000 CR4: 
00000000000406e0
[   74.193016] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
0000000000000000
[   74.193016] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 
0000000000000400
[   74.193016] Process echo (pid: 782, threadinfo ffff8800797f6000, task 
ffff880078184530)
[   74.193016] Stack:
[   74.193016]  ffff8800797f7ef8 ffffffff814d5641 ffff8800797f7ec8 
ffff88007c4313b0
[   74.193016]  0000000000000000 ffff880077eee008 0000000078297000 
0000000000000001
[   74.193016]  ffff8800797f7ef8 ffff880078297000 0000000000000008 
00007fc6dbbcb000
[   74.193016] Call Trace:
[   74.193016]  [<ffffffff814d5641>] efivarfs_file_write+0x111/0x270
[   74.193016]  [<ffffffff811908cc>] vfs_write+0xac/0x180
[   74.193016]  [<ffffffff81190bfa>] sys_write+0x4a/0x90
[   74.193016]  [<ffffffff8162bd69>] system_call_fastpath+0x16/0x1b
[   74.193016] Code: c2 ff ff ff ff be 01 00 00 00 48 89 e5 e8 8b fe ff 
ff 5d c3 90 90 90 90 90 90 90 90 90 55 48 89 e5 66 66 66 66 90 b8 00 01 
00 00 <f0> 66 0f c1 07 0f b6 d4 38 c2 74 0f 66 0f 1f 44 00 00 f3 90 0f
[   74.193016] RIP  [<ffffffff81623b0e>] _raw_spin_lock+0xe/0x30
[   74.193016]  RSP <ffff8800797f7e88>
[   74.193016] CR2: 0000000000000000
[   74.241459] ---[ end trace 675d5a6c2b09c5be ]---
Killed

QEMU with OVMF, 3.6.11, another one:
[   19.437433] general protection fault: 0000 [#1] SMP
[   19.438011] Modules linked in: nf_conntrack_netbios_ns 
nf_conntrack_broadcast ipt_MASQUERADE ip6table_mangle ip6t_REJECT 
nf_conntrack_ipv6 nf_defrag_ipv6 iptable_nat nf_nat iptable_mangle 
nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack 
ebtable_filter ebtables ip6table_filter ip6_tables vfat fat i2c_piix4 
i2c_core virtio_net microcode
[   19.438011] CPU 0
[   19.438011] Pid: 775, comm: cat Tainted: G        W 
3.6.11-3.fc18.x86_64 #1
[   19.438011] RIP: 0010:[<ffffffff814d5664>]  [<ffffffff814d5664>] 
efivarfs_file_write+0x134/0x270
[   19.438011] RSP: 0018:ffff880077fd1e98  EFLAGS: 00010246
[   19.438011] RAX: 3134333231343332 RBX: ffff8800786871f0 RCX: 
0000000000000004
[   19.438011] RDX: 0000000000000007 RSI: ffff88007f45b408 RDI: 
ffff88007f45b008
[   19.438011] RBP: ffff880077fd1ef8 R08: ffff8800786871f0 R09: 
ffff88007f45b408
[   19.438011] R10: 00007fff15780f60 R11: ffffffff81a1c31c R12: 
0000000000000008
[   19.438011] R13: ffff880078919700 R14: 0000000000000004 R15: 
ffff88007f45b000
[   19.438011] FS:  00007fa220298740(0000) GS:ffff88007d800000(0000) 
knlGS:0000000000000000
[   19.438011] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   19.438011] CR2: 0000000001c56000 CR3: 0000000079286000 CR4: 
00000000000406f0
[   19.438011] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
0000000000000000
[   19.438011] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 
0000000000000400
[   19.438011] Process cat (pid: 775, threadinfo ffff880077fd0000, task 
ffff88007e84ae20)
[   19.438011] Stack:
[   19.438011]  ffff88007f45b408 ffff880079a088c0 ffff88007f459000 
ffff88007f45b008
[   19.438011]  0000000778919700 0000000000000001 ffff880077fd1ef8 
ffff880078919700
[   19.438011]  0000000000000008 0000000001c56000 ffff880077fd1f50 
0000000000010000
[   19.438011] Call Trace:
[   19.438011]  [<ffffffff811908cc>] vfs_write+0xac/0x180
[   19.438011]  [<ffffffff81190bfa>] sys_write+0x4a/0x90
[   19.438011]  [<ffffffff8162bd69>] system_call_fastpath+0x16/0x1b
[   19.438011] Code: 8b 7d b0 e8 bf e4 14 00 48 8b 55 b0 4d 8d 8f 08 04 
00 00 4c 89 f1 49 89 d8 48 8b 7d b8 4c 89 ce 48 8b 42 38 4c 89 4d a0 8b 
55 c4 <ff> 50 10 48 85 c0 49 89 c6 4c 8b 4d a0 74 59 48 8b 45 b0 80 00
[   19.438011] RIP  [<ffffffff814d5664>] efivarfs_file_write+0x134/0x270
[   19.438011]  RSP <ffff880077fd1e98>
[   19.475465] ---[ end trace 969a2b2b14764a38 ]---
[   19.479214] BUG: unable to handle kernel NULL pointer dereference at 
0000000000000100
[   19.480040] IP: [<ffffffff8117b543>] kmem_cache_alloc_trace+0x63/0x160
[   19.480040] PGD 3fb00067 PUD 79d51067 PMD 0
[   19.480040] Oops: 0000 [#2] SMP
[   19.480040] Modules linked in: nf_conntrack_netbios_ns 
nf_conntrack_broadcast ipt_MASQUERADE ip6table_mangle ip6t_REJECT 
nf_conntrack_ipv6 nf_defrag_ipv6 iptable_nat nf_nat iptable_mangle 
nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack 
ebtable_filter ebtables ip6table_filter ip6_tables vfat fat i2c_piix4 
i2c_core virtio_net microcode
[   19.480040] CPU 0
[   19.480040] Pid: 775, comm: cat Tainted: G      D W 
3.6.11-3.fc18.x86_64 #1
[   19.480040] RIP: 0010:[<ffffffff8117b543>]  [<ffffffff8117b543>] 
kmem_cache_alloc_trace+0x63/0x160
[   19.480040] RSP: 0018:ffff880077fd19c8  EFLAGS: 00010246
[   19.480040] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 
0000000000000000
[   19.480040] RDX: 000000000000e97e RSI: 00000000000080d0 RDI: 
ffff88007d402d00
[   19.480040] RBP: ffff880077fd1a18 R08: 0000000000016ae0 R09: 
ffffffff812df6ec
[   19.480040] R10: 0000000000002580 R11: 66386366632d3536 R12: 
0000000000000100
[   19.480040] R13: 00000000000080d0 R14: 0000000000000908 R15: 
ffff88007d402d00
[   19.480040] FS:  00007fa220298740(0000) GS:ffff88007d800000(0000) 
knlGS:0000000000000000
[   19.480040] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   19.480040] CR2: 0000000000000100 CR3: 0000000079286000 CR4: 
00000000000406f0
[   19.480040] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 
0000000000000000
[   19.480040] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 
0000000000000400
[   19.480040] Process cat (pid: 775, threadinfo ffff880077fd0000, task 
ffff88007e84ae20)
[   19.480040] Stack:
[   19.480040]  ffff880077fd19d8 ffffffff81201a62 ffffffff812df6ec 
ffffffff81201b36
[   19.480040]  ffff880077fd1a48 0000000000000000 0000000000000000 
ffff88007f459840
[   19.480040]  0000000000000000 ffff88007f570960 ffff880077fd1ab8 
ffffffff812df6ec
[   19.480040] Call Trace:
[   19.480040]  [<ffffffff81201a62>] ? sysfs_add_file+0x12/0x20
[   19.480040]  [<ffffffff812df6ec>] ? kobject_uevent_env+0x14c/0x610
[   19.480040]  [<ffffffff81201b36>] ? sysfs_create_file+0x26/0x30
[   19.480040]  [<ffffffff812df6ec>] kobject_uevent_env+0x14c/0x610
[   19.480040]  [<ffffffff812dee63>] ? kobject_init_and_add+0x63/0x90
[   19.480040]  [<ffffffff812dfbbb>] kobject_uevent+0xb/0x10
[   19.480040]  [<ffffffff814d58d2>] efivar_create_sysfs_entry+0x132/0x1b0
[   19.480040]  [<ffffffff814d5c83>] efi_pstore_write+0x333/0x3a0
[   19.480040]  [<ffffffff8105dc5e>] ? kmsg_dump_get_buffer+0x24e/0x2b0
[   19.480040]  [<ffffffff8126caec>] pstore_dump+0x12c/0x1f0
[   19.480040]  [<ffffffff8106017c>] kmsg_dump+0x9c/0xc0
[   19.480040]  [<ffffffff8105ca09>] oops_exit+0x29/0x30
[   19.480040]  [<ffffffff81624e72>] oops_end+0x72/0xe0
[   19.480040]  [<ffffffff810177d8>] die+0x58/0x90
[   19.480040]  [<ffffffff816249a2>] do_general_protection+0x162/0x170
[   19.480040]  [<ffffffff816242b5>] general_protection+0x25/0x30
[   19.480040]  [<ffffffff814d5664>] ? efivarfs_file_write+0x134/0x270
[   19.480040]  [<ffffffff814d5641>] ? efivarfs_file_write+0x111/0x270
[   19.480040]  [<ffffffff811908cc>] vfs_write+0xac/0x180
[   19.480040]  [<ffffffff81190bfa>] sys_write+0x4a/0x90
[   19.480040]  [<ffffffff8162bd69>] system_call_fastpath+0x16/0x1b
[   19.480040] Code: 4c 8b 4d c0 4d 8b 07 65 4c 03 04 25 c8 db 00 00 49 
8b 50 08 4d 8b 20 4d 85 e4 0f 84 9d 00 00 00 49 63 47 20 4d 8b 07 41 f6 
c0 0f <49> 8b 1c 04 0f 85 c7 00 00 00 48 8d 4a 01 4c 89 e0 65 49 0f c7
[   19.480040] RIP  [<ffffffff8117b543>] kmem_cache_alloc_trace+0x63/0x160
[   19.480040]  RSP <ffff880077fd19c8>
[   19.480040] CR2: 0000000000000100
[   19.539320] ---[ end trace 969a2b2b14764a39 ]---
(Kernel hangs)

--
Lingzhu Xiang

Re: General protection fault in efivarfs

[joeyli]

於 五，2012-12-21 於 19:05 +0800，Lingzhu Xiang 提到：
> The following reproducer triggers certain bugs in efivarfs_file_write.
> 
> #!/bin/bash
> p=/sys/firmware/efi/efivars
> mount -t efivarfs - $p
> cat $p/Lang-* >$p/test-12341234-1234-1234-1234-123412341234
> umount $p
> mount -t efivarfs - $p
> echo -en "\0\0\0\0" >$p/test-12341234-1234-1234-1234-123412341234 

The problem is check EFI_VARIABLE_MASK in efivars.c that is not enough
for deny use 0x00000000 attributes.

Per UEFI spec, runtime variable at least need has attributes
EFI_VARIABLE_BOOTSERVICE_ACCESS and EFI_VARIABLE_RUNTIME_ACCESS.
Otherwise UEFI BIOS will occur unexpected error.

Please try the following patch.


Thanks a lot!
Joey Lee


>From cb0775a36f4d80f9fe2f9afee40c8b7310cbac8a Mon Sep 17 00:00:00 2001
From: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Date: Mon, 24 Dec 2012 18:33:52 +0800
Subject: [PATCH] efivars: Check attributes of variable whan writing at least need to define bootservice and runtime access

The EFI variable filesystem used when system in runtime. The variable
that wes wrote by user space application at least need to define
EFI_VARIABLE_BOOTSERVICE_ACCESS and EFI_VARIABLE_RUNTIME_ACCESS in
attributes.

Cc: Gary Lin <glin-IBi9RG/b67k@public.gmane.org>
Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
---
 drivers/firmware/efivars.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 7b1c374..7aeb4a5 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -706,6 +706,10 @@ static ssize_t efivarfs_file_write(struct file *file,
 	if (attributes & ~(EFI_VARIABLE_MASK))
 		return -EINVAL;
 
+	if (!((attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS) &&
+		(attributes & EFI_VARIABLE_RUNTIME_ACCESS)))
+		return -EINVAL;
+
 	efivars = var->efivars;
 
 	/*
-- 
1.6.4.2

Re: General protection fault in efivarfs

[Lingzhu Xiang]

On 12/24/2012 07:00 PM, joeyli wrote:
> 於 五，2012-12-21 於 19:05 +0800，Lingzhu Xiang 提到：
>> The following reproducer triggers certain bugs in efivarfs_file_write.
>>
>> #!/bin/bash
>> p=/sys/firmware/efi/efivars
>> mount -t efivarfs - $p
>> cat $p/Lang-*>$p/test-12341234-1234-1234-1234-123412341234
>> umount $p
>> mount -t efivarfs - $p
>> echo -en "\0\0\0\0">$p/test-12341234-1234-1234-1234-123412341234
> 
> The problem is check EFI_VARIABLE_MASK in efivars.c that is not enough
> for deny use 0x00000000 attributes.
> 
> Per UEFI spec, runtime variable at least need has attributes
> EFI_VARIABLE_BOOTSERVICE_ACCESS and EFI_VARIABLE_RUNTIME_ACCESS.
> Otherwise UEFI BIOS will occur unexpected error.
> 
> Please try the following patch.

Thank you for your patch.

Per UEFI spec, echo -en "\0\0\0\0" should be equivalent to deleting.
This is what efivarfs_unlink is doing but I wanted to avoid its
underflowing when reproducing this.

This still reproduces the bug and passes the check in your patch:

echo -en "\x07\0\0\0" >$p/test-12341234-1234-1234-1234-123412341234

> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 7b1c374..7aeb4a5 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -706,6 +706,10 @@ static ssize_t efivarfs_file_write(struct file *file,
>   	if (attributes&  ~(EFI_VARIABLE_MASK))
>   		return -EINVAL;
> 
> +	if (!((attributes&  EFI_VARIABLE_BOOTSERVICE_ACCESS)&&
> +		(attributes&  EFI_VARIABLE_RUNTIME_ACCESS)))
> +		return -EINVAL;
> +
>   	efivars = var->efivars;

--
Lingzhu Xiang

Re: General protection fault in efivarfs

[joeyli]

於 二，2012-12-25 於 10:24 +0800，Lingzhu Xiang 提到：
> On 12/24/2012 07:00 PM, joeyli wrote:
> > 於 五，2012-12-21 於 19:05 +0800，Lingzhu Xiang 提到：
> >> The following reproducer triggers certain bugs in efivarfs_file_write.
> >>
> >> #!/bin/bash
> >> p=/sys/firmware/efi/efivars
> >> mount -t efivarfs - $p
> >> cat $p/Lang-*>$p/test-12341234-1234-1234-1234-123412341234
> >> umount $p
> >> mount -t efivarfs - $p
> >> echo -en "\0\0\0\0">$p/test-12341234-1234-1234-1234-123412341234
> > 
> > The problem is check EFI_VARIABLE_MASK in efivars.c that is not enough
> > for deny use 0x00000000 attributes.
> > 
> > Per UEFI spec, runtime variable at least need has attributes
> > EFI_VARIABLE_BOOTSERVICE_ACCESS and EFI_VARIABLE_RUNTIME_ACCESS.
> > Otherwise UEFI BIOS will occur unexpected error.
> > 
> > Please try the following patch.
> 
> Thank you for your patch.
> 
> Per UEFI spec, echo -en "\0\0\0\0" should be equivalent to deleting.

Per spec, the variable will be deleted when set DataSize to zero:

Unless the EFI_VARIABLE_APPEND_WRITE,
EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS, or
EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS attribute is set, a
size of zero causes the variable to be deleted.

But, about the attributes description of SetVariable, it said:

Attributes Attributes bitmask to set for the variable. Refer to the
GetVariable() function description.

In description of GetVariable(), it reference to the definitions of
"Variable Attributes", it said:

Any attempts to access a variable that does not have the attribute set
for runtime access will yield the EFI_NOT_FOUND error.


So, if doesn't set any attribute, then will receive EFI_NOT_FOUND. That
also means we should not allow 0x00000000 in SetVariable(). I still
think we should reply -EINVAL to user space application when they do not
set any attributes.

> This is what efivarfs_unlink is doing but I wanted to avoid its
> underflowing when reproducing this.
> 
> This still reproduces the bug and passes the check in your patch:
> 
> echo -en "\x07\0\0\0" >$p/test-12341234-1234-1234-1234-123412341234

I can NOT reproduce issue by feeding "\x07\0\0\0" to variable on my
system, the test variable was been deleted normally.

My 2 testing environment: 
	+ qemu-kvm with OVMF-0.1+r13902-1.1.x86_64 on openSUSE
	+ Intel DQ57TM board (Tunnel Mountain) with B.11 UEFI BIOS

Kernel version is:
	+ latest commit is 54e37b8dbe on Linus kernel tree
	  with
	  my patch for check attributes need define bootservice and runtime access
	  
I can not reproduce on OVMF and Intel DQ57TM board. After I delete
variable, I also umount/mount filesystem a couple of times and
write/delete again, didn't reproduce issue.

Maybe you can try v3.8-rc1 kernel.

> 
> > diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> > index 7b1c374..7aeb4a5 100644
> > --- a/drivers/firmware/efivars.c
> > +++ b/drivers/firmware/efivars.c
> > @@ -706,6 +706,10 @@ static ssize_t efivarfs_file_write(struct file *file,
> >   	if (attributes&  ~(EFI_VARIABLE_MASK))
> >   		return -EINVAL;
> > 
> > +	if (!((attributes&  EFI_VARIABLE_BOOTSERVICE_ACCESS)&&
> > +		(attributes&  EFI_VARIABLE_RUNTIME_ACCESS)))
> > +		return -EINVAL;
> > +
> >   	efivars = var->efivars;
> 
> --
> Lingzhu Xiang
> 

Thanks
Joey Lee

Re: General protection fault in efivarfs

[joeyli]

於 二，2012-12-25 於 12:13 +0800，joeyli 提到：
> 
> > This is what efivarfs_unlink is doing but I wanted to avoid its
> > underflowing when reproducing this.
> > 
> > This still reproduces the bug and passes the check in your patch:
> > 
> > echo -en "\x07\0\0\0" >$p/test-12341234-1234-1234-1234-123412341234
> 
> I can NOT reproduce issue by feeding "\x07\0\0\0" to variable on my
> system, the test variable was been deleted normally.
> 
> My 2 testing environment: 
>         + qemu-kvm with OVMF-0.1+r13902-1.1.x86_64 on openSUSE
>         + Intel DQ57TM board (Tunnel Mountain) with B.11 UEFI BIOS
> 
> Kernel version is:
>         + latest commit is 54e37b8dbe on Linus kernel tree
>           with
>           my patch for check attributes need define bootservice and
> runtime access
>           
> I can not reproduce on OVMF and Intel DQ57TM board. After I delete
> variable, I also umount/mount filesystem a couple of times and
> write/delete again, didn't reproduce issue.
> 
> Maybe you can try v3.8-rc1 kernel. 

hm... I just re-test and do more times, I also can reproduce on v3.8-rc1
and 54e37b8dbe branch now.


Thanks
Joey Lee

efivarfs: unlinking open files results in spinlock corruption

[Lingzhu Xiang]

On 12/26/2012 02:02 PM, joeyli wrote:
>> Maybe you can try v3.8-rc1 kernel.
>
> hm... I just re-test and do more times, I also can reproduce on v3.8-rc1
> and 54e37b8dbe branch now.

Good news you reproduce it. I manage to isolate the following reproducer.

This reproducer causes general protection fault, NULL dereference or just
hanging on QEMU/OVMF (OVMF-0.1+r13902-1.1) with 3.8-rc1 vanilla kernel.

With a logging point in efivarfs_file_write checking &efivars->lock, it
looks like the spinlock is corrupted before call trace kicks in.

Currently deletion with efivarfs_file_write just does the same thing of
unlinking an file while it's open.

Steps to reproduce:
$ gcc efivarfs-unlink-open-file.c -o efivarfs-unlink-open-file
# mount -t efivarfs - /sys/firmware/efi/efivars
# ./efivarfs-unlink-open-file
[   74.893152] BUG: unable to handle kernel NULL pointer dereference at 
           (null)
[   74.894131] IP: [<ffffffff81646b4e>] _raw_spin_lock+0xe/0x30
[   74.894131] PGD 78b3d067 PUD 3f91b067 PMD 0
[   74.894131] Oops: 0002 [#1] SMP
(...)

---
#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

char path[] = 
"/sys/firmware/efi/efivars/Lang-8be4df61-93ca-11d2-aa0d-00e098032b8c";
int main()
{
	int fd;
	fd = open(path, O_RDONLY);
	if (fd < 0) {
		perror("open");
		return 1;
	}
	if (unlink(path) < 0) {
		perror("unlink");
		return 1;
	}
	if (read(fd, NULL, 0) < 0) {
		perror("read");
		return 1;
	}
	return 0;
}

Re: [PATCH 17/20] efivarfs: Replace magic number with sizeof(attributes)

[Lingzhu Xiang]

On 10/26/2012 03:52 PM, Matt Fleming wrote:
> From: Matt Fleming<matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> Seeing "+ 4" littered throughout the functions gets a bit
> confusing. Use "sizeof(attributes)" which clearly explains what
> quantity we're adding.
>
> Acked-by: Jeremy Kerr<jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> Signed-off-by: Matt Fleming<matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Looks like one +4 got left behind, in efivarfs_fill_super

		i_size_write(inode, size+4);


--
Lingzhu Xiang

Re: efivarfs: unlinking open files results in spinlock corruption

[joeyli]

於 三，2012-12-26 於 17:21 +0800，Lingzhu Xiang 提到：
> On 12/26/2012 02:02 PM, joeyli wrote:
> >> Maybe you can try v3.8-rc1 kernel.
> >
> > hm... I just re-test and do more times, I also can reproduce on
> v3.8-rc1
> > and 54e37b8dbe branch now.
> 
> Good news you reproduce it. I manage to isolate the following
> reproducer.
> 
> This reproducer causes general protection fault, NULL dereference or
> just
> hanging on QEMU/OVMF (OVMF-0.1+r13902-1.1) with 3.8-rc1 vanilla
> kernel.
> 
> With a logging point in efivarfs_file_write checking &efivars->lock,
> it
> looks like the spinlock is corrupted before call trace kicks in.
> 
> Currently deletion with efivarfs_file_write just does the same thing
> of
> unlinking an file while it's open.
> 
> Steps to reproduce:
> $ gcc efivarfs-unlink-open-file.c -o efivarfs-unlink-open-file
> # mount -t efivarfs - /sys/firmware/efi/efivars
> # ./efivarfs-unlink-open-file
> [   74.893152] BUG: unable to handle kernel NULL pointer dereference
> at 
>            (null)
> [   74.894131] IP: [<ffffffff81646b4e>] _raw_spin_lock+0xe/0x30
> [   74.894131] PGD 78b3d067 PUD 3f91b067 PMD 0
> [   74.894131] Oops: 0002 [#1] SMP
> (...) 

I am checking the write/umount/mount/read problem on v3.8-rc1.

The issue also can trigger by read the GHOST test file. When issue
happen, the size of test file is zero. The 'var->efivars' already set to
NULL when issue reproduced, that's why we always got NULL pointer
dereference when try to spin lock it.

If we don't do umount/mount step, then everything is OK for we can
create/delete test file many times. But, if we umount/mount efivarfs
then more easy to trigger issue.

When issue happen, inode of test file didn't removed but efivars already
set to NULL. I think there have race condition between maintain efivars
and vars. Still tracing...


Thanks a lot!
Joey Lee

File lingers after deletion with efivarfs_write_file

[Lingzhu Xiang]

On 10/26/2012 03:51 PM, Matt Fleming wrote:
> From: Jeremy Kerr<jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>
> A write to an efivarfs file will not always result in a variable of
> 'count' size after the EFI SetVariable() call. We may have appended to
> the existing data (ie, with the EFI_VARIABLE_APPEND_WRITE attribute), or
> even have deleted the variable (with an authenticated variable update,
> with a zero datasize).
>
> This change re-reads the updated variable from firmware, to check for
> size changes and deletions. In the latter case, we need to drop the
> dentry.

File lingers after deletion with efivarfs_write_file.

Reproduced on QEMU/OVMF, 3.8-rc vanilla kernel.

Choosing the variable RTC because it's been generated by OVMF when
mounting.

Steps to reproduce (bash commandline):
# P=/sys/firmware/efi/efivars
# VAR=$P/RTC-378d7b65-8da9-4773-b6e4-a47826a833e1
# mount -t efivarfs - $P
# stat -c'%s %h' $VAR
8 1
# dd if=/dev/zero of=$VAR bs=4 count=1 conv=notrunc 2>&-
# stat -c'%s %h' $VAR
8 0

Writing four null bytes to $VAR should delete it.

--
Lingzhu Xiang

Re: efivarfs: unlinking open files results in spinlock corruption

[Lingzhu Xiang]

On 12/26/2012 06:16 PM, joeyli wrote:
> I am checking the write/umount/mount/read problem on v3.8-rc1.
>
> The issue also can trigger by read the GHOST test file. When issue

True. But this reproducer is meant to be isolated from efivarfs_file_write
code path and reproduce a general locking problem.

I separately report the ghost file problem in 
http://thread.gmane.org/gmane.linux.kernel.efi/465

> happen, the size of test file is zero. The 'var->efivars' already set to
> NULL when issue reproduced, that's why we always got NULL pointer
> dereference when try to spin lock it.

efivars comes from private data of the file when it has been released.
It can be some random value instead of NULL, so we see the varying behavior.

> If we don't do umount/mount step, then everything is OK for we can
> create/delete test file many times. But, if we umount/mount efivarfs
> then more easy to trigger issue.

I confirm this.

--
Lingzhu Xiang

Re: [PATCH 01/20] efi: Add support for a UEFI variable filesystem

[Matt Fleming]

On Fri, 2012-12-21 at 13:54 +0800, Lingzhu Xiang wrote:
> On 10/26/2012 03:51 PM, Matt Fleming wrote:
> > +static int efivarfs_unlink(struct inode *dir, struct dentry *dentry)
> > +{
> > +	struct efivar_entry *var = dentry->d_inode->i_private;
> > +	struct efivars *efivars = var->efivars;
> > +	efi_status_t status;
> > +
> > +	spin_lock(&efivars->lock);
> > +
> > +	status = efivars->ops->set_variable(var->var.VariableName,
> > +					&var->var.VendorGuid,
> > +					    0, 0, NULL);
> > +
> > +	if (status == EFI_SUCCESS || status == EFI_NOT_FOUND) {
> > +		list_del(&var->list);
> > +		spin_unlock(&efivars->lock);
> > +		efivar_unregister(var);
> > +		drop_nlink(dir);
> 
> This should be  drop_nlink(dentry->d_inode);

Yep, this looks right. Care to send a proper patch with a Signed-off-by?

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH] efivarfs: Drop link count of the right inode

[Lingzhu Xiang]

efivarfs_unlink() should drop the file's link count, not the directory's.

Signed-off-by: Lingzhu Xiang <lxiang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/firmware/efivars.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index d6b8d2f..60f5324 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -995,7 +995,7 @@ static int efivarfs_unlink(struct inode *dir, struct dentry *dentry)
 		list_del(&var->list);
 		spin_unlock(&efivars->lock);
 		efivar_unregister(var);
-		drop_nlink(dir);
+		drop_nlink(dentry->d_inode);
 		dput(dentry);
 		return 0;
 	}
-- 
1.7.7.6

Re: [PATCH] efivarfs: Drop link count of the right inode

[joeyli]

於 六，2013-01-05 於 13:59 +0800，Lingzhu Xiang 提到：
> efivarfs_unlink() should drop the file's link count, not the directory's.
> 
> Signed-off-by: Lingzhu Xiang <lxiang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Tested-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>


Thanks
Joey Lee

> ---
>  drivers/firmware/efivars.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index d6b8d2f..60f5324 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -995,7 +995,7 @@ static int efivarfs_unlink(struct inode *dir, struct dentry *dentry)
>  		list_del(&var->list);
>  		spin_unlock(&efivars->lock);
>  		efivar_unregister(var);
> -		drop_nlink(dir);
> +		drop_nlink(dentry->d_inode);
>  		dput(dentry);
>  		return 0;
>  	}

Re: [PATCH] efivarfs: Drop link count of the right inode

[Matt Fleming]

On Sat, 2013-01-05 at 13:59 +0800, Lingzhu Xiang wrote:
> efivarfs_unlink() should drop the file's link count, not the directory's.
> 
> Signed-off-by: Lingzhu Xiang <lxiang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/firmware/efivars.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Thanks, I've queued this up with Joey's Tested-by.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: General protection fault in efivarfs

[Matt Fleming]

On Mon, 2012-12-24 at 19:00 +0800, joeyli wrote:
> 於 五，2012-12-21 於 19:05 +0800，Lingzhu Xiang 提到：
> > The following reproducer triggers certain bugs in efivarfs_file_write.
> > 
> > #!/bin/bash
> > p=/sys/firmware/efi/efivars
> > mount -t efivarfs - $p
> > cat $p/Lang-* >$p/test-12341234-1234-1234-1234-123412341234
> > umount $p
> > mount -t efivarfs - $p
> > echo -en "\0\0\0\0" >$p/test-12341234-1234-1234-1234-123412341234 

Thanks for the report!

> The problem is check EFI_VARIABLE_MASK in efivars.c that is not enough
> for deny use 0x00000000 attributes.

Not quite, see below.

> Per UEFI spec, runtime variable at least need has attributes
> EFI_VARIABLE_BOOTSERVICE_ACCESS and EFI_VARIABLE_RUNTIME_ACCESS.
> Otherwise UEFI BIOS will occur unexpected error.
> 
> Please try the following patch.
> 
> 
> Thanks a lot!
> Joey Lee
> 
> 
> >From cb0775a36f4d80f9fe2f9afee40c8b7310cbac8a Mon Sep 17 00:00:00 2001
> From: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
> Date: Mon, 24 Dec 2012 18:33:52 +0800
> Subject: [PATCH] efivars: Check attributes of variable whan writing at least need to define bootservice and runtime access
> 
> The EFI variable filesystem used when system in runtime. The variable
> that wes wrote by user space application at least need to define
> EFI_VARIABLE_BOOTSERVICE_ACCESS and EFI_VARIABLE_RUNTIME_ACCESS in
> attributes.
> 
> Cc: Gary Lin <glin-IBi9RG/b67k@public.gmane.org>
> Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
> ---
>  drivers/firmware/efivars.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 7b1c374..7aeb4a5 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -706,6 +706,10 @@ static ssize_t efivarfs_file_write(struct file *file,
>  	if (attributes & ~(EFI_VARIABLE_MASK))
>  		return -EINVAL;
>  
> +	if (!((attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS) &&
> +		(attributes & EFI_VARIABLE_RUNTIME_ACCESS)))
> +		return -EINVAL;
> +
>  	efivars = var->efivars;
>  
>  	/*

This breaks the case where we want to create a variable that only has
boot time access. It's not inconceivable that someone may want to do
this from Linux. What you should be enforcing is that if
EFI_VARIABLE_RUNTIME_ACCESS is set, then EFI_VARIABLE_BOOTSERVICE_ACCESS
must be also. Can you re-work your patch to handle this case? Preferably
with a comment above the check as for why we have this check, i.e. it's
in the spec.

Regarding zero attributes: if user tools want to delete a variable by
using the zero attribute method, that's fine - we should handle that.
The real problem here is that if we try to delete a non-existent
variable or even if *creating* a variable fails, we don't delete the
file in the file system.

Thanks for the report guys, I'll cook up a patch.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 16/20] efivarfs: Return an error if we fail to read a variable

[Matt Fleming]

On Fri, 2012-12-21 at 15:08 +0800, Lingzhu Xiang wrote:
> On 11/02/2012 04:53 PM, Matt Fleming wrote:
> > Instead of always returning 0 in efivarfs_file_read(), even when we
> > fail to successfully read the variable, convert the EFI status to
> > something meaningful and return that to the caller. This way the user
> > will have some hint as to why the read failed.
> > ...
> >
> > +	case EFI_NOT_FOUND:
> > +		err = -ENOENT;
> 
> This will be ambiguous for userspace. Is it file not being found by
> efivarfs or by firmware?
> 
> Actually I've been confused during testing like this:
> 
> # ll test-12341234-1234-1234-1234-123412341234
> ls: cannot access test-12341234-1234-1234-1234-123412341234: No such 
> file or directory
> # echo -en "\0\0\0\0a" >test-12341234-1234-1234-1234-123412341234
> -bash: echo: write error: No such file or directory
> # ll test-12341234-1234-1234-1234-123412341234
> -rw-r--r--. 1 root root 0 Dec 21 06:19 
> test-12341234-1234-1234-1234-123412341234

Yeah, this file shouldn't exist. I'm working on a patch. Thanks!

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 17/20] efivarfs: Replace magic number with sizeof(attributes)

[Matt Fleming]

On Wed, 2012-12-26 at 17:24 +0800, Lingzhu Xiang wrote:
> On 10/26/2012 03:52 PM, Matt Fleming wrote:
> > From: Matt Fleming<matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >
> > Seeing "+ 4" littered throughout the functions gets a bit
> > confusing. Use "sizeof(attributes)" which clearly explains what
> > quantity we're adding.
> >
> > Acked-by: Jeremy Kerr<jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> > Signed-off-by: Matt Fleming<matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Looks like one +4 got left behind, in efivarfs_fill_super
> 
> 		i_size_write(inode, size+4);

Yep, so it does. I'll cook something up, thanks.

-- 
Matt Fleming, Intel Open Source Technology Center

efivarfs allows non-canonical GUID and duplicate filenames

[Lingzhu Xiang]

On 10/26/2012 03:51 PM, Matt Fleming wrote:
> +static void efivarfs_hex_to_guid(const char *str, efi_guid_t *guid)
> +{
> +	guid->b[0] = hex_to_bin(str[6]) << 4 | hex_to_bin(str[7]);
> +	guid->b[1] = hex_to_bin(str[4]) << 4 | hex_to_bin(str[5]);
> +	guid->b[2] = hex_to_bin(str[2]) << 4 | hex_to_bin(str[3]);
> +	guid->b[3] = hex_to_bin(str[0]) << 4 | hex_to_bin(str[1]);
> +	guid->b[4] = hex_to_bin(str[11]) << 4 | hex_to_bin(str[12]);
> +	guid->b[5] = hex_to_bin(str[9]) << 4 | hex_to_bin(str[10]);
> +	guid->b[6] = hex_to_bin(str[16]) << 4 | hex_to_bin(str[17]);
> +	guid->b[7] = hex_to_bin(str[14]) << 4 | hex_to_bin(str[15]);
> +	guid->b[8] = hex_to_bin(str[19]) << 4 | hex_to_bin(str[20]);
> +	guid->b[9] = hex_to_bin(str[21]) << 4 | hex_to_bin(str[22]);
> +	guid->b[10] = hex_to_bin(str[24]) << 4 | hex_to_bin(str[25]);
> +	guid->b[11] = hex_to_bin(str[26]) << 4 | hex_to_bin(str[27]);
> +	guid->b[12] = hex_to_bin(str[28]) << 4 | hex_to_bin(str[29]);
> +	guid->b[13] = hex_to_bin(str[30]) << 4 | hex_to_bin(str[31]);
> +	guid->b[14] = hex_to_bin(str[32]) << 4 | hex_to_bin(str[33]);
> +	guid->b[15] = hex_to_bin(str[34]) << 4 | hex_to_bin(str[35]);
> +}
> +
> +static int efivarfs_create(struct inode *dir, struct dentry *dentry,
> +			  umode_t mode, bool excl)
> +{
> +	struct inode *inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
> +	struct efivars *efivars = &__efivars;
> +	struct efivar_entry *var;
> +	int namelen, i = 0, err = 0;
> +
> +	if (dentry->d_name.len < 38)
> +		return -EINVAL;
> +
> +	if (!inode)
> +		return -ENOSPC;
> +
> +	var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
> +
> +	if (!var)
> +		return -ENOMEM;
> +
> +	namelen = dentry->d_name.len - GUID_LEN;
> +
> +	efivarfs_hex_to_guid(dentry->d_name.name + namelen + 1,
> +			&var->var.VendorGuid);

efivarfs_hex_to_guid does not check filename validity. 

[root@qemu-ovmf efivars]# touch test-------------------------------------
[root@qemu-ovmf efivars]# ll test*
-rw-r--r--. 1 root root 0 Jan 25 14:57 test-------------------------------------
[root@qemu-ovmf efivars]# cd ..; umount efivars; mount -t efivarfs - efivars; cd -
/sys/firmware/efi/efivars
[root@qemu-ovmf efivars]# ll test*
-rw-r--r--. 1 root root 4 Jan 25 14:57 test-ffffffff-ffff-ffff-ffff-ffffffffffff

[root@qemu-ovmf efivars]# touch BootOrder-8BE4DF61-93CA-11D2-AA0D-00E098032B8C
[root@qemu-ovmf efivars]# ll BootOrder*
-rw-r--r--. 1 root root 12 Jan 25 14:49 BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c
-rw-r--r--. 1 root root  0 Jan 25 14:55 BootOrder-8BE4DF61-93CA-11D2-AA0D-00E098032B8C
[root@qemu-ovmf efivars]# cd ..; umount efivars; mount -t efivarfs - efivars; cd -
/sys/firmware/efi/efivars
[root@qemu-ovmf efivars]# ll BootOrder*
-rw-r--r--. 1 root root 12 Jan 25 14:56 BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c
-rw-r--r--. 1 root root 12 Jan 25 14:56 BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c


--
Lingzhu Xiang

Re: efivarfs allows non-canonical GUID and duplicate filenames

[Matthew Garrett]

On Fri, Jan 25, 2013 at 03:01:43PM +0800, Lingzhu Xiang wrote:

> efivarfs_hex_to_guid does not check filename validity. 

Yeah, that's a problem.

> [root@qemu-ovmf efivars]# touch BootOrder-8BE4DF61-93CA-11D2-AA0D-00E098032B8C

Ha ok. Other than the change in name over unmount, does this one cause 
any problems?

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: efivarfs allows non-canonical GUID and duplicate filenames

[Lingzhu Xiang]

On 01/29/2013 12:44 PM, Matthew Garrett wrote:
>> [root@qemu-ovmf efivars]# touch BootOrder-8BE4DF61-93CA-11D2-AA0D-00E098032B8C
>
> Ha ok. Other than the change in name over unmount, does this one cause
> any problems?

This demonstrates how to create two files with the same name in efivarfs.

[root@qemu-ovmf efivars]# ll BootOrder*
-rw-r--r--. 1 root root 12 Jan 25 14:56 
BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c
-rw-r--r--. 1 root root 12 Jan 25 14:56 
BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c

Re: efivarfs allows non-canonical GUID and duplicate filenames

[Matthew Garrett]

On Tue, Jan 29, 2013 at 01:17:10PM +0800, Lingzhu Xiang wrote:
> On 01/29/2013 12:44 PM, Matthew Garrett wrote:
> >>[root@qemu-ovmf efivars]# touch BootOrder-8BE4DF61-93CA-11D2-AA0D-00E098032B8C
> >
> >Ha ok. Other than the change in name over unmount, does this one cause
> >any problems?
> 
> This demonstrates how to create two files with the same name in efivarfs.
> 
> [root@qemu-ovmf efivars]# ll BootOrder*
> -rw-r--r--. 1 root root 12 Jan 25 14:56
> BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c
> -rw-r--r--. 1 root root 12 Jan 25 14:56
> BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c

Huh. The firmware permits two variables with the same GUID in different 
cases?

-- 
Matthew Garrett | mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org

Re: efivarfs allows non-canonical GUID and duplicate filenames

[Lingzhu Xiang]

On 01/29/2013 01:25 PM, Matthew Garrett wrote:
> On Tue, Jan 29, 2013 at 01:17:10PM +0800, Lingzhu Xiang wrote:
>> On 01/29/2013 12:44 PM, Matthew Garrett wrote:
>>>> [root@qemu-ovmf efivars]# touch BootOrder-8BE4DF61-93CA-11D2-AA0D-00E098032B8C
>>>
>>> Ha ok. Other than the change in name over unmount, does this one cause
>>> any problems?
>>
>> This demonstrates how to create two files with the same name in efivarfs.
>>
>> [root@qemu-ovmf efivars]# ll BootOrder*
>> -rw-r--r--. 1 root root 12 Jan 25 14:56
>> BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c
>> -rw-r--r--. 1 root root 12 Jan 25 14:56
>> BootOrder-8be4df61-93ca-11d2-aa0d-00e098032b8c
>
> Huh. The firmware permits two variables with the same GUID in different
> cases?

The two files points to the same variable.

So I create a file with upper case GUID string, but the upper and lower case
are parsed to be the same binary value and saved separately in sysfs. During
remount, the binary GUID is formatted to lower case, so the upper and lower
case sysfs entries come back to efivarfs with the same name.

This is just trying to explore the impact of not having GUID validation for
filename. Not something new.

Re: efivarfs allows non-canonical GUID and duplicate filenames

[Matt Fleming]

On Tue, 2013-01-29 at 13:46 +0800, Lingzhu Xiang wrote:
> So I create a file with upper case GUID string, but the upper and lower case
> are parsed to be the same binary value and saved separately in sysfs. During
> remount, the binary GUID is formatted to lower case, so the upper and lower
> case sysfs entries come back to efivarfs with the same name.
> 
> This is just trying to explore the impact of not having GUID validation for
> filename. Not something new.

There are two problems with the way filenames are validated currently.
We don't validate the GUID portion at all, which was demonstrated when
you created a file in efivarfs with a filename of,

	test-------------------------------------

Also, we're using the simple dentry operations which has a
case-sensitive d_compare. While the first portion of the filename, the
variable name, is case-sensitive, the GUID part isn't. And that's the
bug that allowed you to create two files with the same GUID, one
uppercase, one lowercase.

I'll take a look at fixing this. Thanks for the report.

-- 
Matt Fleming, Intel Open Source Technology Center