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

Hello,

----- Original Message -----

From: "Greg KH" greg@kroah.com To: "Vladis Dronov" vdronov@redhat.com Cc: stable@vger.kernel.org, "Sasha Levin" sashal@kernel.org 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@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

---

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