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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Matthew Garrett]

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

Signed-off-by: Matthew Garrett <m...@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

[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 <m...@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

[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 <m...@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

[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 <m...@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

[Mike Waychison]

On Wed, Dec 14, 2011 at 1:06 PM, Matthew Garrett <m...@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 <m...@redhat.com>

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

[Mike Waychison]

On Wed, Dec 14, 2011 at 1:06 PM, Matthew Garrett <m...@redhat.com> wrote:

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.

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.

[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 | mj...@srcf.ucam.org

[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

[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 | mj...@srcf.ucam.org

[Mike Waychison]

On Wed, Dec 14, 2011 at 2:39 PM, Matthew Garrett <mj...@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 :)

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).

[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

[Mike Waychison]

On Wed, Dec 14, 2011 at 3:22 PM, H. Peter Anvin <h...@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 <h...@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.

[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 | mj...@srcf.ucam.org