[v3,1/3] efi: fix a race and a buffer overflow while reading efivars via sysfs
diff mbox series

Message ID 20200305084041.24053-2-vdronov@redhat.com
State In Next
Commit 286d3250c9d6437340203fb64938bea344729a0e
Headers show
Series
  • efi: fix a race and add a sanity check
Related show

Commit Message

Vladis Dronov March 5, 2020, 8:40 a.m. UTC
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(-)

Comments

Ard Biesheuvel March 5, 2020, 8:45 a.m. UTC | #1
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
>
Vladis Dronov March 5, 2020, 10:52 a.m. UTC | #2
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

Patch
diff mbox series

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()) {