* [PATCH 4.19, 4.14, 4.9, 4.4 0/2] efi: fix a race and add a sanity check @ 2020-03-16 13:19 Vladis Dronov 2020-03-16 13:19 ` [PATCH 4.19, 4.14, 4.9, 4.4 1/2] efi: Fix a race and a buffer overflow while reading efivars via sysfs Vladis Dronov 2020-03-16 13:19 ` [PATCH 4.19, 4.14, 4.9, 4.4 2/2] efi: Add a sanity check to efivar_store_raw() Vladis Dronov 0 siblings, 2 replies; 6+ messages in thread From: Vladis Dronov @ 2020-03-16 13:19 UTC (permalink / raw) To: stable; +Cc: Sasha Levin 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(-) ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 4.19, 4.14, 4.9, 4.4 1/2] efi: Fix a race and a buffer overflow while reading efivars via sysfs 2020-03-16 13:19 [PATCH 4.19, 4.14, 4.9, 4.4 0/2] efi: fix a race and add a sanity check Vladis Dronov @ 2020-03-16 13:19 ` Vladis Dronov 2020-03-16 13:27 ` Greg KH 2020-03-16 13:19 ` [PATCH 4.19, 4.14, 4.9, 4.4 2/2] efi: Add a sanity check to efivar_store_raw() Vladis Dronov 1 sibling, 1 reply; 6+ messages in thread From: Vladis Dronov @ 2020-03-16 13:19 UTC (permalink / raw) To: stable; +Cc: Sasha Levin 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* 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 2020-03-16 13:19 ` [PATCH 4.19, 4.14, 4.9, 4.4 1/2] efi: Fix a race and a buffer overflow while reading efivars via sysfs Vladis Dronov @ 2020-03-16 13:27 ` Greg KH 2020-03-16 14:32 ` Vladis Dronov 0 siblings, 1 reply; 6+ messages in thread From: Greg KH @ 2020-03-16 13:27 UTC (permalink / raw) To: Vladis Dronov; +Cc: stable, Sasha Levin 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* 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 2020-03-16 13:27 ` Greg KH @ 2020-03-16 14:32 ` Vladis Dronov 0 siblings, 0 replies; 6+ messages in thread From: Vladis Dronov @ 2020-03-16 14:32 UTC (permalink / raw) To: Greg KH; +Cc: stable, Sasha Levin 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 4.19, 4.14, 4.9, 4.4 2/2] efi: Add a sanity check to efivar_store_raw() 2020-03-16 13:19 [PATCH 4.19, 4.14, 4.9, 4.4 0/2] efi: fix a race and add a sanity check Vladis Dronov 2020-03-16 13:19 ` [PATCH 4.19, 4.14, 4.9, 4.4 1/2] efi: Fix a race and a buffer overflow while reading efivars via sysfs Vladis Dronov @ 2020-03-16 13:19 ` Vladis Dronov 2020-03-16 15:50 ` Greg KH 1 sibling, 1 reply; 6+ messages in thread From: Vladis Dronov @ 2020-03-16 13:19 UTC (permalink / raw) To: stable; +Cc: Sasha Levin 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 4.19, 4.14, 4.9, 4.4 2/2] efi: Add a sanity check to efivar_store_raw() 2020-03-16 13:19 ` [PATCH 4.19, 4.14, 4.9, 4.4 2/2] efi: Add a sanity check to efivar_store_raw() Vladis Dronov @ 2020-03-16 15:50 ` Greg KH 0 siblings, 0 replies; 6+ messages in thread From: Greg KH @ 2020-03-16 15:50 UTC (permalink / raw) To: Vladis Dronov; +Cc: stable, Sasha Levin 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-16 15:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-16 13:19 [PATCH 4.19, 4.14, 4.9, 4.4 0/2] efi: fix a race and add a sanity check Vladis Dronov 2020-03-16 13:19 ` [PATCH 4.19, 4.14, 4.9, 4.4 1/2] efi: Fix a race and a buffer overflow while reading efivars via sysfs Vladis Dronov 2020-03-16 13:27 ` Greg KH 2020-03-16 14:32 ` Vladis Dronov 2020-03-16 13:19 ` [PATCH 4.19, 4.14, 4.9, 4.4 2/2] efi: Add a sanity check to efivar_store_raw() Vladis Dronov 2020-03-16 15:50 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).