Subject: Re: [PATCH v3 1/3] efi: fix a race and a buffer overflow while reading efivars via sysfs

On Thu, 5 Mar 2020 at 09:41, Vladis Dronov <[email protected]> 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 <[email protected]> and the LTP testsuite
> Link: https://lore.kernel.org/linux-efi/[email protected]/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 <[email protected]>
> ---
> 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
>

2020-03-05 08:53:29

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [PATCH v3 0/3] efi: fix a race and add a sanity check

On Thu, 5 Mar 2020 at 09:41, Vladis Dronov <[email protected]> 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(-)
>

2020-03-05 10:53:45

by Vladis Dronov

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] efi: fix a race and a buffer overflow while reading efivars via sysfs

Hello, Ard!

> > Reported-by: Bob Sanders <[email protected]> and the LTP testsuite
> > Link:
> > https://lore.kernel.org/linux-efi/[email protected]/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

2020-03-08 16:39:42

by tip-bot2 for Eric Biggers

[permalink] [raw]

Subject: [tip: efi/urgent] efi: Fix a race and a buffer overflow while reading efivars via sysfs

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID: 286d3250c9d6437340203fb64938bea344729a0e
Gitweb: https://git.kernel.org/tip/286d3250c9d6437340203fb64938bea344729a0e
Author: Vladis Dronov <[email protected]>
AuthorDate: Sun, 08 Mar 2020 09:08:54 +01:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Sun, 08 Mar 2020 09:56:34 +01:00

efi: Fix a race and a buffer overflow while reading efivars via sysfs

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.

Fixes: e14ab23dde12b80d ("efivars: efivar_entry API")
Reported-by: Bob Sanders <[email protected]> and the LTP testsuite
Signed-off-by: Vladis Dronov <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Cc: <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Link: https://lore.kernel.org/r/[email protected]
---
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 7576450..69f13bc 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()) {

2020-03-08 16:40:04

by tip-bot2 for Eric Biggers

[permalink] [raw]

Subject: [tip: efi/urgent] efi: Add a sanity check to efivar_store_raw()

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID: d6c066fda90d578aacdf19771a027ed484a79825
Gitweb: https://git.kernel.org/tip/d6c066fda90d578aacdf19771a027ed484a79825
Author: Vladis Dronov <[email protected]>
AuthorDate: Sun, 08 Mar 2020 09:08:55 +01:00
Committer: Ingo Molnar <[email protected]>
CommitterDate: Sun, 08 Mar 2020 09:56:48 +01:00

efi: Add a sanity check to efivar_store_raw()

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 <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Cc: <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Link: https://lore.kernel.org/r/[email protected]
---
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 69f13bc..aff3dfb 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;