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@xxxxxxx> and the LTP testsuite
> Signed-off-by: Vladis Dronov <vdronov@xxxxxxxxxx>
> ---
> 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
