Hello,
----- Original Message -----
> From: "Greg KH" <greg@xxxxxxxxx>
> To: "Vladis Dronov" <vdronov@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx, "Sasha Levin" <sashal@xxxxxxxxxx>
> Sent: Monday, March 16, 2020 2:27:18 PM
> Subject: Re: [PATCH 4.19, 4.14, 4.9, 4.4 1/2] efi: Fix a race and a buffer overflow while reading efivars via sysfs
>
> On Mon, Mar 16, 2020 at 02:19:37PM +0100, Vladis Dronov wrote:
> > 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@xxxxxxx> and the LTP testsuite
> > Signed-off-by: Vladis Dronov <vdronov@xxxxxxxxxx>
> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Link: https://lore.kernel.org/r/20200305084041.24053-2-vdronov@xxxxxxxxxx
> > Link: https://lore.kernel.org/r/20200308080859.21568-24-ardb@xxxxxxxxxx
> > ---
> > 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 3e626fd9bd4e..c8688490f148 100644
> > --- a/drivers/firmware/efi/efivars.c
> > +++ b/drivers/firmware/efi/efivars.c
> > @@ -139,13 +139,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)
> > @@ -172,13 +175,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);
> > @@ -189,12 +195,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);
> > @@ -314,14 +323,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 (is_compat()) {
> > --
> > 2.20.1
> >
>
> This is already in all of my stable trees, did it need to be somehow
> backported differently to 4.19 and older?
It looks like I've misunderstood "... failed to apply to 4.XX-stable tree" messages.
This exact patch does not need any special backporting (if it applies fine to the
tree). Apologies for the spam and traffic.
> thanks,
>
> greg k-h
Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
