There is a race and a buffer overflow while reading an efi variable and the first patch fixes it. The second patch adds a sanity check to efivar_store_raw(). The original patchset applies to the 5.x trees fine.
Vladis Dronov (2): efi: fix a race and a buffer overflow while reading efivars via sysfs efi: add a sanity check to efivar_store_raw()
drivers/firmware/efi/efivars.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
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()) {
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?
thanks,
greg k-h
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
commit d6c066fda90d578aacdf19771a027ed484a79825 upstream.
Add a sanity check to efivar_store_raw() the same way efivar_{attr,size,data}_read() and efivar_show_raw() have it.
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-3-vdronov@redhat.com Link: https://lore.kernel.org/r/20200308080859.21568-25-ardb@kernel.org --- drivers/firmware/efi/efivars.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c index c8688490f148..1c65f5ac4368 100644 --- a/drivers/firmware/efi/efivars.c +++ b/drivers/firmware/efi/efivars.c @@ -272,6 +272,9 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count) u8 *data; int err;
+ if (!entry || !buf) + return -EINVAL; + if (is_compat()) { struct compat_efi_variable *compat;
On Mon, Mar 16, 2020 at 02:19:38PM +0100, Vladis Dronov wrote:
commit d6c066fda90d578aacdf19771a027ed484a79825 upstream.
Add a sanity check to efivar_store_raw() the same way efivar_{attr,size,data}_read() and efivar_show_raw() have it.
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-3-vdronov@redhat.com Link: https://lore.kernel.org/r/20200308080859.21568-25-ardb@kernel.org
drivers/firmware/efi/efivars.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c index c8688490f148..1c65f5ac4368 100644 --- a/drivers/firmware/efi/efivars.c +++ b/drivers/firmware/efi/efivars.c @@ -272,6 +272,9 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count) u8 *data; int err;
- if (!entry || !buf)
return -EINVAL;- if (is_compat()) { struct compat_efi_variable *compat;
2.20.1
Now queued up, thanks.
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Greg KH -
Vladis Dronov