* [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs
@ 2020-03-03 8:55 Vladis Dronov
2020-03-03 9:15 ` Ard Biesheuvel
2020-03-04 14:07 ` [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs joeyli
0 siblings, 2 replies; 23+ messages in thread
From: Vladis Dronov @ 2020-03-03 8:55 UTC (permalink / raw)
To: Ard Biesheuvel, linux-efi; +Cc: linux-kernel
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@hpe.com> and the LTP testsuite
Signed-off-by: Vladis Dronov <vdronov@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-03 8:55 [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs Vladis Dronov
@ 2020-03-03 9:15 ` Ard Biesheuvel
2020-03-03 10:14 ` Vladis Dronov
` (3 more replies)
2020-03-04 14:07 ` [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs joeyli
1 sibling, 4 replies; 23+ messages in thread
From: Ard Biesheuvel @ 2020-03-03 9:15 UTC (permalink / raw)
To: Vladis Dronov; +Cc: linux-efi, Linux Kernel Mailing List
On Tue, 3 Mar 2020 at 09:55, Vladis Dronov <vdronov@redhat.com> 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@hpe.com> and the LTP testsuite
> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
Wouldn't it be easier to pass a var_data_size stack variable into
efivar_entry_get(), and only update the value in 'var' if it is <=
1024?
> ---
> 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
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-03 9:15 ` Ard Biesheuvel
@ 2020-03-03 10:14 ` Vladis Dronov
2020-03-03 10:20 ` Ard Biesheuvel
2020-03-03 10:24 ` Vladis Dronov
2020-03-04 15:45 ` Vladis Dronov
` (2 subsequent siblings)
3 siblings, 2 replies; 23+ messages in thread
From: Vladis Dronov @ 2020-03-03 10:14 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi, Linux Kernel Mailing List
Hello, Ard, all,
----- Original Message -----
> From: "Ard Biesheuvel" <ardb@kernel.org>
> To: "Vladis Dronov" <vdronov@redhat.com>
> Cc: "linux-efi" <linux-efi@vger.kernel.org>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
> Sent: Tuesday, March 3, 2020 10:15:41 AM
> Subject: Re: [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs
>
> On Tue, 3 Mar 2020 at 09:55, Vladis Dronov <vdronov@redhat.com> 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@hpe.com> and the LTP testsuite
> > Signed-off-by: Vladis Dronov <vdronov@redhat.com>
>
> Wouldn't it be easier to pass a var_data_size stack variable into
> efivar_entry_get(), and only update the value in 'var' if it is <=
> 1024?
>
I was thinking about this approach, but this way we still do not protect
var from a concurrent access. For example, efivar_data_read() can race
with itself:
// reading var size 5
efivar_data_read()
efivar_entry_get()
// reading the same var
efivar_data_read()
var->DataSize = 1024;
efivar_entry_get()
// var->DataSize is SUDDENLY 1024
memcpy(buf, var->Data, var->DataSize);
return var->DataSize;
Also efivar read functions still can race with the write function
efivar_store_raw(). Surely, the race window is much smaller but it is there.
I strongly believe we need to protect all data accesses here with a lock.
May be not in a way I suggest, may be by a per-var mutex, but I believe
this is overcomplication.
> > ---
> > 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-03 10:14 ` Vladis Dronov
@ 2020-03-03 10:20 ` Ard Biesheuvel
2020-03-03 10:24 ` Vladis Dronov
1 sibling, 0 replies; 23+ messages in thread
From: Ard Biesheuvel @ 2020-03-03 10:20 UTC (permalink / raw)
To: Vladis Dronov; +Cc: linux-efi, Linux Kernel Mailing List
On Tue, 3 Mar 2020 at 11:14, Vladis Dronov <vdronov@redhat.com> wrote:
>
> Hello, Ard, all,
>
> ----- Original Message -----
> > From: "Ard Biesheuvel" <ardb@kernel.org>
> > To: "Vladis Dronov" <vdronov@redhat.com>
> > Cc: "linux-efi" <linux-efi@vger.kernel.org>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
> > Sent: Tuesday, March 3, 2020 10:15:41 AM
> > Subject: Re: [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs
> >
> > On Tue, 3 Mar 2020 at 09:55, Vladis Dronov <vdronov@redhat.com> 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@hpe.com> and the LTP testsuite
> > > Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> >
> > Wouldn't it be easier to pass a var_data_size stack variable into
> > efivar_entry_get(), and only update the value in 'var' if it is <=
> > 1024?
> >
>
> I was thinking about this approach, but this way we still do not protect
> var from a concurrent access. For example, efivar_data_read() can race
> with itself:
>
> // reading var size 5
> efivar_data_read()
> efivar_entry_get()
> // reading the same var
> efivar_data_read()
> var->DataSize = 1024;
> efivar_entry_get()
> // var->DataSize is SUDDENLY 1024
> memcpy(buf, var->Data, var->DataSize);
I'd assume that the memcpy() is not executed if var_data_size > 1024,
and var->DataSize is not assigned in that case either.
> return var->DataSize;
>
> Also efivar read functions still can race with the write function
> efivar_store_raw(). Surely, the race window is much smaller but it is there.
> I strongly believe we need to protect all data accesses here with a lock.
>
> May be not in a way I suggest, may be by a per-var mutex, but I believe
> this is overcomplication.
>
> > > ---
> > > 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
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-03 10:14 ` Vladis Dronov
2020-03-03 10:20 ` Ard Biesheuvel
@ 2020-03-03 10:24 ` Vladis Dronov
2020-03-04 6:51 ` joeyli
1 sibling, 1 reply; 23+ messages in thread
From: Vladis Dronov @ 2020-03-03 10:24 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi, Linux Kernel Mailing List
Hello, Ard, all,
> > Wouldn't it be easier to pass a var_data_size stack variable into
> > efivar_entry_get(), and only update the value in 'var' if it is <=
> > 1024?
> >
>
> I was thinking about this approach, but this way we still do not protect
> var from a concurrent access. For example, efivar_data_read() can race
> with itself:
Oh, indeed, this race is not possible the way you sugget with a var_data_size
stack variable. Unfortunately, AFAIU, the read/write race stays:
> ... efivar read functions still can race with the write function
> efivar_store_raw(). Surely, the race window is much smaller but it is there.
> I strongly believe we need to protect all data accesses here with a lock.
Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-03 10:24 ` Vladis Dronov
@ 2020-03-04 6:51 ` joeyli
0 siblings, 0 replies; 23+ messages in thread
From: joeyli @ 2020-03-04 6:51 UTC (permalink / raw)
To: Vladis Dronov; +Cc: Ard Biesheuvel, linux-efi, Linux Kernel Mailing List
Hi all,
On Tue, Mar 03, 2020 at 05:24:58AM -0500, Vladis Dronov wrote:
> Hello, Ard, all,
>
> > > Wouldn't it be easier to pass a var_data_size stack variable into
> > > efivar_entry_get(), and only update the value in 'var' if it is <=
> > > 1024?
> > >
> >
> > I was thinking about this approach, but this way we still do not protect
> > var from a concurrent access. For example, efivar_data_read() can race
> > with itself:
>
> Oh, indeed, this race is not possible the way you sugget with a var_data_size
> stack variable. Unfortunately, AFAIU, the read/write race stays:
>
> > ... efivar read functions still can race with the write function
> > efivar_store_raw(). Surely, the race window is much smaller but it is there.
> > I strongly believe we need to protect all data accesses here with a lock.
>
Looks that kernel uses EFI protocol to query variable everytime, then
why should kernel keeps a copy of variable data size, data and attributes in
memory? It makes sense to keep VariableName and VendorGuid, but why data?
The efi_variable can be used to interactive with userland. But we do not
need to keep a data copy in efi_variable with efivar_entry. e.g. The
efivarfs_file_read() allocates a buffer for reading variable instead
of using efi_variable->Data.
Regards
Joey Lee
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-03 8:55 [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs Vladis Dronov
2020-03-03 9:15 ` Ard Biesheuvel
@ 2020-03-04 14:07 ` joeyli
2020-03-04 16:06 ` Vladis Dronov
1 sibling, 1 reply; 23+ messages in thread
From: joeyli @ 2020-03-04 14:07 UTC (permalink / raw)
To: Vladis Dronov; +Cc: Ard Biesheuvel, linux-efi, linux-kernel
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@hpe.com> and the LTP testsuite
> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
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@suse.com>
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-03 9:15 ` Ard Biesheuvel
2020-03-03 10:14 ` Vladis Dronov
@ 2020-03-04 15:45 ` Vladis Dronov
2020-03-04 15:47 ` Ard Biesheuvel
2020-03-04 15:49 ` [PATCH v2] " Vladis Dronov
2020-03-05 8:40 ` [PATCH v3 0/3] efi: fix a race and add a sanity check Vladis Dronov
3 siblings, 1 reply; 23+ messages in thread
From: Vladis Dronov @ 2020-03-04 15:45 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi, Linux Kernel Mailing List
Hello, Ard,
> Wouldn't it be easier to pass a var_data_size stack variable into
> efivar_entry_get(), and only update the value in 'var' if it is <=
> 1024?
I have prepared a v2 patch with an approach you suggest and will send it
out shortly. It indeed simpler and fixes only the overflow bug mentioned.
Could you, please, review it and if you like it, probably, accept it?
In case I've implemented your idea incorrectly, could you, please,
correct me?
Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-04 15:45 ` Vladis Dronov
@ 2020-03-04 15:47 ` Ard Biesheuvel
0 siblings, 0 replies; 23+ messages in thread
From: Ard Biesheuvel @ 2020-03-04 15:47 UTC (permalink / raw)
To: Vladis Dronov; +Cc: linux-efi, Linux Kernel Mailing List
On Wed, 4 Mar 2020 at 16:45, Vladis Dronov <vdronov@redhat.com> wrote:
>
> Hello, Ard,
>
> > Wouldn't it be easier to pass a var_data_size stack variable into
> > efivar_entry_get(), and only update the value in 'var' if it is <=
> > 1024?
>
> I have prepared a v2 patch with an approach you suggest and will send it
> out shortly. It indeed simpler and fixes only the overflow bug mentioned.
>
> Could you, please, review it and if you like it, probably, accept it?
> In case I've implemented your idea incorrectly, could you, please,
> correct me?
>
Absolutely! Thanks for taking the time to fix these bugs, your
contributions are most welcome (and apologies if my responses
suggested otherwise)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-03 9:15 ` Ard Biesheuvel
2020-03-03 10:14 ` Vladis Dronov
2020-03-04 15:45 ` Vladis Dronov
@ 2020-03-04 15:49 ` Vladis Dronov
2020-03-04 15:57 ` Ard Biesheuvel
2020-03-05 6:01 ` joeyli
2020-03-05 8:40 ` [PATCH v3 0/3] efi: fix a race and add a sanity check Vladis Dronov
3 siblings, 2 replies; 23+ messages in thread
From: Vladis Dronov @ 2020-03-04 15:49 UTC (permalink / raw)
To: Ard Biesheuvel, linux-efi, joeyli; +Cc: linux-kernel
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 using a local variable for a var's data buffer size so it
does not get overwritten. Also add a sanity check to efivar_store_raw().
Reported-by: Bob Sanders <bob.sanders@hpe.com> and the LTP testsuite
Signed-off-by: Vladis Dronov <vdronov@redhat.com>
---
drivers/firmware/efi/efi-pstore.c | 2 +-
drivers/firmware/efi/efivars.c | 32 ++++++++++++++++++++++---------
drivers/firmware/efi/vars.c | 2 +-
3 files changed, 25 insertions(+), 11 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..16a617f9c5cf 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -83,13 +83,16 @@ static ssize_t
efivar_attr_read(struct efivar_entry *entry, char *buf)
{
struct efi_variable *var = &entry->var;
+ unsigned long size = sizeof(var->Data);
char *str = buf;
+ int ret;
if (!entry || !buf)
return -EINVAL;
- var->DataSize = 1024;
- if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
+ ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
+ var->DataSize = size;
+ if (ret)
return -EIO;
if (var->Attributes & EFI_VARIABLE_NON_VOLATILE)
@@ -116,13 +119,16 @@ static ssize_t
efivar_size_read(struct efivar_entry *entry, char *buf)
{
struct efi_variable *var = &entry->var;
+ unsigned long size = sizeof(var->Data);
char *str = buf;
+ int ret;
if (!entry || !buf)
return -EINVAL;
- var->DataSize = 1024;
- if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
+ ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
+ var->DataSize = size;
+ if (ret)
return -EIO;
str += sprintf(str, "0x%lx\n", var->DataSize);
@@ -133,12 +139,15 @@ static ssize_t
efivar_data_read(struct efivar_entry *entry, char *buf)
{
struct efi_variable *var = &entry->var;
+ unsigned long size = sizeof(var->Data);
+ int ret;
if (!entry || !buf)
return -EINVAL;
- var->DataSize = 1024;
- if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
+ ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
+ var->DataSize = size;
+ if (ret)
return -EIO;
memcpy(buf, var->Data, var->DataSize);
@@ -199,6 +208,9 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
u8 *data;
int err;
+ if (!entry || !buf)
+ return -EINVAL;
+
if (in_compat_syscall()) {
struct compat_efi_variable *compat;
@@ -250,14 +262,16 @@ efivar_show_raw(struct efivar_entry *entry, char *buf)
{
struct efi_variable *var = &entry->var;
struct compat_efi_variable *compat;
+ unsigned long datasize = sizeof(var->Data);
size_t size;
+ int ret;
if (!entry || !buf)
return 0;
- var->DataSize = 1024;
- if (efivar_entry_get(entry, &entry->var.Attributes,
- &entry->var.DataSize, entry->var.Data))
+ ret = efivar_entry_get(entry, &var->Attributes, &datasize, var->Data);
+ var->DataSize = size;
+ if (ret)
return -EIO;
if (in_compat_syscall()) {
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 436d1776bc7b..5f2a4d162795 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -1071,7 +1071,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
--
2.20.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-04 15:49 ` [PATCH v2] " Vladis Dronov
@ 2020-03-04 15:57 ` Ard Biesheuvel
2020-03-04 17:18 ` Vladis Dronov
2020-03-05 6:01 ` joeyli
1 sibling, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-03-04 15:57 UTC (permalink / raw)
To: Vladis Dronov; +Cc: linux-efi, joeyli, Linux Kernel Mailing List
On Wed, 4 Mar 2020 at 16:50, Vladis Dronov <vdronov@redhat.com> 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 using a local variable for a var's data buffer size so it
> does not get overwritten. Also add a sanity check to efivar_store_raw().
>
> Reported-by: Bob Sanders <bob.sanders@hpe.com> and the LTP testsuite
> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> ---
> drivers/firmware/efi/efi-pstore.c | 2 +-
> drivers/firmware/efi/efivars.c | 32 ++++++++++++++++++++++---------
> drivers/firmware/efi/vars.c | 2 +-
> 3 files changed, 25 insertions(+), 11 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.
> *
> */
This hunk can be dropped now, I guess
> diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
> index 7576450c8254..16a617f9c5cf 100644
> --- a/drivers/firmware/efi/efivars.c
> +++ b/drivers/firmware/efi/efivars.c
> @@ -83,13 +83,16 @@ static ssize_t
> efivar_attr_read(struct efivar_entry *entry, char *buf)
> {
> struct efi_variable *var = &entry->var;
> + unsigned long size = sizeof(var->Data);
> char *str = buf;
> + int ret;
>
> if (!entry || !buf)
> return -EINVAL;
>
> - var->DataSize = 1024;
> - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
> + ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> + var->DataSize = size;
For my understanding, could you explain why we do the assignment here?
Does var->DataSize matter in this case? Can it deviate from 1024?
> + if (ret)
> return -EIO;
>
> if (var->Attributes & EFI_VARIABLE_NON_VOLATILE)
> @@ -116,13 +119,16 @@ static ssize_t
> efivar_size_read(struct efivar_entry *entry, char *buf)
> {
> struct efi_variable *var = &entry->var;
> + unsigned long size = sizeof(var->Data);
> char *str = buf;
> + int ret;
>
> if (!entry || !buf)
> return -EINVAL;
>
> - var->DataSize = 1024;
> - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
> + ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> + var->DataSize = size;
> + if (ret)
> return -EIO;
>
> str += sprintf(str, "0x%lx\n", var->DataSize);
> @@ -133,12 +139,15 @@ static ssize_t
> efivar_data_read(struct efivar_entry *entry, char *buf)
> {
> struct efi_variable *var = &entry->var;
> + unsigned long size = sizeof(var->Data);
> + int ret;
>
> if (!entry || !buf)
> return -EINVAL;
>
> - var->DataSize = 1024;
> - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
> + ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> + var->DataSize = size;
> + if (ret)
> return -EIO;
>
> memcpy(buf, var->Data, var->DataSize);
> @@ -199,6 +208,9 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
> u8 *data;
> int err;
>
> + if (!entry || !buf)
> + return -EINVAL;
> +
So what are we sanity checking here? When might this occur? Does it
need to be in the same patch?
> if (in_compat_syscall()) {
> struct compat_efi_variable *compat;
>
> @@ -250,14 +262,16 @@ efivar_show_raw(struct efivar_entry *entry, char *buf)
> {
> struct efi_variable *var = &entry->var;
> struct compat_efi_variable *compat;
> + unsigned long datasize = sizeof(var->Data);
> size_t size;
> + int ret;
>
> if (!entry || !buf)
> return 0;
>
> - var->DataSize = 1024;
> - if (efivar_entry_get(entry, &entry->var.Attributes,
> - &entry->var.DataSize, entry->var.Data))
> + ret = efivar_entry_get(entry, &var->Attributes, &datasize, var->Data);
> + var->DataSize = size;
> + if (ret)
> return -EIO;
>
> if (in_compat_syscall()) {
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 436d1776bc7b..5f2a4d162795 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -1071,7 +1071,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
We can drop this.
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-04 14:07 ` [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs joeyli
@ 2020-03-04 16:06 ` Vladis Dronov
0 siblings, 0 replies; 23+ messages in thread
From: Vladis Dronov @ 2020-03-04 16:06 UTC (permalink / raw)
To: joeyli; +Cc: Ard Biesheuvel, linux-efi, linux-kernel
Hello, Joey, all,
Let me address both of your emails here.
> Looks that kernel uses EFI protocol to query variable everytime, then
> why should kernel keeps a copy of variable data size, data and attributes in
> memory? It makes sense to keep VariableName and VendorGuid, but why data?
>
> The efi_variable can be used to interactive with userland. But we do not
> need to keep a data copy in efi_variable with efivar_entry. e.g. The
> efivarfs_file_read() allocates a buffer for reading variable instead
> of using efi_variable->Data.
Indeed, as far as I understand the code, we keep var's data in a memory. I
cannot tell why such was done when this code was written. Given that this
code is considered "old" and may even be obsoleted, I wouldn't like to start
a deep rewrite, and only focus on fixing bugs, like the one mentioned.
> 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@suse.com>
Thank you for your time on the review! Much appreciated. Still, I've just
sent out a v2 patch (with you in To:) which is indeed simpler and implements
and idea Ard suggested, and fixes only the exact bug mentioned. I'm not sure
which one will be accepted (if any), could you please, to look at v2 also?
Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-04 15:57 ` Ard Biesheuvel
@ 2020-03-04 17:18 ` Vladis Dronov
2020-03-04 17:21 ` Ard Biesheuvel
0 siblings, 1 reply; 23+ messages in thread
From: Vladis Dronov @ 2020-03-04 17:18 UTC (permalink / raw)
To: Ard Biesheuvel, joeyli, linux-efi; +Cc: Linux Kernel Mailing List
Hello, Ard, Joye, all,
----- Original Message -----
> From: "Ard Biesheuvel" <ardb@kernel.org>
> To: "Vladis Dronov" <vdronov@redhat.com>
> Cc: "linux-efi" <linux-efi@vger.kernel.org>, "joeyli" <jlee@suse.com>, "Linux Kernel Mailing List"
> <linux-kernel@vger.kernel.org>
> Sent: Wednesday, March 4, 2020 4:57:16 PM
> Subject: Re: [PATCH v2] efi: fix a race and a buffer overflow while reading efivars via sysfs
>
> On Wed, 4 Mar 2020 at 16:50, Vladis Dronov <vdronov@redhat.com> 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 using a local variable for a var's data buffer size so it
> > does not get overwritten. Also add a sanity check to efivar_store_raw().
> >
> > Reported-by: Bob Sanders <bob.sanders@hpe.com> and the LTP testsuite
> > Signed-off-by: Vladis Dronov <vdronov@redhat.com>
AFAIU, you can modify suggested patches, could you please, add a link here
so further reader has a reference (I forgot to do it myself):
Link: https://lore.kernel.org/linux-efi/20200303085528.27658-1-vdronov@redhat.com/T/#u
> > ---
> > drivers/firmware/efi/efi-pstore.c | 2 +-
> > drivers/firmware/efi/efivars.c | 32 ++++++++++++++++++++++---------
> > drivers/firmware/efi/vars.c | 2 +-
> > 3 files changed, 25 insertions(+), 11 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.
> > *
> > */
>
> This hunk can be dropped now, I guess
I surely do not have much experience in writing upstream patches. But I saw people
doing small fixes like this one, say, commit 589b7289 ("While we are here, the previous
line has some trailing whitespace; clean that up as well"). This is a small mistype
and I just wanted to fix it and did not wanted to allocate a whole commit for that.
I believe a bigger commit is an acceptable place to fix mistypes.
AFAIU, a maintainer can modify suggested patches, so please, feel free to drop this
hunk, if you feel this is a right thing.
> > diff --git a/drivers/firmware/efi/efivars.c
> > b/drivers/firmware/efi/efivars.c
> > index 7576450c8254..16a617f9c5cf 100644
> > --- a/drivers/firmware/efi/efivars.c
> > +++ b/drivers/firmware/efi/efivars.c
> > @@ -83,13 +83,16 @@ static ssize_t
> > efivar_attr_read(struct efivar_entry *entry, char *buf)
> > {
> > struct efi_variable *var = &entry->var;
> > + unsigned long size = sizeof(var->Data);
> > char *str = buf;
> > + int ret;
> >
> > if (!entry || !buf)
> > return -EINVAL;
> >
> > - var->DataSize = 1024;
> > - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize,
> > var->Data))
> > + ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> > + var->DataSize = size;
>
> For my understanding, could you explain why we do the assignment here?
> Does var->DataSize matter in this case? Can it deviate from 1024?
Yes, the other code expects var->DataSize to be set to a real size of a var
after efivar_entry_get() call. For example, efivar_show_raw():
compat->DataSize = var->DataSize;
and efivar_data_read():
memcpy(buf, var->Data, var->DataSize);
return var->DataSize;
Yes, we can change the code to use size here, but this will make struct efi_variable
*var inconsistent (name, guid, data, attr set properly, but not size). It feels so
incorrect to leave this struct inconsistent. I'm not sure that code which calls
efivar_{attr,size,data}_read()/efivar_show_raw() is not using this struct's ->DataSize
field later.
> > + if (ret)
> > return -EIO;
> >
> > if (var->Attributes & EFI_VARIABLE_NON_VOLATILE)
> > @@ -116,13 +119,16 @@ static ssize_t
> > efivar_size_read(struct efivar_entry *entry, char *buf)
> > {
> > struct efi_variable *var = &entry->var;
> > + unsigned long size = sizeof(var->Data);
> > char *str = buf;
> > + int ret;
> >
> > if (!entry || !buf)
> > return -EINVAL;
> >
> > - var->DataSize = 1024;
> > - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize,
> > var->Data))
> > + ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> > + var->DataSize = size;
> > + if (ret)
> > return -EIO;
> >
> > str += sprintf(str, "0x%lx\n", var->DataSize);
> > @@ -133,12 +139,15 @@ static ssize_t
> > efivar_data_read(struct efivar_entry *entry, char *buf)
> > {
> > struct efi_variable *var = &entry->var;
> > + unsigned long size = sizeof(var->Data);
> > + int ret;
> >
> > if (!entry || !buf)
> > return -EINVAL;
> >
> > - var->DataSize = 1024;
> > - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize,
> > var->Data))
> > + ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> > + var->DataSize = size;
> > + if (ret)
> > return -EIO;
> >
> > memcpy(buf, var->Data, var->DataSize);
> > @@ -199,6 +208,9 @@ efivar_store_raw(struct efivar_entry *entry, const char
> > *buf, size_t count)
> > u8 *data;
> > int err;
> >
> > + if (!entry || !buf)
> > + return -EINVAL;
> > +
>
> So what are we sanity checking here? When might this occur? Does it
> need to be in the same patch?
efivar_{attr,size,data}_read()/efivar_show_raw() has this check, I believe
it is reasonable to add it here too. In case entry or buf happen to be NULL
it will lead to a NULL-deref later:
compat = (struct compat_efi_variable *)buf;
memcpy(compat->VariableName, var->VariableName, EFI_VAR_NAME_LEN);
I see this as more-or-less related and too small for a whole separate commit.
Please, feel free to drop this hunk, if you believe this is not correct. Would
you like me to send a separate patch adding the check above in this case?
> > if (in_compat_syscall()) {
> > struct compat_efi_variable *compat;
> >
> > @@ -250,14 +262,16 @@ efivar_show_raw(struct efivar_entry *entry, char
> > *buf)
> > {
> > struct efi_variable *var = &entry->var;
> > struct compat_efi_variable *compat;
> > + unsigned long datasize = sizeof(var->Data);
> > size_t size;
> > + int ret;
> >
> > if (!entry || !buf)
> > return 0;
> >
> > - var->DataSize = 1024;
> > - if (efivar_entry_get(entry, &entry->var.Attributes,
> > - &entry->var.DataSize, entry->var.Data))
> > + ret = efivar_entry_get(entry, &var->Attributes, &datasize,
> > var->Data);
> > + var->DataSize = size;
> > + if (ret)
> > return -EIO;
> >
> > if (in_compat_syscall()) {
> > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > index 436d1776bc7b..5f2a4d162795 100644
> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -1071,7 +1071,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
>
> We can drop this.
>
> > --
> > 2.20.1
Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-04 17:18 ` Vladis Dronov
@ 2020-03-04 17:21 ` Ard Biesheuvel
0 siblings, 0 replies; 23+ messages in thread
From: Ard Biesheuvel @ 2020-03-04 17:21 UTC (permalink / raw)
To: Vladis Dronov; +Cc: joeyli, linux-efi, Linux Kernel Mailing List
On Wed, 4 Mar 2020 at 18:18, Vladis Dronov <vdronov@redhat.com> wrote:
>
> Hello, Ard, Joye, all,
>
> ----- Original Message -----
> > From: "Ard Biesheuvel" <ardb@kernel.org>
> > To: "Vladis Dronov" <vdronov@redhat.com>
> > Cc: "linux-efi" <linux-efi@vger.kernel.org>, "joeyli" <jlee@suse.com>, "Linux Kernel Mailing List"
> > <linux-kernel@vger.kernel.org>
> > Sent: Wednesday, March 4, 2020 4:57:16 PM
> > Subject: Re: [PATCH v2] efi: fix a race and a buffer overflow while reading efivars via sysfs
> >
> > On Wed, 4 Mar 2020 at 16:50, Vladis Dronov <vdronov@redhat.com> 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 using a local variable for a var's data buffer size so it
> > > does not get overwritten. Also add a sanity check to efivar_store_raw().
> > >
> > > Reported-by: Bob Sanders <bob.sanders@hpe.com> and the LTP testsuite
> > > Signed-off-by: Vladis Dronov <vdronov@redhat.com>
>
> AFAIU, you can modify suggested patches, could you please, add a link here
> so further reader has a reference (I forgot to do it myself):
>
> Link: https://lore.kernel.org/linux-efi/20200303085528.27658-1-vdronov@redhat.com/T/#u
>
> > > ---
> > > drivers/firmware/efi/efi-pstore.c | 2 +-
> > > drivers/firmware/efi/efivars.c | 32 ++++++++++++++++++++++---------
> > > drivers/firmware/efi/vars.c | 2 +-
> > > 3 files changed, 25 insertions(+), 11 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.
> > > *
> > > */
> >
> > This hunk can be dropped now, I guess
>
> I surely do not have much experience in writing upstream patches. But I saw people
> doing small fixes like this one, say, commit 589b7289 ("While we are here, the previous
> line has some trailing whitespace; clean that up as well"). This is a small mistype
> and I just wanted to fix it and did not wanted to allocate a whole commit for that.
> I believe a bigger commit is an acceptable place to fix mistypes.
>
> AFAIU, a maintainer can modify suggested patches, so please, feel free to drop this
> hunk, if you feel this is a right thing.
>
I am not going to perform surgery on your patches. Please drop this
hunk (and the one at the end) in the next version.
> > > diff --git a/drivers/firmware/efi/efivars.c
> > > b/drivers/firmware/efi/efivars.c
> > > index 7576450c8254..16a617f9c5cf 100644
> > > --- a/drivers/firmware/efi/efivars.c
> > > +++ b/drivers/firmware/efi/efivars.c
> > > @@ -83,13 +83,16 @@ static ssize_t
> > > efivar_attr_read(struct efivar_entry *entry, char *buf)
> > > {
> > > struct efi_variable *var = &entry->var;
> > > + unsigned long size = sizeof(var->Data);
> > > char *str = buf;
> > > + int ret;
> > >
> > > if (!entry || !buf)
> > > return -EINVAL;
> > >
> > > - var->DataSize = 1024;
> > > - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize,
> > > var->Data))
> > > + ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> > > + var->DataSize = size;
> >
> > For my understanding, could you explain why we do the assignment here?
> > Does var->DataSize matter in this case? Can it deviate from 1024?
>
> Yes, the other code expects var->DataSize to be set to a real size of a var
> after efivar_entry_get() call. For example, efivar_show_raw():
>
> compat->DataSize = var->DataSize;
>
> and efivar_data_read():
>
> memcpy(buf, var->Data, var->DataSize);
> return var->DataSize;
>
> Yes, we can change the code to use size here, but this will make struct efi_variable
> *var inconsistent (name, guid, data, attr set properly, but not size). It feels so
> incorrect to leave this struct inconsistent. I'm not sure that code which calls
> efivar_{attr,size,data}_read()/efivar_show_raw() is not using this struct's ->DataSize
> field later.
>
OK, that makes sense.
> > > + if (ret)
> > > return -EIO;
> > >
> > > if (var->Attributes & EFI_VARIABLE_NON_VOLATILE)
> > > @@ -116,13 +119,16 @@ static ssize_t
> > > efivar_size_read(struct efivar_entry *entry, char *buf)
> > > {
> > > struct efi_variable *var = &entry->var;
> > > + unsigned long size = sizeof(var->Data);
> > > char *str = buf;
> > > + int ret;
> > >
> > > if (!entry || !buf)
> > > return -EINVAL;
> > >
> > > - var->DataSize = 1024;
> > > - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize,
> > > var->Data))
> > > + ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> > > + var->DataSize = size;
> > > + if (ret)
> > > return -EIO;
> > >
> > > str += sprintf(str, "0x%lx\n", var->DataSize);
> > > @@ -133,12 +139,15 @@ static ssize_t
> > > efivar_data_read(struct efivar_entry *entry, char *buf)
> > > {
> > > struct efi_variable *var = &entry->var;
> > > + unsigned long size = sizeof(var->Data);
> > > + int ret;
> > >
> > > if (!entry || !buf)
> > > return -EINVAL;
> > >
> > > - var->DataSize = 1024;
> > > - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize,
> > > var->Data))
> > > + ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> > > + var->DataSize = size;
> > > + if (ret)
> > > return -EIO;
> > >
> > > memcpy(buf, var->Data, var->DataSize);
> > > @@ -199,6 +208,9 @@ efivar_store_raw(struct efivar_entry *entry, const char
> > > *buf, size_t count)
> > > u8 *data;
> > > int err;
> > >
> > > + if (!entry || !buf)
> > > + return -EINVAL;
> > > +
> >
> > So what are we sanity checking here? When might this occur? Does it
> > need to be in the same patch?
>
> efivar_{attr,size,data}_read()/efivar_show_raw() has this check, I believe
> it is reasonable to add it here too. In case entry or buf happen to be NULL
> it will lead to a NULL-deref later:
>
> compat = (struct compat_efi_variable *)buf;
> memcpy(compat->VariableName, var->VariableName, EFI_VAR_NAME_LEN);
>
> I see this as more-or-less related and too small for a whole separate commit.
> Please, feel free to drop this hunk, if you believe this is not correct. Would
> you like me to send a separate patch adding the check above in this case?
>
Yes, please. Make it a two-piece series with a cover letter.
> > > if (in_compat_syscall()) {
> > > struct compat_efi_variable *compat;
> > >
> > > @@ -250,14 +262,16 @@ efivar_show_raw(struct efivar_entry *entry, char
> > > *buf)
> > > {
> > > struct efi_variable *var = &entry->var;
> > > struct compat_efi_variable *compat;
> > > + unsigned long datasize = sizeof(var->Data);
> > > size_t size;
> > > + int ret;
> > >
> > > if (!entry || !buf)
> > > return 0;
> > >
> > > - var->DataSize = 1024;
> > > - if (efivar_entry_get(entry, &entry->var.Attributes,
> > > - &entry->var.DataSize, entry->var.Data))
> > > + ret = efivar_entry_get(entry, &var->Attributes, &datasize,
> > > var->Data);
> > > + var->DataSize = size;
> > > + if (ret)
> > > return -EIO;
> > >
> > > if (in_compat_syscall()) {
> > > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > > index 436d1776bc7b..5f2a4d162795 100644
> > > --- a/drivers/firmware/efi/vars.c
> > > +++ b/drivers/firmware/efi/vars.c
> > > @@ -1071,7 +1071,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
> >
> > We can drop this.
... or make it a 3 piece series if you *really* want to clean up the
whitespace :-)
> >
> > > --
> > > 2.20.1
>
> Best regards,
> Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-04 15:49 ` [PATCH v2] " Vladis Dronov
2020-03-04 15:57 ` Ard Biesheuvel
@ 2020-03-05 6:01 ` joeyli
2020-03-05 6:17 ` Vladis Dronov
1 sibling, 1 reply; 23+ messages in thread
From: joeyli @ 2020-03-05 6:01 UTC (permalink / raw)
To: Vladis Dronov; +Cc: Ard Biesheuvel, linux-efi, linux-kernel
Hi Vladis,
On Wed, Mar 04, 2020 at 04:49:36PM +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 using a local variable for a var's data buffer size so it
> does not get overwritten. Also add a sanity check to efivar_store_raw().
>
> Reported-by: Bob Sanders <bob.sanders@hpe.com> and the LTP testsuite
> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> ---
> drivers/firmware/efi/efi-pstore.c | 2 +-
> drivers/firmware/efi/efivars.c | 32 ++++++++++++++++++++++---------
> drivers/firmware/efi/vars.c | 2 +-
> 3 files changed, 25 insertions(+), 11 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
[...snip]
> @@ -250,14 +262,16 @@ efivar_show_raw(struct efivar_entry *entry, char *buf)
> {
> struct efi_variable *var = &entry->var;
> struct compat_efi_variable *compat;
> + unsigned long datasize = sizeof(var->Data);
> size_t size;
> + int ret;
>
> if (!entry || !buf)
> return 0;
>
> - var->DataSize = 1024;
> - if (efivar_entry_get(entry, &entry->var.Attributes,
> - &entry->var.DataSize, entry->var.Data))
> + ret = efivar_entry_get(entry, &var->Attributes, &datasize, var->Data);
> + var->DataSize = size;
The size is indeterminate here. I think that it should uses datasize?
var->DataSize = datasize;
> + if (ret)
> return -EIO;
>
> if (in_compat_syscall()) {
Regards
Joey Lee
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-05 6:01 ` joeyli
@ 2020-03-05 6:17 ` Vladis Dronov
0 siblings, 0 replies; 23+ messages in thread
From: Vladis Dronov @ 2020-03-05 6:17 UTC (permalink / raw)
To: joeyli; +Cc: Ard Biesheuvel, linux-efi, linux-kernel
Hello, Joey, all,
> > - var->DataSize = 1024;
> > - if (efivar_entry_get(entry, &entry->var.Attributes,
> > - &entry->var.DataSize, entry->var.Data))
> > + ret = efivar_entry_get(entry, &var->Attributes, &datasize, var->Data);
> > + var->DataSize = size;
>
> The size is indeterminate here. I think that it should uses datasize?
> var->DataSize = datasize;
Indeed, my mistake. Thank you much! I will fix it in the v3 patchset I'm
currently composing.
Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 0/3] efi: fix a race and add a sanity check
2020-03-03 9:15 ` Ard Biesheuvel
` (2 preceding siblings ...)
2020-03-04 15:49 ` [PATCH v2] " Vladis Dronov
@ 2020-03-05 8:40 ` Vladis Dronov
2020-03-05 8:40 ` [PATCH v3 1/3] efi: fix a race and a buffer overflow while reading efivars via sysfs Vladis Dronov
` (3 more replies)
3 siblings, 4 replies; 23+ messages in thread
From: Vladis Dronov @ 2020-03-05 8:40 UTC (permalink / raw)
To: Ard Biesheuvel, linux-efi, joeyli; +Cc: linux-kernel
There is a race and a buffer overflow while reading an efi variable
and the first patch fixes it. The second patch adds a sanity check
to efivar_store_raw(). And the third one just fixes mistypes in
comments.
Vladis Dronov (3):
efi: fix a race and a buffer overflow while reading efivars via sysfs
efi: add a sanity check to efivar_store_raw()
efi: fix a mistype in comments mentioning efivar_entry_iter_begin()
drivers/firmware/efi/efi-pstore.c | 2 +-
drivers/firmware/efi/efivars.c | 32 +++++++++++++++++++++++---------
drivers/firmware/efi/vars.c | 2 +-
3 files changed, 25 insertions(+), 11 deletions(-)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 1/3] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-05 8:40 ` [PATCH v3 0/3] efi: fix a race and add a sanity check Vladis Dronov
@ 2020-03-05 8:40 ` Vladis Dronov
2020-03-05 8:45 ` Ard Biesheuvel
2020-03-05 8:40 ` [PATCH v3 2/3] efi: add a sanity check to efivar_store_raw() Vladis Dronov
` (2 subsequent siblings)
3 siblings, 1 reply; 23+ messages in thread
From: Vladis Dronov @ 2020-03-05 8:40 UTC (permalink / raw)
To: Ard Biesheuvel, linux-efi, joeyli; +Cc: linux-kernel
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 using a local variable for a var's data buffer size so it
does not get overwritten.
Reported-by: Bob Sanders <bob.sanders@hpe.com> and the LTP testsuite
Link: https://lore.kernel.org/linux-efi/20200303085528.27658-1-vdronov@redhat.com/T/#u
Signed-off-by: Vladis Dronov <vdronov@redhat.com>
---
drivers/firmware/efi/efivars.c | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index 7576450c8254..69f13bc4b931 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -83,13 +83,16 @@ static ssize_t
efivar_attr_read(struct efivar_entry *entry, char *buf)
{
struct efi_variable *var = &entry->var;
+ unsigned long size = sizeof(var->Data);
char *str = buf;
+ int ret;
if (!entry || !buf)
return -EINVAL;
- var->DataSize = 1024;
- if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
+ ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
+ var->DataSize = size;
+ if (ret)
return -EIO;
if (var->Attributes & EFI_VARIABLE_NON_VOLATILE)
@@ -116,13 +119,16 @@ static ssize_t
efivar_size_read(struct efivar_entry *entry, char *buf)
{
struct efi_variable *var = &entry->var;
+ unsigned long size = sizeof(var->Data);
char *str = buf;
+ int ret;
if (!entry || !buf)
return -EINVAL;
- var->DataSize = 1024;
- if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
+ ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
+ var->DataSize = size;
+ if (ret)
return -EIO;
str += sprintf(str, "0x%lx\n", var->DataSize);
@@ -133,12 +139,15 @@ static ssize_t
efivar_data_read(struct efivar_entry *entry, char *buf)
{
struct efi_variable *var = &entry->var;
+ unsigned long size = sizeof(var->Data);
+ int ret;
if (!entry || !buf)
return -EINVAL;
- var->DataSize = 1024;
- if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
+ ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
+ var->DataSize = size;
+ if (ret)
return -EIO;
memcpy(buf, var->Data, var->DataSize);
@@ -250,14 +259,16 @@ efivar_show_raw(struct efivar_entry *entry, char *buf)
{
struct efi_variable *var = &entry->var;
struct compat_efi_variable *compat;
+ unsigned long datasize = sizeof(var->Data);
size_t size;
+ int ret;
if (!entry || !buf)
return 0;
- var->DataSize = 1024;
- if (efivar_entry_get(entry, &entry->var.Attributes,
- &entry->var.DataSize, entry->var.Data))
+ ret = efivar_entry_get(entry, &var->Attributes, &datasize, var->Data);
+ var->DataSize = datasize;
+ if (ret)
return -EIO;
if (in_compat_syscall()) {
--
2.20.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 2/3] efi: add a sanity check to efivar_store_raw()
2020-03-05 8:40 ` [PATCH v3 0/3] efi: fix a race and add a sanity check Vladis Dronov
2020-03-05 8:40 ` [PATCH v3 1/3] efi: fix a race and a buffer overflow while reading efivars via sysfs Vladis Dronov
@ 2020-03-05 8:40 ` Vladis Dronov
2020-03-05 8:40 ` [PATCH v3 3/3] efi: fix a mistype in comments mentioning efivar_entry_iter_begin() Vladis Dronov
2020-03-05 8:51 ` [PATCH v3 0/3] efi: fix a race and add a sanity check Ard Biesheuvel
3 siblings, 0 replies; 23+ messages in thread
From: Vladis Dronov @ 2020-03-05 8:40 UTC (permalink / raw)
To: Ard Biesheuvel, linux-efi, joeyli; +Cc: linux-kernel
Add a sanity check to efivar_store_raw() the same way
efivar_{attr,size,data}_read() and efivar_show_raw() have it.
Signed-off-by: Vladis Dronov <vdronov@redhat.com>
---
drivers/firmware/efi/efivars.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index 69f13bc4b931..aff3dfb4d7ba 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -208,6 +208,9 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
u8 *data;
int err;
+ if (!entry || !buf)
+ return -EINVAL;
+
if (in_compat_syscall()) {
struct compat_efi_variable *compat;
--
2.20.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 3/3] efi: fix a mistype in comments mentioning efivar_entry_iter_begin()
2020-03-05 8:40 ` [PATCH v3 0/3] efi: fix a race and add a sanity check Vladis Dronov
2020-03-05 8:40 ` [PATCH v3 1/3] efi: fix a race and a buffer overflow while reading efivars via sysfs Vladis Dronov
2020-03-05 8:40 ` [PATCH v3 2/3] efi: add a sanity check to efivar_store_raw() Vladis Dronov
@ 2020-03-05 8:40 ` Vladis Dronov
2020-03-05 8:51 ` [PATCH v3 0/3] efi: fix a race and add a sanity check Ard Biesheuvel
3 siblings, 0 replies; 23+ messages in thread
From: Vladis Dronov @ 2020-03-05 8:40 UTC (permalink / raw)
To: Ard Biesheuvel, linux-efi, joeyli; +Cc: linux-kernel
Signed-off-by: Vladis Dronov <vdronov@redhat.com>
---
drivers/firmware/efi/efi-pstore.c | 2 +-
drivers/firmware/efi/vars.c | 2 +-
2 files changed, 2 insertions(+), 2 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/vars.c b/drivers/firmware/efi/vars.c
index 436d1776bc7b..5f2a4d162795 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -1071,7 +1071,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
--
2.20.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/3] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-05 8:40 ` [PATCH v3 1/3] efi: fix a race and a buffer overflow while reading efivars via sysfs Vladis Dronov
@ 2020-03-05 8:45 ` Ard Biesheuvel
2020-03-05 10:52 ` Vladis Dronov
0 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-03-05 8:45 UTC (permalink / raw)
To: Vladis Dronov; +Cc: linux-efi, joeyli, Linux Kernel Mailing List
On Thu, 5 Mar 2020 at 09:41, Vladis Dronov <vdronov@redhat.com> 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 using a local variable for a var's data buffer size so it
> does not get overwritten.
>
> Reported-by: Bob Sanders <bob.sanders@hpe.com> and the LTP testsuite
> Link: https://lore.kernel.org/linux-efi/20200303085528.27658-1-vdronov@redhat.com/T/#u
For the future, please don't add these links. This one points to the
old version of the patch, not to this one. It will be added by the
tooling once the patch gets picked up.
> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> ---
> drivers/firmware/efi/efivars.c | 29 ++++++++++++++++++++---------
> 1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
> index 7576450c8254..69f13bc4b931 100644
> --- a/drivers/firmware/efi/efivars.c
> +++ b/drivers/firmware/efi/efivars.c
> @@ -83,13 +83,16 @@ static ssize_t
> efivar_attr_read(struct efivar_entry *entry, char *buf)
> {
> struct efi_variable *var = &entry->var;
> + unsigned long size = sizeof(var->Data);
> char *str = buf;
> + int ret;
>
> if (!entry || !buf)
> return -EINVAL;
>
> - var->DataSize = 1024;
> - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
> + ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> + var->DataSize = size;
> + if (ret)
> return -EIO;
>
> if (var->Attributes & EFI_VARIABLE_NON_VOLATILE)
> @@ -116,13 +119,16 @@ static ssize_t
> efivar_size_read(struct efivar_entry *entry, char *buf)
> {
> struct efi_variable *var = &entry->var;
> + unsigned long size = sizeof(var->Data);
> char *str = buf;
> + int ret;
>
> if (!entry || !buf)
> return -EINVAL;
>
> - var->DataSize = 1024;
> - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
> + ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> + var->DataSize = size;
> + if (ret)
> return -EIO;
>
> str += sprintf(str, "0x%lx\n", var->DataSize);
> @@ -133,12 +139,15 @@ static ssize_t
> efivar_data_read(struct efivar_entry *entry, char *buf)
> {
> struct efi_variable *var = &entry->var;
> + unsigned long size = sizeof(var->Data);
> + int ret;
>
> if (!entry || !buf)
> return -EINVAL;
>
> - var->DataSize = 1024;
> - if (efivar_entry_get(entry, &var->Attributes, &var->DataSize, var->Data))
> + ret = efivar_entry_get(entry, &var->Attributes, &size, var->Data);
> + var->DataSize = size;
> + if (ret)
> return -EIO;
>
> memcpy(buf, var->Data, var->DataSize);
> @@ -250,14 +259,16 @@ efivar_show_raw(struct efivar_entry *entry, char *buf)
> {
> struct efi_variable *var = &entry->var;
> struct compat_efi_variable *compat;
> + unsigned long datasize = sizeof(var->Data);
> size_t size;
> + int ret;
>
> if (!entry || !buf)
> return 0;
>
> - var->DataSize = 1024;
> - if (efivar_entry_get(entry, &entry->var.Attributes,
> - &entry->var.DataSize, entry->var.Data))
> + ret = efivar_entry_get(entry, &var->Attributes, &datasize, var->Data);
> + var->DataSize = datasize;
> + if (ret)
> return -EIO;
>
> if (in_compat_syscall()) {
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 0/3] efi: fix a race and add a sanity check
2020-03-05 8:40 ` [PATCH v3 0/3] efi: fix a race and add a sanity check Vladis Dronov
` (2 preceding siblings ...)
2020-03-05 8:40 ` [PATCH v3 3/3] efi: fix a mistype in comments mentioning efivar_entry_iter_begin() Vladis Dronov
@ 2020-03-05 8:51 ` Ard Biesheuvel
3 siblings, 0 replies; 23+ messages in thread
From: Ard Biesheuvel @ 2020-03-05 8:51 UTC (permalink / raw)
To: Vladis Dronov; +Cc: linux-efi, joeyli, Linux Kernel Mailing List
On Thu, 5 Mar 2020 at 09:41, Vladis Dronov <vdronov@redhat.com> wrote:
>
> There is a race and a buffer overflow while reading an efi variable
> and the first patch fixes it. The second patch adds a sanity check
> to efivar_store_raw(). And the third one just fixes mistypes in
> comments.
>
> Vladis Dronov (3):
> efi: fix a race and a buffer overflow while reading efivars via sysfs
> efi: add a sanity check to efivar_store_raw()
> efi: fix a mistype in comments mentioning efivar_entry_iter_begin()
>
Queued in efi/next
Thanks!
> drivers/firmware/efi/efi-pstore.c | 2 +-
> drivers/firmware/efi/efivars.c | 32 +++++++++++++++++++++++---------
> drivers/firmware/efi/vars.c | 2 +-
> 3 files changed, 25 insertions(+), 11 deletions(-)
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/3] efi: fix a race and a buffer overflow while reading efivars via sysfs
2020-03-05 8:45 ` Ard Biesheuvel
@ 2020-03-05 10:52 ` Vladis Dronov
0 siblings, 0 replies; 23+ messages in thread
From: Vladis Dronov @ 2020-03-05 10:52 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi, joeyli, Linux Kernel Mailing List
Hello, Ard!
> > Reported-by: Bob Sanders <bob.sanders@hpe.com> and the LTP testsuite
> > Link:
> > https://lore.kernel.org/linux-efi/20200303085528.27658-1-vdronov@redhat.com/T/#u
>
> For the future, please don't add these links. This one points to the
> old version of the patch, not to this one. It will be added by the
> tooling once the patch gets picked up.
Aha. I was under an impression Links: are added manually by authors. My intention
here was to point at the root message of the whole discussion, not at the patch.
Anyway, thank you for the review and for bearing with me!
Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2020-03-05 10:52 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03 8:55 [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs Vladis Dronov
2020-03-03 9:15 ` Ard Biesheuvel
2020-03-03 10:14 ` Vladis Dronov
2020-03-03 10:20 ` Ard Biesheuvel
2020-03-03 10:24 ` Vladis Dronov
2020-03-04 6:51 ` joeyli
2020-03-04 15:45 ` Vladis Dronov
2020-03-04 15:47 ` Ard Biesheuvel
2020-03-04 15:49 ` [PATCH v2] " Vladis Dronov
2020-03-04 15:57 ` Ard Biesheuvel
2020-03-04 17:18 ` Vladis Dronov
2020-03-04 17:21 ` Ard Biesheuvel
2020-03-05 6:01 ` joeyli
2020-03-05 6:17 ` Vladis Dronov
2020-03-05 8:40 ` [PATCH v3 0/3] efi: fix a race and add a sanity check Vladis Dronov
2020-03-05 8:40 ` [PATCH v3 1/3] efi: fix a race and a buffer overflow while reading efivars via sysfs Vladis Dronov
2020-03-05 8:45 ` Ard Biesheuvel
2020-03-05 10:52 ` Vladis Dronov
2020-03-05 8:40 ` [PATCH v3 2/3] efi: add a sanity check to efivar_store_raw() Vladis Dronov
2020-03-05 8:40 ` [PATCH v3 3/3] efi: fix a mistype in comments mentioning efivar_entry_iter_begin() Vladis Dronov
2020-03-05 8:51 ` [PATCH v3 0/3] efi: fix a race and add a sanity check Ard Biesheuvel
2020-03-04 14:07 ` [PATCH] efi: fix a race and a buffer overflow while reading efivars via sysfs joeyli
2020-03-04 16:06 ` Vladis Dronov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).