From 74d5b500b0184d6ddf4e59328b50a9521c1cd1be Mon Sep 17 00:00:00 2001 From: Matthew Garrett Date: Sat, 2 Mar 2013 19:40:17 -0500 Subject: [PATCH 1/3] efi: be more paranoid about available space when creating variables UEFI variables are typically stored in flash. For various reasons, avaiable space is typically not reclaimed immediately upon the deletion of a variable - instead, the system will garbage collect during initialisation after a reboot. Some systems appear to handle this garbage collection extremely poorly, failing if more than 50% of the system flash is in use. This can result in the machine refusing to boot. The safest thing to do for the moment is to forbid writes if they'd end up using more than half of the storage space. We can make this more finegrained later if we come up with a method for identifying the broken machines. Signed-off-by: Matthew Garrett Cc: Signed-off-by: Matt Fleming --- drivers/firmware/efivars.c | 106 +++++++++++++++++++++++++++++++++------------ 1 file changed, 79 insertions(+), 27 deletions(-) diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index 7320bf8..0d50497 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -426,6 +426,44 @@ get_var_data(struct efivars *efivars, struct efi_variable *var) return status; } +static efi_status_t +check_var_size_locked(struct efivars *efivars, u32 attributes, + unsigned long size) +{ + u64 storage_size, remaining_size, max_size; + efi_status_t status; + const struct efivar_operations *fops = efivars->ops; + + if (!efivars->ops->query_variable_info) + return EFI_UNSUPPORTED; + + status = fops->query_variable_info(attributes, &storage_size, + &remaining_size, &max_size); + + if (status != EFI_SUCCESS) + return status; + + if (!storage_size || size > remaining_size || size > max_size || + (remaining_size - size) < (storage_size / 2)) + return EFI_OUT_OF_RESOURCES; + + return status; +} + + +static efi_status_t +check_var_size(struct efivars *efivars, u32 attributes, unsigned long size) +{ + efi_status_t status; + unsigned long flags; + + spin_lock_irqsave(&efivars->lock, flags); + status = check_var_size_locked(efivars, attributes, size); + spin_unlock_irqrestore(&efivars->lock, flags); + + return status; +} + static ssize_t efivar_guid_read(struct efivar_entry *entry, char *buf) { @@ -547,11 +585,16 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count) } spin_lock_irq(&efivars->lock); - status = efivars->ops->set_variable(new_var->VariableName, - &new_var->VendorGuid, - new_var->Attributes, - new_var->DataSize, - new_var->Data); + + status = check_var_size_locked(efivars, new_var->Attributes, + new_var->DataSize + utf16_strsize(new_var->VariableName, 1024)); + + if (status == EFI_SUCCESS || status == EFI_UNSUPPORTED) + status = efivars->ops->set_variable(new_var->VariableName, + &new_var->VendorGuid, + new_var->Attributes, + new_var->DataSize, + new_var->Data); spin_unlock_irq(&efivars->lock); @@ -702,8 +745,7 @@ static ssize_t efivarfs_file_write(struct file *file, u32 attributes; struct inode *inode = file->f_mapping->host; unsigned long datasize = count - sizeof(attributes); - unsigned long newdatasize; - u64 storage_size, remaining_size, max_size; + unsigned long newdatasize, varsize; ssize_t bytes = 0; if (count < sizeof(attributes)) @@ -722,28 +764,18 @@ static ssize_t efivarfs_file_write(struct file *file, * amounts of memory. Pick a default size of 64K if * QueryVariableInfo() isn't supported by the firmware. */ - spin_lock_irq(&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); - } - - spin_unlock_irq(&efivars->lock); + varsize = datasize + utf16_strsize(var->var.VariableName, 1024); + status = check_var_size(efivars, attributes, varsize); if (status != EFI_SUCCESS) { if (status != EFI_UNSUPPORTED) return efi_status_to_err(status); - remaining_size = 65536; + if (datasize > 65536) + return -ENOSPC; } - if (datasize > remaining_size) - return -ENOSPC; - data = kmalloc(datasize, GFP_KERNEL); if (!data) return -ENOMEM; @@ -765,6 +797,19 @@ static ssize_t efivarfs_file_write(struct file *file, */ spin_lock_irq(&efivars->lock); + /* + * Ensure that the available space hasn't shrunk below the safe level + */ + + status = check_var_size_locked(efivars, attributes, varsize); + + if (status != EFI_SUCCESS && status != EFI_UNSUPPORTED) { + spin_unlock_irq(&efivars->lock); + kfree(data); + + return efi_status_to_err(status); + } + status = efivars->ops->set_variable(var->var.VariableName, &var->var.VendorGuid, attributes, datasize, @@ -1345,7 +1390,6 @@ static int efi_pstore_write(enum pstore_type_id type, efi_guid_t vendor = LINUX_EFI_CRASH_GUID; struct efivars *efivars = psi->data; int i, ret = 0; - u64 storage_space, remaining_space, max_variable_size; efi_status_t status = EFI_NOT_FOUND; unsigned long flags; @@ -1365,11 +1409,11 @@ static int efi_pstore_write(enum pstore_type_id type, * size: a size of logging data * DUMP_NAME_LEN * 2: a maximum size of variable name */ - status = efivars->ops->query_variable_info(PSTORE_EFI_ATTRIBUTES, - &storage_space, - &remaining_space, - &max_variable_size); - if (status || remaining_space < size + DUMP_NAME_LEN * 2) { + + status = check_var_size_locked(efivars, PSTORE_EFI_ATTRIBUTES, + size + DUMP_NAME_LEN * 2); + + if (status) { spin_unlock_irqrestore(&efivars->lock, flags); *id = part; return -ENOSPC; @@ -1544,6 +1588,14 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj, return -EINVAL; } + status = check_var_size_locked(efivars, new_var->Attributes, + new_var->DataSize + utf16_strsize(new_var->VariableName, 1024)); + + if (status && status != EFI_UNSUPPORTED) { + spin_unlock_irq(&efivars->lock); + return efi_status_to_err(status); + } + /* now *really* create the variable via EFI */ status = efivars->ops->set_variable(new_var->VariableName, &new_var->VendorGuid, -- 1.8.1.2 From 8200cc0633605f417a1f0c229772f9033d57ea0a Mon Sep 17 00:00:00 2001 From: Matt Fleming Date: Tue, 5 Mar 2013 07:40:16 +0000 Subject: [PATCH 2/3] efivars: efivarfs_valid_name() should handle pstore syntax Stricter validation was introduced with commit da27a24383b2b ("efivarfs: guid part of filenames are case-insensitive") and commit 47f531e8ba3b ("efivarfs: Validate filenames much more aggressively"), which is necessary for the guid portion of efivarfs filenames, but we don't need to be so strict with the first part, the variable name. The UEFI specification doesn't impose any constraints on variable names other than they be a NULL-terminated string. The above commits caused a regression that resulted in users seeing the following message, $ sudo mount -v /sys/firmware/efi/efivars mount: Cannot allocate memory whenever pstore EFI variables were present in the variable store, since their variable names failed to pass the following check, /* GUID should be right after the first '-' */ if (s - 1 != strchr(str, '-')) as a typical pstore filename is of the form, dump-type0-10-1-. The fix is trivial since the guid portion of the filename is GUID_LEN bytes, we can use (len - GUID_LEN) to ensure the '-' character is where we expect it to be. (The bogus ENOMEM error value will be fixed in a separate patch.) Reported-by: Joseph Yasi Reported-by: Lingzhu Xiang Cc: Josh Boyer Cc: Jeremy Kerr Cc: Matthew Garrett Cc: Signed-off-by: Matt Fleming --- drivers/firmware/efivars.c | 4 +- tools/testing/selftests/efivarfs/efivarfs.sh | 59 ++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index 0d50497..1b9a6e1 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -974,8 +974,8 @@ static bool efivarfs_valid_name(const char *str, int len) if (len < GUID_LEN + 2) return false; - /* GUID should be right after the first '-' */ - if (s - 1 != strchr(str, '-')) + /* GUID must be preceded by a '-' */ + if (*(s - 1) != '-') return false; /* diff --git a/tools/testing/selftests/efivarfs/efivarfs.sh b/tools/testing/selftests/efivarfs/efivarfs.sh index 880cdd5..77edcdc 100644 --- a/tools/testing/selftests/efivarfs/efivarfs.sh +++ b/tools/testing/selftests/efivarfs/efivarfs.sh @@ -125,6 +125,63 @@ test_open_unlink() ./open-unlink $file } +# test that we can create a range of filenames +test_valid_filenames() +{ + local attrs='\x07\x00\x00\x00' + local ret=0 + + local file_list="abc dump-type0-11-1-1362436005 1234 -" + for f in $file_list; do + local file=$efivarfs_mount/$f-$test_guid + + printf "$attrs\x00" > $file + + if [ ! -e $file ]; then + echo "$file could not be created" >&2 + ret=1 + else + rm $file + fi + done + + exit $ret +} + +test_invalid_filenames() +{ + local attrs='\x07\x00\x00\x00' + local ret=0 + + local file_list=" + -1234-1234-1234-123456789abc + foo + foo-bar + -foo- + foo-barbazba-foob-foob-foob-foobarbazfoo + foo------------------------------------- + -12345678-1234-1234-1234-123456789abc + a-12345678=1234-1234-1234-123456789abc + a-12345678-1234=1234-1234-123456789abc + a-12345678-1234-1234=1234-123456789abc + a-12345678-1234-1234-1234=123456789abc + 1112345678-1234-1234-1234-123456789abc" + + for f in $file_list; do + local file=$efivarfs_mount/$f + + printf "$attrs\x00" 2>/dev/null > $file + + if [ -e $file ]; then + echo "Creating $file should have failed" >&2 + rm $file + ret=1 + fi + done + + exit $ret +} + check_prereqs rc=0 @@ -135,5 +192,7 @@ run_test test_create_read run_test test_delete run_test test_zero_size_delete run_test test_open_unlink +run_test test_valid_filenames +run_test test_invalid_filenames exit $rc -- 1.8.1.2 From 396c0285825255c6e2549c9a6eec6c23a35c9f7f Mon Sep 17 00:00:00 2001 From: Matt Fleming Date: Tue, 5 Mar 2013 12:46:30 +0000 Subject: [PATCH 3/3] efivarfs: return accurate error code in efivarfs_fill_super() Joseph was hitting a failure case when mounting efivarfs which resulted in an incorrect error message, $ sudo mount -v /sys/firmware/efi/efivars mount: Cannot allocate memory triggered when efivarfs_valid_name() returned -EINVAL. Make sure we pass accurate return values up the stack if efivarfs_fill_super() fails to build inodes for EFI variables. Reported-by: Joseph Yasi Reported-by: Lingzhu Xiang Cc: Josh Boyer Cc: Jeremy Kerr Cc: Matthew Garrett Cc: Signed-off-by: Matt Fleming --- drivers/firmware/efivars.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index 1b9a6e1..bea32d1 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -1163,15 +1163,22 @@ static struct dentry_operations efivarfs_d_ops = { static struct dentry *efivarfs_alloc_dentry(struct dentry *parent, char *name) { + struct dentry *d; struct qstr q; + int err; q.name = name; q.len = strlen(name); - if (efivarfs_d_hash(NULL, NULL, &q)) - return NULL; + err = efivarfs_d_hash(NULL, NULL, &q); + if (err) + return ERR_PTR(err); + + d = d_alloc(parent, &q); + if (d) + return d; - return d_alloc(parent, &q); + return ERR_PTR(-ENOMEM); } static int efivarfs_fill_super(struct super_block *sb, void *data, int silent) @@ -1181,6 +1188,7 @@ static int efivarfs_fill_super(struct super_block *sb, void *data, int silent) struct efivar_entry *entry, *n; struct efivars *efivars = &__efivars; char *name; + int err = -ENOMEM; efivarfs_sb = sb; @@ -1231,8 +1239,10 @@ static int efivarfs_fill_super(struct super_block *sb, void *data, int silent) goto fail_name; dentry = efivarfs_alloc_dentry(root, name); - if (!dentry) + if (IS_ERR(dentry)) { + err = PTR_ERR(dentry); goto fail_inode; + } /* copied by the above to local storage in the dentry. */ kfree(name); @@ -1259,7 +1269,7 @@ fail_inode: fail_name: kfree(name); fail: - return -ENOMEM; + return err; } static struct dentry *efivarfs_mount(struct file_system_type *fs_type, -- 1.8.1.2