Hi Vladis,
On Tue, Mar 03, 2020 at 09:55:28AM +0100, Vladis Dronov wrote:
> There is a race and a buffer overflow corrupting a kernel memory while
> reading an efi variable with a size more than 1024 bytes via the older
> sysfs method. This happens because accessing struct efi_variable in
> efivar_{attr,size,data}_read() and friends is not protected from
> a concurrent access leading to a kernel memory corruption and, at best,
> to a crash. The race scenario is the following:
>
> CPU0: CPU1:
> efivar_attr_read()
> var->DataSize = 1024;
> efivar_entry_get(... &var->DataSize)
> down_interruptible(&efivars_lock)
> efivar_attr_read() // same efi var
> var->DataSize = 1024;
> efivar_entry_get(... &var->DataSize)
> down_interruptible(&efivars_lock)
> virt_efi_get_variable()
> // returns EFI_BUFFER_TOO_SMALL but
> // var->DataSize is set to a real
> // var size more than 1024 bytes
> up(&efivars_lock)
> virt_efi_get_variable()
> // called with var->DataSize set
> // to a real var size, returns
> // successfully and overwrites
> // a 1024-bytes kernel buffer
> up(&efivars_lock)
>
> This can be reproduced by concurrent reading of an efi variable which size
> is more than 1024 bytes:
>
> ts# for cpu in $(seq 0 $(nproc --ignore=1)); do ( taskset -c $cpu \
> cat /sys/firmware/efi/vars/KEKDefault*/size & ) ; done
>
> Fix this by protecting struct efi_variable access by efivars_lock by using
> efivar_entry_iter_begin()/efivar_entry_iter_end(). Brush up and unify
> efivar_entry_[gs]et() and __efivar_entry_[gs]et() for this. This looks
> simpler than introducing a separate lock for every struct efi_variable.
>
> Also fix the same race in efivar_store_raw() and efivar_show_raw(). The
> call in efi_pstore_read_func() is protected like this already.
>
> Reported-by: Bob Sanders <bob.sanders@xxxxxxx> and the LTP testsuite
> Signed-off-by: Vladis Dronov <vdronov@xxxxxxxxxx>
I have reviewed and tested this patch. It's good to me if we still want
to use efi_variable structure as the return buffer of UEFI get/set_variable
protocols.
Please feel free to add:
Reviewed-by: Joey Lee <jlee@xxxxxxxx>
Regards
Joey Lee
> ---
> drivers/firmware/efi/efi-pstore.c | 2 +-
> drivers/firmware/efi/efivars.c | 72 ++++++++++++++++++++++++-------
> drivers/firmware/efi/vars.c | 47 ++++++++++++--------
> include/linux/efi.h | 2 +
> 4 files changed, 90 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
> index 9ea13e8d12ec..e4767a7ce973 100644
> --- a/drivers/firmware/efi/efi-pstore.c
> +++ b/drivers/firmware/efi/efi-pstore.c
> @@ -161,7 +161,7 @@ static int efi_pstore_scan_sysfs_exit(struct efivar_entry *pos,
> *
> * @record: pstore record to pass to callback
> *
> - * You MUST call efivar_enter_iter_begin() before this function, and
> + * You MUST call efivar_entry_iter_begin() before this function, and
> * efivar_entry_iter_end() afterwards.
> *
> */
> diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
> index 7576450c8254..f415cf863ee0 100644
> --- a/drivers/firmware/efi/efivars.c
> +++ b/drivers/firmware/efi/efivars.c
> @@ -88,9 +88,15 @@ efivar_attr_read(struct efivar_entry *entry, char *buf)
> if (!entry || !buf)
> return -EINVAL;
>
> + if (efivar_entry_iter_begin())
> + return -EINTR;
> +
> var->DataSize = 1024;
> - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
> + if (__efivar_entry_get(entry, &var->Attributes, &var->DataSize,
> + var->Data)) {
> + efivar_entry_iter_end();
> return -EIO;
> + }
>
> if (var->Attributes & EFI_VARIABLE_NON_VOLATILE)
> str += sprintf(str, "EFI_VARIABLE_NON_VOLATILE\n");
> @@ -109,6 +115,8 @@ efivar_attr_read(struct efivar_entry *entry, char *buf)
> "EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS\n");
> if (var->Attributes & EFI_VARIABLE_APPEND_WRITE)
> str += sprintf(str, "EFI_VARIABLE_APPEND_WRITE\n");
> +
> + efivar_entry_iter_end();
> return str - buf;
> }
>
> @@ -121,11 +129,19 @@ efivar_size_read(struct efivar_entry *entry, char *buf)
> if (!entry || !buf)
> return -EINVAL;
>
> + if (efivar_entry_iter_begin())
> + return -EINTR;
> +
> var->DataSize = 1024;
> - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
> + if (__efivar_entry_get(entry, &var->Attributes, &var->DataSize,
> + var->Data)) {
> + efivar_entry_iter_end();
> return -EIO;
> + }
>
> str += sprintf(str, "0x%lx\n", var->DataSize);
> +
> + efivar_entry_iter_end();
> return str - buf;
> }
>
> @@ -137,11 +153,19 @@ efivar_data_read(struct efivar_entry *entry, char *buf)
> if (!entry || !buf)
> return -EINVAL;
>
> + if (efivar_entry_iter_begin())
> + return -EINTR;
> +
> var->DataSize = 1024;
> - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
> + if (__efivar_entry_get(entry, &var->Attributes, &var->DataSize,
> + var->Data)) {
> + efivar_entry_iter_end();
> return -EIO;
> + }
>
> memcpy(buf, var->Data, var->DataSize);
> +
> + efivar_entry_iter_end();
> return var->DataSize;
> }
>
> @@ -197,13 +221,21 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
> efi_guid_t vendor;
> u32 attributes;
> u8 *data;
> - int err;
> + int err = 0;
> +
> + if (!entry || !buf)
> + return -EINVAL;
> +
> + if (efivar_entry_iter_begin())
> + return -EINTR;
>
> if (in_compat_syscall()) {
> struct compat_efi_variable *compat;
>
> - if (count != sizeof(*compat))
> - return -EINVAL;
> + if (count != sizeof(*compat)) {
> + err = -EINVAL;
> + goto out;
> + }
>
> compat = (struct compat_efi_variable *)buf;
> attributes = compat->Attributes;
> @@ -214,12 +246,14 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
>
> err = sanity_check(var, name, vendor, size, attributes, data);
> if (err)
> - return err;
> + goto out;
>
> copy_out_compat(&entry->var, compat);
> } else {
> - if (count != sizeof(struct efi_variable))
> - return -EINVAL;
> + if (count != sizeof(struct efi_variable)) {
> + err = -EINVAL;
> + goto out;
> + }
>
> new_var = (struct efi_variable *)buf;
>
> @@ -231,18 +265,20 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
>
> err = sanity_check(var, name, vendor, size, attributes, data);
> if (err)
> - return err;
> + goto out;
>
> memcpy(&entry->var, new_var, count);
> }
>
> - err = efivar_entry_set(entry, attributes, size, data, NULL);
> + err = __efivar_entry_set(entry, attributes, size, data, NULL);
> if (err) {
> printk(KERN_WARNING "efivars: set_variable() failed: status=%d\n", err);
> - return -EIO;
> + err = -EIO;
> }
>
> - return count;
> +out:
> + efivar_entry_iter_end();
> + return err ?: count;
> }
>
> static ssize_t
> @@ -255,10 +291,15 @@ efivar_show_raw(struct efivar_entry *entry, char *buf)
> if (!entry || !buf)
> return 0;
>
> + if (efivar_entry_iter_begin())
> + return -EINTR;
> +
> var->DataSize = 1024;
> - if (efivar_entry_get(entry, &entry->var.Attributes,
> - &entry->var.DataSize, entry->var.Data))
> + if (__efivar_entry_get(entry, &var->Attributes, &var->DataSize,
> + var->Data)) {
> + efivar_entry_iter_end();
> return -EIO;
> + }
>
> if (in_compat_syscall()) {
> compat = (struct compat_efi_variable *)buf;
> @@ -276,6 +317,7 @@ efivar_show_raw(struct efivar_entry *entry, char *buf)
> memcpy(buf, var, size);
> }
>
> + efivar_entry_iter_end();
> return size;
> }
>
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 436d1776bc7b..4a47e20a7667 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -636,7 +636,7 @@ int efivar_entry_delete(struct efivar_entry *entry)
> EXPORT_SYMBOL_GPL(efivar_entry_delete);
>
> /**
> - * efivar_entry_set - call set_variable()
> + * __efivar_entry_set - call set_variable()
> * @entry: entry containing the EFI variable to write
> * @attributes: variable attributes
> * @size: size of @data buffer
> @@ -655,8 +655,12 @@ EXPORT_SYMBOL_GPL(efivar_entry_delete);
> * Returns 0 on success, -EINTR if we can't grab the semaphore,
> * -EEXIST if a lookup is performed and the entry already exists on
> * the list, or a converted EFI status code if set_variable() fails.
> + *
> + * The caller MUST call efivar_entry_iter_begin() and
> + * efivar_entry_iter_end() before and after the invocation of this
> + * function, respectively.
> */
> -int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
> +int __efivar_entry_set(struct efivar_entry *entry, u32 attributes,
> unsigned long size, void *data, struct list_head *head)
> {
> const struct efivar_operations *ops;
> @@ -664,9 +668,6 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
> efi_char16_t *name = entry->var.VariableName;
> efi_guid_t vendor = entry->var.VendorGuid;
>
> - if (down_interruptible(&efivars_lock))
> - return -EINTR;
> -
> if (!__efivars) {
> up(&efivars_lock);
> return -EINVAL;
> @@ -682,10 +683,28 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
> status = ops->set_variable(name, &vendor,
> attributes, size, data);
>
> - up(&efivars_lock);
> -
> return efi_status_to_err(status);
> +}
> +EXPORT_SYMBOL_GPL(__efivar_entry_set);
>
> +/**
> + * efivar_entry_set - call set_variable()
> + *
> + * This function takes efivars_lock and calls __efivar_entry_set()
> + */
> +int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
> + unsigned long size, void *data, struct list_head *head)
> +{
> + int ret;
> +
> + if (down_interruptible(&efivars_lock))
> + return -EINTR;
> +
> + ret = __efivar_entry_set(entry, attributes, size, data, head);
> +
> + up(&efivars_lock);
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(efivar_entry_set);
>
> @@ -914,22 +933,16 @@ EXPORT_SYMBOL_GPL(__efivar_entry_get);
> int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
> unsigned long *size, void *data)
> {
> - efi_status_t status;
> + int ret;
>
> if (down_interruptible(&efivars_lock))
> return -EINTR;
>
> - if (!__efivars) {
> - up(&efivars_lock);
> - return -EINVAL;
> - }
> + ret = __efivar_entry_get(entry, attributes, size, data);
>
> - status = __efivars->ops->get_variable(entry->var.VariableName,
> - &entry->var.VendorGuid,
> - attributes, size, data);
> up(&efivars_lock);
>
> - return efi_status_to_err(status);
> + return ret;
> }
> EXPORT_SYMBOL_GPL(efivar_entry_get);
>
> @@ -1071,7 +1084,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_iter_end);
> * entry on the list. It is safe for @func to remove entries in the
> * list via efivar_entry_delete().
> *
> - * You MUST call efivar_enter_iter_begin() before this function, and
> + * You MUST call efivar_entry_iter_begin() before this function, and
> * efivar_entry_iter_end() afterwards.
> *
> * It is possible to begin iteration from an arbitrary entry within
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 7efd7072cca5..5c3db088d375 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1414,6 +1414,8 @@ int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
> unsigned long *size, void *data);
> int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
> unsigned long *size, void *data);
> +int __efivar_entry_set(struct efivar_entry *entry, u32 attributes,
> + unsigned long size, void *data, struct list_head *head);
> int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
> unsigned long size, void *data, struct list_head *head);
> int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
> --
> 2.20.1
