Update UEFI variable support

Update UEFI variable support

[Matthew Garrett]

Our EFI variable support is artifically limited at the moment. This patchset
adds support for arbitrary variable sizes, limited only by what the platform
supports. It also fixes a case where we were unnecessarily fiddling with
sysfs while in the process of shutting down or crashing, which triggered
some other issues.

[PATCH 1/4] uefi: Populate runtime_version

[Matthew Garrett]

The runtime_version field isn't getting populated, causing various UEFI
calls to fail. Fix.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 arch/x86/platform/efi/efi.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 37718f0..4294cb5 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -552,6 +552,8 @@ void __init efi_init(void)
 		 * virtual mode.
 		 */
 		efi.get_time = phys_efi_get_time;
+
+		efi.runtime_version = runtime->hdr.revision;
 	} else
 		printk(KERN_ERR "Could not map the EFI runtime service "
 		       "table!\n");
-- 
1.7.7.1

[PATCH 2/4] efi: Only create sysfs entries while booting or running

[Matthew Garrett]

There's no point in creating sysfs entries while the machine's on its way
down. Skip it in that case in order to avoid complexity.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/firmware/efivars.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index b0a8117..eb07f13 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -552,11 +552,18 @@ static int efi_pstore_write(enum pstore_type_id type, u64 *id,
 	if (found)
 		efivar_unregister(found);
 
-	if (size)
-		ret = efivar_create_sysfs_entry(efivars,
-					  utf16_strsize(efi_name,
-							DUMP_NAME_LEN * 2),
-					  efi_name, &vendor);
+	if (size) {
+		switch (system_state) {
+		case SYSTEM_BOOTING:
+		case SYSTEM_RUNNING:
+			ret = efivar_create_sysfs_entry(efivars,
+							utf16_strsize(efi_name,
+							    DUMP_NAME_LEN * 2),
+							efi_name, &vendor);
+		default:
+			break;
+		}
+	}
 
 	*id = part;
 	return ret;
-- 
1.7.7.1

[PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[Matthew Garrett]

The EFI variables code has a hard limit of 1024 bytes for variable length.
This restriction only existed in version 0.99 of the EFI specification,
and is not relevant on any existing hardware. Add support for a longer
structure that incorporates the existing one, allowing old userspace to
carry on working while allowing newer userspace to write larger variables
via the same interface.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/firmware/efivars.c |  240 ++++++++++++++++++++++++++++----------------
 1 files changed, 154 insertions(+), 86 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index eb07f13..1bef80c 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -92,13 +92,6 @@ MODULE_VERSION(EFIVARS_VERSION);
 
 #define DUMP_NAME_LEN 52
 
-/*
- * The maximum size of VariableName + Data = 1024
- * Therefore, it's reasonable to save that much
- * space in each part of the structure,
- * and we use a page for reading/writing.
- */
-
 struct efi_variable {
 	efi_char16_t  VariableName[1024/sizeof(efi_char16_t)];
 	efi_guid_t    VendorGuid;
@@ -108,10 +101,20 @@ struct efi_variable {
 	__u32         Attributes;
 } __attribute__((packed));
 
+/*
+ * Older versions of this driver had a fixed 1024-byte buffer. We need to
+ * maintain compatibility with old userspace, so we provide an extended
+ * structure to allow userspace to write larger variables
+ */
+
+struct extended_efi_variable {
+	struct efi_variable header;
+	__u8 Data[0];
+} __attribute__((packed));
 
 struct efivar_entry {
 	struct efivars *efivars;
-	struct efi_variable var;
+	struct extended_efi_variable *var;
 	struct list_head list;
 	struct kobject kobj;
 };
@@ -192,21 +195,41 @@ utf16_strncmp(const efi_char16_t *a, const efi_char16_t *b, size_t len)
 }
 
 static efi_status_t
-get_var_data_locked(struct efivars *efivars, struct efi_variable *var)
+get_var_data_locked(struct efivars *efivars, struct extended_efi_variable **var)
 {
 	efi_status_t status;
+	unsigned long length;
+
+	if (!*var)
+		*var = kmalloc(sizeof(struct extended_efi_variable), GFP_KERNEL);
+
+	(*var)->header.DataSize = 0;
+	status = efivars->ops->get_variable((*var)->header.VariableName,
+					    &(*var)->header.VendorGuid,
+					    &(*var)->header.Attributes,
+					    &(*var)->header.DataSize,
+					    (*var)->Data);
+
+	if (status == EFI_BUFFER_TOO_SMALL) {
+		*var = krealloc(*var, sizeof(struct extended_efi_variable) +
+				(*var)->header.DataSize, GFP_KERNEL);
+		status = efivars->ops->get_variable((*var)->header.VariableName,
+						    &(*var)->header.VendorGuid,
+						    &(*var)->header.Attributes,
+						    &(*var)->header.DataSize,
+						    (*var)->Data);
+	}
+
+	length = ((*var)->header.DataSize < 1024) ? (*var)->header.DataSize :
+		1024;
+
+	memcpy(&(*var)->header.Data, &(*var)->Data, length);
 
-	var->DataSize = 1024;
-	status = efivars->ops->get_variable(var->VariableName,
-					    &var->VendorGuid,
-					    &var->Attributes,
-					    &var->DataSize,
-					    var->Data);
 	return status;
 }
 
 static efi_status_t
-get_var_data(struct efivars *efivars, struct efi_variable *var)
+get_var_data(struct efivars *efivars, struct extended_efi_variable **var)
 {
 	efi_status_t status;
 
@@ -224,13 +247,13 @@ get_var_data(struct efivars *efivars, struct efi_variable *var)
 static ssize_t
 efivar_guid_read(struct efivar_entry *entry, char *buf)
 {
-	struct efi_variable *var = &entry->var;
+	struct extended_efi_variable *var = entry->var;
 	char *str = buf;
 
 	if (!entry || !buf)
 		return 0;
 
-	efi_guid_unparse(&var->VendorGuid, str);
+	efi_guid_unparse(&var->header.VendorGuid, str);
 	str += strlen(str);
 	str += sprintf(str, "\n");
 
@@ -240,22 +263,22 @@ efivar_guid_read(struct efivar_entry *entry, char *buf)
 static ssize_t
 efivar_attr_read(struct efivar_entry *entry, char *buf)
 {
-	struct efi_variable *var = &entry->var;
+	struct extended_efi_variable *var = entry->var;
 	char *str = buf;
 	efi_status_t status;
 
 	if (!entry || !buf)
 		return -EINVAL;
 
-	status = get_var_data(entry->efivars, var);
+	status = get_var_data(entry->efivars, &var);
 	if (status != EFI_SUCCESS)
 		return -EIO;
 
-	if (var->Attributes & 0x1)
+	if (var->header.Attributes & 0x1)
 		str += sprintf(str, "EFI_VARIABLE_NON_VOLATILE\n");
-	if (var->Attributes & 0x2)
+	if (var->header.Attributes & 0x2)
 		str += sprintf(str, "EFI_VARIABLE_BOOTSERVICE_ACCESS\n");
-	if (var->Attributes & 0x4)
+	if (var->header.Attributes & 0x4)
 		str += sprintf(str, "EFI_VARIABLE_RUNTIME_ACCESS\n");
 	return str - buf;
 }
@@ -263,36 +286,36 @@ efivar_attr_read(struct efivar_entry *entry, char *buf)
 static ssize_t
 efivar_size_read(struct efivar_entry *entry, char *buf)
 {
-	struct efi_variable *var = &entry->var;
+	struct extended_efi_variable *var = entry->var;
 	char *str = buf;
 	efi_status_t status;
 
 	if (!entry || !buf)
 		return -EINVAL;
 
-	status = get_var_data(entry->efivars, var);
+	status = get_var_data(entry->efivars, &var);
 	if (status != EFI_SUCCESS)
 		return -EIO;
 
-	str += sprintf(str, "0x%lx\n", var->DataSize);
+	str += sprintf(str, "0x%lx\n", var->header.DataSize);
 	return str - buf;
 }
 
 static ssize_t
 efivar_data_read(struct efivar_entry *entry, char *buf)
 {
-	struct efi_variable *var = &entry->var;
+	struct extended_efi_variable *var = entry->var;
 	efi_status_t status;
 
 	if (!entry || !buf)
 		return -EINVAL;
 
-	status = get_var_data(entry->efivars, var);
+	status = get_var_data(entry->efivars, &var);
 	if (status != EFI_SUCCESS)
 		return -EIO;
 
-	memcpy(buf, var->Data, var->DataSize);
-	return var->DataSize;
+	memcpy(buf, var->Data, var->header.DataSize);
+	return var->header.DataSize;
 }
 /*
  * We allow each variable to be edited via rewriting the
@@ -301,34 +324,46 @@ efivar_data_read(struct efivar_entry *entry, char *buf)
 static ssize_t
 efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
 {
-	struct efi_variable *new_var, *var = &entry->var;
+	struct extended_efi_variable *new_var, *var = entry->var;
+	struct efi_variable *tmp_var = NULL;
 	struct efivars *efivars = entry->efivars;
 	efi_status_t status = EFI_NOT_FOUND;
+	int error = 0;
 
-	if (count != sizeof(struct efi_variable))
+	if (count == sizeof(struct efi_variable)) {
+		tmp_var = (struct efi_variable *)buf;
+		new_var = kmalloc(sizeof(struct efi_variable) +
+				  tmp_var->DataSize, GFP_KERNEL);
+		memcpy(&new_var->header, tmp_var, sizeof(struct efi_variable));
+		memcpy(&new_var->Data, tmp_var->Data, tmp_var->DataSize);
+	} else if (count > sizeof(struct efi_variable)) {
+		new_var = (struct extended_efi_variable *)buf;
+	} else {
 		return -EINVAL;
+	}
 
-	new_var = (struct efi_variable *)buf;
 	/*
 	 * If only updating the variable data, then the name
 	 * and guid should remain the same
 	 */
-	if (memcmp(new_var->VariableName, var->VariableName, sizeof(var->VariableName)) ||
-		efi_guidcmp(new_var->VendorGuid, var->VendorGuid)) {
+	if (memcmp(new_var->header.VariableName, var->header.VariableName, sizeof(var->header.VariableName)) ||
+		efi_guidcmp(new_var->header.VendorGuid, var->header.VendorGuid)) {
 		printk(KERN_ERR "efivars: Cannot edit the wrong variable!\n");
-		return -EINVAL;
+		error = -EINVAL;
+		goto out;
 	}
 
-	if ((new_var->DataSize <= 0) || (new_var->Attributes == 0)){
+	if ((new_var->header.DataSize <= 0) || (new_var->header.Attributes == 0)) {
 		printk(KERN_ERR "efivars: DataSize & Attributes must be valid!\n");
-		return -EINVAL;
+		error = -EINVAL;
+		goto out;
 	}
 
 	spin_lock(&efivars->lock);
-	status = efivars->ops->set_variable(new_var->VariableName,
-					    &new_var->VendorGuid,
-					    new_var->Attributes,
-					    new_var->DataSize,
+	status = efivars->ops->set_variable(new_var->header.VariableName,
+					    &new_var->header.VendorGuid,
+					    new_var->header.Attributes,
+					    new_var->header.DataSize,
 					    new_var->Data);
 
 	spin_unlock(&efivars->lock);
@@ -336,28 +371,37 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
 	if (status != EFI_SUCCESS) {
 		printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n",
 			status);
-		return -EIO;
+		error = -EIO;
+		goto out;
 	}
 
-	memcpy(&entry->var, new_var, count);
+	memcpy(entry->var, new_var, count);
+out:
+	/* Free the extended structure if we needed to allocate it */
+	if (tmp_var && new_var)
+		kfree(new_var);
+
+	if (error)
+		return error;
+
 	return count;
 }
 
 static ssize_t
 efivar_show_raw(struct efivar_entry *entry, char *buf)
 {
-	struct efi_variable *var = &entry->var;
+	struct extended_efi_variable *var = entry->var;
 	efi_status_t status;
 
 	if (!entry || !buf)
 		return 0;
 
-	status = get_var_data(entry->efivars, var);
+	status = get_var_data(entry->efivars, &var);
 	if (status != EFI_SUCCESS)
 		return -EIO;
 
-	memcpy(buf, var, sizeof(*var));
-	return sizeof(*var);
+	memcpy(buf, var, sizeof(*var) + var->header.DataSize);
+	return sizeof(*var) + var->header.DataSize;
 }
 
 /*
@@ -404,6 +448,7 @@ static const struct sysfs_ops efivar_attr_ops = {
 static void efivar_release(struct kobject *kobj)
 {
 	struct efivar_entry *var = container_of(kobj, struct efivar_entry, kobj);
+	kfree(var->var);
 	kfree(var);
 }
 
@@ -468,21 +513,21 @@ static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
 	unsigned long time;
 
 	while (&efivars->walk_entry->list != &efivars->list) {
-		if (!efi_guidcmp(efivars->walk_entry->var.VendorGuid,
+		if (!efi_guidcmp(efivars->walk_entry->var->header.VendorGuid,
 				 vendor)) {
 			for (i = 0; i < DUMP_NAME_LEN; i++) {
-				name[i] = efivars->walk_entry->var.VariableName[i];
+				name[i] = efivars->walk_entry->var->header.VariableName[i];
 			}
 			if (sscanf(name, "dump-type%u-%u-%lu", type, &part, &time) == 3) {
 				*id = part;
 				timespec->tv_sec = time;
 				timespec->tv_nsec = 0;
 				get_var_data_locked(efivars, &efivars->walk_entry->var);
-				size = efivars->walk_entry->var.DataSize;
+				size = efivars->walk_entry->var->header.DataSize;
 				*buf = kmalloc(size, GFP_KERNEL);
 				if (*buf == NULL)
 					return -ENOMEM;
-				memcpy(*buf, efivars->walk_entry->var.Data,
+				memcpy(*buf, efivars->walk_entry->var->Data,
 				       size);
 				efivars->walk_entry = list_entry(efivars->walk_entry->list.next,
 					           struct efivar_entry, list);
@@ -521,19 +566,19 @@ static int efi_pstore_write(enum pstore_type_id type, u64 *id,
 	list_for_each_entry(entry, &efivars->list, list) {
 		get_var_data_locked(efivars, &entry->var);
 
-		if (efi_guidcmp(entry->var.VendorGuid, vendor))
+		if (efi_guidcmp(entry->var->header.VendorGuid, vendor))
 			continue;
-		if (utf16_strncmp(entry->var.VariableName, efi_name,
+		if (utf16_strncmp(entry->var->header.VariableName, efi_name,
 				  utf16_strlen(efi_name)))
 			continue;
 		/* Needs to be a prefix */
-		if (entry->var.VariableName[utf16_strlen(efi_name)] == 0)
+		if (entry->var->header.VariableName[utf16_strlen(efi_name)] == 0)
 			continue;
 
 		/* found */
 		found = entry;
-		efivars->ops->set_variable(entry->var.VariableName,
-					   &entry->var.VendorGuid,
+		efivars->ops->set_variable(entry->var->header.VariableName,
+					   &entry->var->header.VendorGuid,
 					   PSTORE_EFI_ATTRIBUTES,
 					   0, NULL);
 	}
@@ -552,7 +597,7 @@ static int efi_pstore_write(enum pstore_type_id type, u64 *id,
 	if (found)
 		efivar_unregister(found);
 
-	if (size) {
+	if (size)
 		switch (system_state) {
 		case SYSTEM_BOOTING:
 		case SYSTEM_RUNNING:
@@ -563,7 +608,6 @@ static int efi_pstore_write(enum pstore_type_id type, u64 *id,
 		default:
 			break;
 		}
-	}
 
 	*id = part;
 	return ret;
@@ -621,62 +665,89 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
 			     struct bin_attribute *bin_attr,
 			     char *buf, loff_t pos, size_t count)
 {
-	struct efi_variable *new_var = (struct efi_variable *)buf;
+	struct efi_variable *new_var = NULL;
+	struct extended_efi_variable *ext_new_var = NULL;
 	struct efivars *efivars = bin_attr->private;
 	struct efivar_entry *search_efivar, *n;
 	unsigned long strsize1, strsize2;
 	efi_status_t status = EFI_NOT_FOUND;
 	int found = 0;
+	int error = 0;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
 
+	if (count > sizeof(struct efi_variable))
+		ext_new_var = (struct extended_efi_variable *)buf;
+	else if (count == sizeof(struct efi_variable))
+		new_var = (struct efi_variable *)buf;
+	else
+		return -EINVAL;
+
 	spin_lock(&efivars->lock);
 
+	if (new_var) {
+		ext_new_var = kmalloc(sizeof(struct extended_efi_variable) +
+				      new_var->DataSize, GFP_KERNEL);
+		memcpy(&ext_new_var->header, new_var, sizeof(struct efi_variable));
+		memcpy(ext_new_var->Data, new_var->Data, new_var->DataSize);
+	}
+
 	/*
 	 * Does this variable already exist?
 	 */
 	list_for_each_entry_safe(search_efivar, n, &efivars->list, list) {
-		strsize1 = utf16_strsize(search_efivar->var.VariableName, 1024);
-		strsize2 = utf16_strsize(new_var->VariableName, 1024);
+		strsize1 = utf16_strsize(search_efivar->var->header.VariableName, 1024);
+		strsize2 = utf16_strsize(ext_new_var->header.VariableName, 1024);
 		if (strsize1 == strsize2 &&
-			!memcmp(&(search_efivar->var.VariableName),
-				new_var->VariableName, strsize1) &&
-			!efi_guidcmp(search_efivar->var.VendorGuid,
-				new_var->VendorGuid)) {
+			!memcmp(&(search_efivar->var->header.VariableName),
+				ext_new_var->header.VariableName, strsize1) &&
+			!efi_guidcmp(search_efivar->var->header.VendorGuid,
+				ext_new_var->header.VendorGuid)) {
 			found = 1;
 			break;
 		}
 	}
 	if (found) {
 		spin_unlock(&efivars->lock);
-		return -EINVAL;
+		error = -EINVAL;
+		goto out;
 	}
 
 	/* now *really* create the variable via EFI */
-	status = efivars->ops->set_variable(new_var->VariableName,
-					    &new_var->VendorGuid,
-					    new_var->Attributes,
-					    new_var->DataSize,
-					    new_var->Data);
+	status = efivars->ops->set_variable(ext_new_var->header.VariableName,
+					    &ext_new_var->header.VendorGuid,
+					    ext_new_var->header.Attributes,
+					    ext_new_var->header.DataSize,
+					    ext_new_var->Data);
 
 	if (status != EFI_SUCCESS) {
 		printk(KERN_WARNING "efivars: set_variable() failed: status=%lx\n",
 			status);
 		spin_unlock(&efivars->lock);
-		return -EIO;
+		error = -EIO;
+		goto out;
 	}
 	spin_unlock(&efivars->lock);
 
 	/* Create the entry in sysfs.  Locking is not required here */
 	status = efivar_create_sysfs_entry(efivars,
-					   utf16_strsize(new_var->VariableName,
+					   utf16_strsize(ext_new_var->header.VariableName,
 							 1024),
-					   new_var->VariableName,
-					   &new_var->VendorGuid);
+					   ext_new_var->header.VariableName,
+					   &ext_new_var->header.VendorGuid);
 	if (status) {
 		printk(KERN_WARNING "efivars: variable created, but sysfs entry wasn't.\n");
 	}
+
+out:
+	/* Free the temporary structure if we needed it */
+	if (new_var && ext_new_var)
+		kfree(ext_new_var);
+
+	if (error)
+		return error;
+
 	return count;
 }
 
@@ -700,12 +771,12 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
 	 * Does this variable already exist?
 	 */
 	list_for_each_entry_safe(search_efivar, n, &efivars->list, list) {
-		strsize1 = utf16_strsize(search_efivar->var.VariableName, 1024);
+		strsize1 = utf16_strsize(search_efivar->var->header.VariableName, 1024);
 		strsize2 = utf16_strsize(del_var->VariableName, 1024);
 		if (strsize1 == strsize2 &&
-			!memcmp(&(search_efivar->var.VariableName),
+			!memcmp(&(search_efivar->var->header.VariableName),
 				del_var->VariableName, strsize1) &&
-			!efi_guidcmp(search_efivar->var.VendorGuid,
+			!efi_guidcmp(search_efivar->var->header.VendorGuid,
 				del_var->VendorGuid)) {
 			found = 1;
 			break;
@@ -813,9 +884,11 @@ efivar_create_sysfs_entry(struct efivars *efivars,
 	}
 
 	new_efivar->efivars = efivars;
-	memcpy(new_efivar->var.VariableName, variable_name,
+	new_efivar->var = kzalloc(sizeof(struct extended_efi_variable),
+				  GFP_KERNEL);
+	memcpy(new_efivar->var->header.VariableName, variable_name,
 		variable_name_size);
-	memcpy(&(new_efivar->var.VendorGuid), vendor_guid, sizeof(efi_guid_t));
+	memcpy(&(new_efivar->var->header.VendorGuid), vendor_guid, sizeof(efi_guid_t));
 
 	/* Convert Unicode to normal chars (assume top bits are 0),
 	   ala UTF-8 */
@@ -954,11 +1027,6 @@ int register_efivars(struct efivars *efivars,
 		goto out;
 	}
 
-	/*
-	 * Per EFI spec, the maximum storage allocated for both
-	 * the variable name and variable data is 1024 bytes.
-	 */
-
 	do {
 		variable_name_size = 1024;
 
-- 
1.7.7.1

[PATCH 4/4] EFI: Add support for QueryVariableInfo() call

[Matthew Garrett]

UEFI 2.0 and later provides a call for identifying hardware variable
capabilities. Use this to work out the maximum size of the variables we
can store, and throw errors where appropriate.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/firmware/efivars.c     |   53 +++++++++++++++++++++++++++++++++++++---
 drivers/firmware/google/gsmi.c |    9 +++++++
 include/linux/efi.h            |    1 +
 3 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 1bef80c..805dba7 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -327,8 +327,9 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
 	struct extended_efi_variable *new_var, *var = entry->var;
 	struct efi_variable *tmp_var = NULL;
 	struct efivars *efivars = entry->efivars;
-	efi_status_t status = EFI_NOT_FOUND;
+	efi_status_t status;
 	int error = 0;
+	u64 storage_space, remaining_space, max_variable_size, size;
 
 	if (count == sizeof(struct efi_variable)) {
 		tmp_var = (struct efi_variable *)buf;
@@ -360,6 +361,22 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
 	}
 
 	spin_lock(&efivars->lock);
+
+	size = new_var->header.DataSize +
+		utf16_strnlen(new_var->header.VariableName, 512) * 2;
+
+	status = efivars->ops->query_variable_info(new_var->header.Attributes,
+						   &storage_space,
+						   &remaining_space,
+						   &max_variable_size);
+	if (status == EFI_SUCCESS) {
+		if (size > remaining_space || size > max_variable_size) {
+			spin_unlock(&efivars->lock);
+			error = -ENOSPC;
+			goto out;
+		}
+	}
+
 	status = efivars->ops->set_variable(new_var->header.VariableName,
 					    &new_var->header.VendorGuid,
 					    new_var->header.Attributes,
@@ -670,9 +687,10 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
 	struct efivars *efivars = bin_attr->private;
 	struct efivar_entry *search_efivar, *n;
 	unsigned long strsize1, strsize2;
-	efi_status_t status = EFI_NOT_FOUND;
+	efi_status_t status;
 	int found = 0;
 	int error = 0;
+	u64 storage_space, remaining_space, max_variable_size, size;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
@@ -693,6 +711,21 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
 		memcpy(ext_new_var->Data, new_var->Data, new_var->DataSize);
 	}
 
+	size = ext_new_var->header.DataSize +
+		utf16_strnlen(ext_new_var->header.VariableName, 512) * 2;
+
+	status = efivars->ops->query_variable_info(ext_new_var->header.Attributes,
+						   &storage_space,
+						   &remaining_space,
+						   &max_variable_size);
+	if (status == EFI_SUCCESS) {
+		if (size > remaining_space || size > max_variable_size) {
+			spin_unlock(&efivars->lock);
+			error = -ENOSPC;
+			goto out;
+		}
+	}
+
 	/*
 	 * Does this variable already exist?
 	 */
@@ -1009,6 +1042,7 @@ int register_efivars(struct efivars *efivars,
 	efi_char16_t *variable_name;
 	unsigned long variable_name_size = 1024;
 	int error = 0;
+	u64 storage_space, remaining_space, max_variable_size;
 
 	variable_name = kzalloc(variable_name_size, GFP_KERNEL);
 	if (!variable_name) {
@@ -1056,9 +1090,19 @@ int register_efivars(struct efivars *efivars,
 
 	efivars->efi_pstore_info = efi_pstore_info;
 
-	efivars->efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
+	status = efivars->ops->query_variable_info(PSTORE_EFI_ATTRIBUTES,
+						   &storage_space,
+						   &remaining_space,
+						   &max_variable_size);
+	if (status != EFI_SUCCESS)
+		max_variable_size = 1024;
+
+	max_variable_size -= DUMP_NAME_LEN * 2;
+
+	efivars->efi_pstore_info.buf = kmalloc(max_variable_size, GFP_KERNEL);
+
 	if (efivars->efi_pstore_info.buf) {
-		efivars->efi_pstore_info.bufsize = 1024;
+		efivars->efi_pstore_info.bufsize = max_variable_size;
 		efivars->efi_pstore_info.data = efivars;
 		spin_lock_init(&efivars->efi_pstore_info.buf_lock);
 		pstore_register(&efivars->efi_pstore_info);
@@ -1103,6 +1147,7 @@ efivars_init(void)
 	ops.get_variable = efi.get_variable;
 	ops.set_variable = efi.set_variable;
 	ops.get_next_variable = efi.get_next_variable;
+	ops.query_variable_info = efi.query_variable_info;
 	error = register_efivars(&__efivars, &ops, efi_kobj);
 	if (error)
 		goto err_put;
diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
index c4e7c59..8600e3f 100644
--- a/drivers/firmware/google/gsmi.c
+++ b/drivers/firmware/google/gsmi.c
@@ -419,6 +419,14 @@ static efi_status_t gsmi_get_next_variable(unsigned long *name_size,
 	return ret;
 }
 
+static efi_status_t gsmi_query_variable_info(u32 attr,
+					     u64 *storage_space,
+					     u64 *remaining_space,
+					     u64 *max_variable_size)
+{
+	return EFI_UNSUPPORTED;
+}
+
 static efi_status_t gsmi_set_variable(efi_char16_t *name,
 				      efi_guid_t *vendor,
 				      u32 attr,
@@ -473,6 +481,7 @@ static const struct efivar_operations efivar_ops = {
 	.get_variable = gsmi_get_variable,
 	.set_variable = gsmi_set_variable,
 	.get_next_variable = gsmi_get_next_variable,
+	.query_variable_info = gsmi_query_variable_info,
 };
 
 static ssize_t eventlog_write(struct file *filp, struct kobject *kobj,
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2362a0b..f3f3860 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -446,6 +446,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 {
-- 
1.7.7.1

Re: [PATCH 2/4] efi: Only create sysfs entries while booting or running

[Mike Waychison]

On Wed, Dec 14, 2011 at 1:06 PM, Matthew Garrett <mjg@redhat.com> wrote:
> There's no point in creating sysfs entries while the machine's on its way
> down. Skip it in that case in order to avoid complexity.
>
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

Acked-by: Mike Waychison <mikew@google.com>

> ---
>  drivers/firmware/efivars.c |   17 ++++++++++++-----
>  1 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index b0a8117..eb07f13 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -552,11 +552,18 @@ static int efi_pstore_write(enum pstore_type_id type, u64 *id,
>        if (found)
>                efivar_unregister(found);
>
> -       if (size)
> -               ret = efivar_create_sysfs_entry(efivars,
> -                                         utf16_strsize(efi_name,
> -                                                       DUMP_NAME_LEN * 2),
> -                                         efi_name, &vendor);
> +       if (size) {
> +               switch (system_state) {
> +               case SYSTEM_BOOTING:
> +               case SYSTEM_RUNNING:
> +                       ret = efivar_create_sysfs_entry(efivars,
> +                                                       utf16_strsize(efi_name,
> +                                                           DUMP_NAME_LEN * 2),
> +                                                       efi_name, &vendor);
> +               default:
> +                       break;
> +               }
> +       }
>
>        *id = part;
>        return ret;
> --
> 1.7.7.1
>

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[Mike Waychison]

On Wed, Dec 14, 2011 at 1:06 PM, Matthew Garrett <mjg@redhat.com> wrote:
> The EFI variables code has a hard limit of 1024 bytes for variable length.
> This restriction only existed in version 0.99 of the EFI specification,
> and is not relevant on any existing hardware. Add support for a longer
> structure that incorporates the existing one, allowing old userspace to
> carry on working while allowing newer userspace to write larger variables
> via the same interface.
>
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/firmware/efivars.c |  240 ++++++++++++++++++++++++++++----------------
>  1 files changed, 154 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index eb07f13..1bef80c 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -92,13 +92,6 @@ MODULE_VERSION(EFIVARS_VERSION);
>
>  #define DUMP_NAME_LEN 52
>
> -/*
> - * The maximum size of VariableName + Data = 1024
> - * Therefore, it's reasonable to save that much
> - * space in each part of the structure,
> - * and we use a page for reading/writing.
> - */
> -
>  struct efi_variable {
>        efi_char16_t  VariableName[1024/sizeof(efi_char16_t)];
>        efi_guid_t    VendorGuid;
> @@ -108,10 +101,20 @@ struct efi_variable {
>        __u32         Attributes;
>  } __attribute__((packed));
>
> +/*
> + * Older versions of this driver had a fixed 1024-byte buffer. We need to
> + * maintain compatibility with old userspace, so we provide an extended
> + * structure to allow userspace to write larger variables
> + */
> +
> +struct extended_efi_variable {
> +       struct efi_variable header;
> +       __u8 Data[0];
> +} __attribute__((packed));
>
>  struct efivar_entry {
>        struct efivars *efivars;
> -       struct efi_variable var;
> +       struct extended_efi_variable *var;
>        struct list_head list;
>        struct kobject kobj;
>  };
> @@ -192,21 +195,41 @@ utf16_strncmp(const efi_char16_t *a, const efi_char16_t *b, size_t len)
>  }
>
>  static efi_status_t
> -get_var_data_locked(struct efivars *efivars, struct efi_variable *var)
> +get_var_data_locked(struct efivars *efivars, struct extended_efi_variable **var)
>  {
>        efi_status_t status;
> +       unsigned long length;
> +
> +       if (!*var)
> +               *var = kmalloc(sizeof(struct extended_efi_variable), GFP_KERNEL);

Aren't we holding a spinlock here?

> +
> +       (*var)->header.DataSize = 0;
> +       status = efivars->ops->get_variable((*var)->header.VariableName,
> +                                           &(*var)->header.VendorGuid,
> +                                           &(*var)->header.Attributes,
> +                                           &(*var)->header.DataSize,
> +                                           (*var)->Data);

This doesn't look right.  ->Data here is after the Data[1024] buffer
embedded in (*var)->header, and a read into this buffer will corrupt
the heap.

> +
> +       if (status == EFI_BUFFER_TOO_SMALL) {
> +               *var = krealloc(*var, sizeof(struct extended_efi_variable) +
> +                               (*var)->header.DataSize, GFP_KERNEL);
> +               status = efivars->ops->get_variable((*var)->header.VariableName,
> +                                                   &(*var)->header.VendorGuid,
> +                                                   &(*var)->header.Attributes,
> +                                                   &(*var)->header.DataSize,
> +                                                   (*var)->Data);
> +       }
> +
> +       length = ((*var)->header.DataSize < 1024) ? (*var)->header.DataSize :
> +               1024;
> +
> +       memcpy(&(*var)->header.Data, &(*var)->Data, length);

This memcpy clobbers the header.Data with the corrupted data when we
didn't use the second path.

>
> -       var->DataSize = 1024;
> -       status = efivars->ops->get_variable(var->VariableName,
> -                                           &var->VendorGuid,
> -                                           &var->Attributes,
> -                                           &var->DataSize,
> -                                           var->Data);
>        return status;
>  }
>
>  static efi_status_t
> -get_var_data(struct efivars *efivars, struct efi_variable *var)
> +get_var_data(struct efivars *efivars, struct extended_efi_variable **var)
>  {
>        efi_status_t status;
>
> @@ -224,13 +247,13 @@ get_var_data(struct efivars *efivars, struct efi_variable *var)
>  static ssize_t
>  efivar_guid_read(struct efivar_entry *entry, char *buf)
>  {
> -       struct efi_variable *var = &entry->var;
> +       struct extended_efi_variable *var = entry->var;
>        char *str = buf;
>
>        if (!entry || !buf)
>                return 0;
>
> -       efi_guid_unparse(&var->VendorGuid, str);
> +       efi_guid_unparse(&var->header.VendorGuid, str);
>        str += strlen(str);
>        str += sprintf(str, "\n");
>
> @@ -240,22 +263,22 @@ efivar_guid_read(struct efivar_entry *entry, char *buf)
>  static ssize_t
>  efivar_attr_read(struct efivar_entry *entry, char *buf)
>  {
> -       struct efi_variable *var = &entry->var;
> +       struct extended_efi_variable *var = entry->var;
>        char *str = buf;
>        efi_status_t status;
>
>        if (!entry || !buf)
>                return -EINVAL;
>
> -       status = get_var_data(entry->efivars, var);
> +       status = get_var_data(entry->efivars, &var);
>        if (status != EFI_SUCCESS)
>                return -EIO;
>
> -       if (var->Attributes & 0x1)
> +       if (var->header.Attributes & 0x1)
>                str += sprintf(str, "EFI_VARIABLE_NON_VOLATILE\n");
> -       if (var->Attributes & 0x2)
> +       if (var->header.Attributes & 0x2)
>                str += sprintf(str, "EFI_VARIABLE_BOOTSERVICE_ACCESS\n");
> -       if (var->Attributes & 0x4)
> +       if (var->header.Attributes & 0x4)
>                str += sprintf(str, "EFI_VARIABLE_RUNTIME_ACCESS\n");
>        return str - buf;
>  }
> @@ -263,36 +286,36 @@ efivar_attr_read(struct efivar_entry *entry, char *buf)
>  static ssize_t
>  efivar_size_read(struct efivar_entry *entry, char *buf)
>  {
> -       struct efi_variable *var = &entry->var;
> +       struct extended_efi_variable *var = entry->var;
>        char *str = buf;
>        efi_status_t status;
>
>        if (!entry || !buf)
>                return -EINVAL;
>
> -       status = get_var_data(entry->efivars, var);
> +       status = get_var_data(entry->efivars, &var);
>        if (status != EFI_SUCCESS)
>                return -EIO;
>
> -       str += sprintf(str, "0x%lx\n", var->DataSize);
> +       str += sprintf(str, "0x%lx\n", var->header.DataSize);
>        return str - buf;
>  }
>
>  static ssize_t
>  efivar_data_read(struct efivar_entry *entry, char *buf)
>  {
> -       struct efi_variable *var = &entry->var;
> +       struct extended_efi_variable *var = entry->var;
>        efi_status_t status;
>
>        if (!entry || !buf)
>                return -EINVAL;
>
> -       status = get_var_data(entry->efivars, var);
> +       status = get_var_data(entry->efivars, &var);
>        if (status != EFI_SUCCESS)
>                return -EIO;
>
> -       memcpy(buf, var->Data, var->DataSize);
> -       return var->DataSize;
> +       memcpy(buf, var->Data, var->header.DataSize);
> +       return var->header.DataSize;
>  }
>  /*
>  * We allow each variable to be edited via rewriting the
> @@ -301,34 +324,46 @@ efivar_data_read(struct efivar_entry *entry, char *buf)
>  static ssize_t
>  efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
>  {
> -       struct efi_variable *new_var, *var = &entry->var;
> +       struct extended_efi_variable *new_var, *var = entry->var;
> +       struct efi_variable *tmp_var = NULL;
>        struct efivars *efivars = entry->efivars;
>        efi_status_t status = EFI_NOT_FOUND;
> +       int error = 0;
>
> -       if (count != sizeof(struct efi_variable))
> +       if (count == sizeof(struct efi_variable)) {
> +               tmp_var = (struct efi_variable *)buf;
> +               new_var = kmalloc(sizeof(struct efi_variable) +
> +                                 tmp_var->DataSize, GFP_KERNEL);
> +               memcpy(&new_var->header, tmp_var, sizeof(struct efi_variable));
> +               memcpy(&new_var->Data, tmp_var->Data, tmp_var->DataSize);
> +       } else if (count > sizeof(struct efi_variable)) {
> +               new_var = (struct extended_efi_variable *)buf;
> +       } else {
>                return -EINVAL;
> +       }

Ugh.  This is difficult to follow, and complicates the memory freeing path :(

We need to be careful here.  The store_raw ABI is broken, in the sense
that the ABI from compat mode differs from that in 32bit mode (there
is a long in the efi_variable structure which changes the offsets).  I
don't know how to fix it properly and still maintain proper ABI
compatibility.

What are your thoughts on _not_ wrapping efi_variable with
extended_efi_variable, and instead just using a
"internal_efi_variable" structure that we copy stuff into/outof.  I
think that would make the memory management for dealing with the
different sizes a lot easier to follow.

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[Matthew Garrett]

On Wed, Dec 14, 2011 at 02:14:27PM -0800, Mike Waychison wrote:

> >  static efi_status_t
> > -get_var_data_locked(struct efivars *efivars, struct efi_variable *var)
> > +get_var_data_locked(struct efivars *efivars, struct extended_efi_variable **var)
> >  {
> >        efi_status_t status;
> > +       unsigned long length;
> > +
> > +       if (!*var)
> > +               *var = kmalloc(sizeof(struct extended_efi_variable), GFP_KERNEL);
> 
> Aren't we holding a spinlock here?

Good point.

> > +
> > +       (*var)->header.DataSize = 0;
> > +       status = efivars->ops->get_variable((*var)->header.VariableName,
> > +                                           &(*var)->header.VendorGuid,
> > +                                           &(*var)->header.Attributes,
> > +                                           &(*var)->header.DataSize,
> > +                                           (*var)->Data);
> 
> This doesn't look right.  ->Data here is after the Data[1024] buffer
> embedded in (*var)->header, and a read into this buffer will corrupt
> the heap.

DataSize is 0, so we'll never actually read anything back here.

> > +
> > +       if (status == EFI_BUFFER_TOO_SMALL) {
> > +               *var = krealloc(*var, sizeof(struct extended_efi_variable) +
> > +                               (*var)->header.DataSize, GFP_KERNEL);
> > +               status = efivars->ops->get_variable((*var)->header.VariableName,
> > +                                                   &(*var)->header.VendorGuid,
> > +                                                   &(*var)->header.Attributes,
> > +                                                   &(*var)->header.DataSize,
> > +                                                   (*var)->Data);
> > +       }
> > +
> > +       length = ((*var)->header.DataSize < 1024) ? (*var)->header.DataSize :
> > +               1024;
> > +
> > +       memcpy(&(*var)->header.Data, &(*var)->Data, length);
> 
> This memcpy clobbers the header.Data with the corrupted data when we
> didn't use the second path.

We'll always follow the second path providing there's actually data to 
read back. If there isn't then length will be 0.

> > +       if (count == sizeof(struct efi_variable)) {
> > +               tmp_var = (struct efi_variable *)buf;
> > +               new_var = kmalloc(sizeof(struct efi_variable) +
> > +                                 tmp_var->DataSize, GFP_KERNEL);
> > +               memcpy(&new_var->header, tmp_var, sizeof(struct efi_variable));
> > +               memcpy(&new_var->Data, tmp_var->Data, tmp_var->DataSize);
> > +       } else if (count > sizeof(struct efi_variable)) {
> > +               new_var = (struct extended_efi_variable *)buf;
> > +       } else {
> >                return -EINVAL;
> > +       }
> 
> Ugh.  This is difficult to follow, and complicates the memory freeing path :(

Entirely agreed.

> We need to be careful here.  The store_raw ABI is broken, in the sense
> that the ABI from compat mode differs from that in 32bit mode (there
> is a long in the efi_variable structure which changes the offsets).  I
> don't know how to fix it properly and still maintain proper ABI
> compatibility.

True.

> What are your thoughts on _not_ wrapping efi_variable with
> extended_efi_variable, and instead just using a
> "internal_efi_variable" structure that we copy stuff into/outof.  I
> think that would make the memory management for dealing with the
> different sizes a lot easier to follow.

Hm. I think that'd only work if we expose a new interface. Writes would 
be easy enough to handle, but reads still need to work for old apps.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[H. Peter Anvin]

On 12/14/2011 02:39 PM, Matthew Garrett wrote:
> 
>> We need to be careful here.  The store_raw ABI is broken, in the sense
>> that the ABI from compat mode differs from that in 32bit mode (there
>> is a long in the efi_variable structure which changes the offsets).  I
>> don't know how to fix it properly and still maintain proper ABI
>> compatibility.
> 
> True.
> 
>> What are your thoughts on _not_ wrapping efi_variable with
>> extended_efi_variable, and instead just using a
>> "internal_efi_variable" structure that we copy stuff into/outof.  I
>> think that would make the memory management for dealing with the
>> different sizes a lot easier to follow.
> 
> Hm. I think that'd only work if we expose a new interface. Writes would 
> be easy enough to handle, but reads still need to work for old apps.
> 

Would making the old ABI readonly and add a new write interface resolve
the problem with compat mode?

	-hpa

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[Matthew Garrett]

On Wed, Dec 14, 2011 at 02:44:34PM -0800, H. Peter Anvin wrote:
> On 12/14/2011 02:39 PM, Matthew Garrett wrote:
> >> What are your thoughts on _not_ wrapping efi_variable with
> >> extended_efi_variable, and instead just using a
> >> "internal_efi_variable" structure that we copy stuff into/outof.  I
> >> think that would make the memory management for dealing with the
> >> different sizes a lot easier to follow.
> > 
> > Hm. I think that'd only work if we expose a new interface. Writes would 
> > be easy enough to handle, but reads still need to work for old apps.
> > 
> 
> Would making the old ABI readonly and add a new write interface resolve
> the problem with compat mode?

Yes, but we'd break existing userspace.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[Mike Waychison]

On Wed, Dec 14, 2011 at 2:39 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Wed, Dec 14, 2011 at 02:14:27PM -0800, Mike Waychison wrote:
>
>> >  static efi_status_t
>> > -get_var_data_locked(struct efivars *efivars, struct efi_variable *var)
>> > +get_var_data_locked(struct efivars *efivars, struct extended_efi_variable **var)
>> >  {
>> >        efi_status_t status;
>> > +       unsigned long length;
>> > +
>> > +       if (!*var)
>> > +               *var = kmalloc(sizeof(struct extended_efi_variable), GFP_KERNEL);
>>
>> Aren't we holding a spinlock here?
>
> Good point.
>
>> > +
>> > +       (*var)->header.DataSize = 0;
>> > +       status = efivars->ops->get_variable((*var)->header.VariableName,
>> > +                                           &(*var)->header.VendorGuid,
>> > +                                           &(*var)->header.Attributes,
>> > +                                           &(*var)->header.DataSize,
>> > +                                           (*var)->Data);
>>
>> This doesn't look right.  ->Data here is after the Data[1024] buffer
>> embedded in (*var)->header, and a read into this buffer will corrupt
>> the heap.
>
> DataSize is 0, so we'll never actually read anything back here.

Ah.  I missed that.  Hmm, I wonder if this actually works :)

>
>> > +
>> > +       if (status == EFI_BUFFER_TOO_SMALL) {
>> > +               *var = krealloc(*var, sizeof(struct extended_efi_variable) +
>> > +                               (*var)->header.DataSize, GFP_KERNEL);
>> > +               status = efivars->ops->get_variable((*var)->header.VariableName,
>> > +                                                   &(*var)->header.VendorGuid,
>> > +                                                   &(*var)->header.Attributes,
>> > +                                                   &(*var)->header.DataSize,
>> > +                                                   (*var)->Data);
>> > +       }
>> > +
>> > +       length = ((*var)->header.DataSize < 1024) ? (*var)->header.DataSize :
>> > +               1024;
>> > +
>> > +       memcpy(&(*var)->header.Data, &(*var)->Data, length);
>>
>> This memcpy clobbers the header.Data with the corrupted data when we
>> didn't use the second path.
>
> We'll always follow the second path providing there's actually data to
> read back. If there isn't then length will be 0.
>
>> > +       if (count == sizeof(struct efi_variable)) {
>> > +               tmp_var = (struct efi_variable *)buf;
>> > +               new_var = kmalloc(sizeof(struct efi_variable) +
>> > +                                 tmp_var->DataSize, GFP_KERNEL);
>> > +               memcpy(&new_var->header, tmp_var, sizeof(struct efi_variable));
>> > +               memcpy(&new_var->Data, tmp_var->Data, tmp_var->DataSize);
>> > +       } else if (count > sizeof(struct efi_variable)) {
>> > +               new_var = (struct extended_efi_variable *)buf;
>> > +       } else {
>> >                return -EINVAL;
>> > +       }
>>
>> Ugh.  This is difficult to follow, and complicates the memory freeing path :(
>
> Entirely agreed.
>
>> We need to be careful here.  The store_raw ABI is broken, in the sense
>> that the ABI from compat mode differs from that in 32bit mode (there
>> is a long in the efi_variable structure which changes the offsets).  I
>> don't know how to fix it properly and still maintain proper ABI
>> compatibility.
>
> True.
>
>> What are your thoughts on _not_ wrapping efi_variable with
>> extended_efi_variable, and instead just using a
>> "internal_efi_variable" structure that we copy stuff into/outof.  I
>> think that would make the memory management for dealing with the
>> different sizes a lot easier to follow.
>
> Hm. I think that'd only work if we expose a new interface. Writes would
> be easy enough to handle, but reads still need to work for old apps.

Well,l we could *not* support returning all the data field for
datasize > 1024, and simply truncate the field.  We are limited by
PAGE_SIZE by sysfs here anyway (so we don't really want to have a
variable size memcpy in efivar_show_raw).

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[H. Peter Anvin]

On 12/14/2011 02:57 PM, Matthew Garrett wrote:
> On Wed, Dec 14, 2011 at 02:44:34PM -0800, H. Peter Anvin wrote:
>> On 12/14/2011 02:39 PM, Matthew Garrett wrote:
>>>> What are your thoughts on _not_ wrapping efi_variable with
>>>> extended_efi_variable, and instead just using a
>>>> "internal_efi_variable" structure that we copy stuff into/outof.  I
>>>> think that would make the memory management for dealing with the
>>>> different sizes a lot easier to follow.
>>>
>>> Hm. I think that'd only work if we expose a new interface. Writes would 
>>> be easy enough to handle, but reads still need to work for old apps.
>>>
>>
>> Would making the old ABI readonly and add a new write interface resolve
>> the problem with compat mode?
> 
> Yes, but we'd break existing userspace.
> 

Obviously.  However, you indicated above that that might be acceptable
-- how many things use this ABI to set variables?

	-hpa

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[H. Peter Anvin]

On 12/14/2011 02:57 PM, Mike Waychison wrote:
> 
> Well,l we could *not* support returning all the data field for
> datasize > 1024, and simply truncate the field.  We are limited by
> PAGE_SIZE by sysfs here anyway (so we don't really want to have a
> variable size memcpy in efivar_show_raw).
> 

That may be the biggest reason to avoid sysfs.  As far as I know sysfs
doesn't allow seq_file to be used.

	-hpa

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[Mike Waychison]

On Wed, Dec 14, 2011 at 3:00 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 12/14/2011 02:57 PM, Mike Waychison wrote:
>>
>> Well,l we could *not* support returning all the data field for
>> datasize > 1024, and simply truncate the field.  We are limited by
>> PAGE_SIZE by sysfs here anyway (so we don't really want to have a
>> variable size memcpy in efivar_show_raw).
>>
>
> That may be the biggest reason to avoid sysfs.  As far as I know sysfs
> doesn't allow seq_file to be used.
>

Completely agreed.  I don't think a seq_file is warranted in this case
in particular, but the dummification of the interfaces in sysfs sure
makes it hard to do anything that isn't a "single value string".

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[H. Peter Anvin]

On 12/14/2011 03:06 PM, Mike Waychison wrote:
> On Wed, Dec 14, 2011 at 3:00 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 12/14/2011 02:57 PM, Mike Waychison wrote:
>>>
>>> Well,l we could *not* support returning all the data field for
>>> datasize > 1024, and simply truncate the field.  We are limited by
>>> PAGE_SIZE by sysfs here anyway (so we don't really want to have a
>>> variable size memcpy in efivar_show_raw).
>>>
>>
>> That may be the biggest reason to avoid sysfs.  As far as I know sysfs
>> doesn't allow seq_file to be used.
>>
> 
> Completely agreed.  I don't think a seq_file is warranted in this case
> in particular, but the dummification of the interfaces in sysfs sure
> makes it hard to do anything that isn't a "single value string".
> 

Well, seq_file is a good way to deal with arbitrary length data.

	-hpa

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[Mike Waychison]

On Wed, Dec 14, 2011 at 3:22 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 12/14/2011 03:06 PM, Mike Waychison wrote:
>> On Wed, Dec 14, 2011 at 3:00 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>> On 12/14/2011 02:57 PM, Mike Waychison wrote:
>>>>
>>>> Well,l we could *not* support returning all the data field for
>>>> datasize > 1024, and simply truncate the field.  We are limited by
>>>> PAGE_SIZE by sysfs here anyway (so we don't really want to have a
>>>> variable size memcpy in efivar_show_raw).
>>>>
>>>
>>> That may be the biggest reason to avoid sysfs.  As far as I know sysfs
>>> doesn't allow seq_file to be used.
>>>
>>
>> Completely agreed.  I don't think a seq_file is warranted in this case
>> in particular, but the dummification of the interfaces in sysfs sure
>> makes it hard to do anything that isn't a "single value string".
>>
>
> Well, seq_file is a good way to deal with arbitrary length data.
>

seq_file maps well to arbitrary record counts (keeping records
self-consistent), but not so well for arbitrarily large records.

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[Matthew Garrett]

On Wed, Dec 14, 2011 at 02:57:51PM -0800, Mike Waychison wrote:
> On Wed, Dec 14, 2011 at 2:39 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > Hm. I think that'd only work if we expose a new interface. Writes would
> > be easy enough to handle, but reads still need to work for old apps.
> 
> Well,l we could *not* support returning all the data field for
> datasize > 1024, and simply truncate the field.  We are limited by
> PAGE_SIZE by sysfs here anyway (so we don't really want to have a
> variable size memcpy in efivar_show_raw).

We'll pretty much definitely want to be able to read values > 1024 once 
people start shoving keys in there. The PAGE_SIZE limit is one that I'd 
forgotten about, though.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[Matthew Garrett]

On Wed, Dec 14, 2011 at 02:58:20PM -0800, H. Peter Anvin wrote:
> On 12/14/2011 02:57 PM, Matthew Garrett wrote:
> > Yes, but we'd break existing userspace.
> > 
> 
> Obviously.  However, you indicated above that that might be acceptable
> -- how many things use this ABI to set variables?

Ah, sorry - my suggestion had been to add an additional interface, not 
just replace the existing one. efibootmgr is the only userland I know of 
that interacts directly, but it's entirely possible that there are 
others - I'd guess Google have something.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[H. Peter Anvin]

On 12/15/2011 07:43 AM, Matthew Garrett wrote:
>
> We'll pretty much definitely want to be able to read values>  1024 once
> people start shoving keys in there. The PAGE_SIZE limit is one that I'd
> forgotten about, though.
>

The jump from 1024 to 4096 seems to not be very long... hence I think we 
may need a different approach.

Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes

[Matthew Garrett]

On Thu, Dec 15, 2011 at 07:46:44AM -0800, H. Peter Anvin wrote:
> On 12/15/2011 07:43 AM, Matthew Garrett wrote:
> >
> >We'll pretty much definitely want to be able to read values>  1024 once
> >people start shoving keys in there. The PAGE_SIZE limit is one that I'd
> >forgotten about, though.
> >
> 
> The jump from 1024 to 4096 seems to not be very long... hence I
> think we may need a different approach.

Yeah. I'll start thinking about the most sensible filesystem interface 
for this.

-- 
Matthew Garrett | mjg59@srcf.ucam.org