| From b34baa86fef266a5dd8a8b722e9fcb6bf495039e Mon Sep 17 00:00:00 2001 |
| From: Vladis Dronov <vdronov@redhat.com> |
| Date: Sun, 8 Mar 2020 09:08:54 +0100 |
| Subject: [PATCH] efi: Fix a race and a buffer overflow while reading efivars |
| via sysfs |
| |
| commit 286d3250c9d6437340203fb64938bea344729a0e upstream. |
| |
| 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 <bob.sanders@hpe.com> and the LTP testsuite |
| Signed-off-by: Vladis Dronov <vdronov@redhat.com> |
| Signed-off-by: Ard Biesheuvel <ardb@kernel.org> |
| Signed-off-by: Ingo Molnar <mingo@kernel.org> |
| Cc: <stable@vger.kernel.org> |
| Link: https://lore.kernel.org/r/20200305084041.24053-2-vdronov@redhat.com |
| Link: https://lore.kernel.org/r/20200308080859.21568-24-ardb@kernel.org |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| 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.7.4 |
| |