Skip to content

Commit

Permalink
firmware/efi: Add NULL pointer checks in efivars API functions
Browse files Browse the repository at this point in the history 
[ Upstream commit ab2180a ]

Since commit:

   ce2e6db ("brcmfmac: Add support for getting nvram contents from EFI variables")

we have a device driver accessing the efivars API. Several functions in
the efivars API assume __efivars is set, i.e., that they will be accessed
only after efivars_register() has been called. However, the following NULL
pointer access was reported calling efivar_entry_size() from the brcmfmac
device driver:

  Unable to handle kernel NULL pointer dereference at virtual address 00000008
  pgd = 60bfa5f1
  [00000008] *pgd=00000000
  Internal error: Oops: 5 [#1] SMP ARM
  ...
  Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
  Workqueue: events request_firmware_work_func
  PC is at efivar_entry_size+0x28/0x90
  LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
  pc : [<c0c40718>]    lr : [<bf2a3ef4>]    psr: a00d0113
  sp : ede7fe28  ip : ee983410  fp : c1787f30
  r10: 00000000  r9 : 00000000  r8 : bf2b2258
  r7 : ee983000  r6 : c1604c48  r5 : ede7fe88  r4 : edf337c0
  r3 : 00000000  r2 : 00000000  r1 : ede7fe88  r0 : c17712c8
  Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
  Control: 10c5387d  Table: ad16804a  DAC: 00000051

Disassembly showed that the local static variable __efivars is NULL,
which is not entirely unexpected given that it is a non-EFI platform.

So add a NULL pointer check to efivar_entry_size(), and to related
functions while at it. In efivars_register() a couple of sanity checks
are added as well.

Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Eric Snowberg <eric.snowberg@oracle.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Joe Perches <joe@perches.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Nathan Chancellor <natechancellor@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Sedat Dilek <sedat.dilek@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: YiFei Zhu <zhuyifei1999@gmail.com>
Cc: linux-efi@vger.kernel.org
Link: http://lkml.kernel.org/r/20181129171230.18699-9-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
  • Loading branch information
@gregkh
Arend van Spriel authored and gregkh committed Feb 12, 2019
1 parent 9c74cd4 commit eca31fc
Showing 1 changed file with 78 additions and 21 deletions.
99 changes: 78 additions & 21 deletions drivers/firmware/efi/vars.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,12 @@ EXPORT_SYMBOL_GPL(efivar_variable_is_removable);
static efi_status_t
check_var_size(u32 attributes, unsigned long size)
{
const struct efivar_operations *fops = __efivars->ops;
const struct efivar_operations *fops;

if (!__efivars)
return EFI_UNSUPPORTED;

fops = __efivars->ops;

if (!fops->query_variable_store)
return EFI_UNSUPPORTED;
Expand All @@ -329,7 +334,12 @@ check_var_size(u32 attributes, unsigned long size)
static efi_status_t
check_var_size_nonblocking(u32 attributes, unsigned long size)
{
const struct efivar_operations *fops = __efivars->ops;
const struct efivar_operations *fops;

if (!__efivars)
return EFI_UNSUPPORTED;

fops = __efivars->ops;

if (!fops->query_variable_store)
return EFI_UNSUPPORTED;
Expand Down Expand Up @@ -429,13 +439,18 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
void *data, bool duplicates, struct list_head *head)
{
const struct efivar_operations *ops = __efivars->ops;
const struct efivar_operations *ops;
unsigned long variable_name_size = 1024;
efi_char16_t *variable_name;
efi_status_t status;
efi_guid_t vendor_guid;
int err = 0;

if (!__efivars)
return -EFAULT;

ops = __efivars->ops;

variable_name = kzalloc(variable_name_size, GFP_KERNEL);
if (!variable_name) {
printk(KERN_ERR "efivars: Memory allocation failed.\n");
Expand Down Expand Up @@ -583,12 +598,14 @@ static void efivar_entry_list_del_unlock(struct efivar_entry *entry)
*/
int __efivar_entry_delete(struct efivar_entry *entry)
{
const struct efivar_operations *ops = __efivars->ops;
efi_status_t status;

status = ops->set_variable(entry->var.VariableName,
&entry->var.VendorGuid,
0, 0, NULL);
if (!__efivars)
return -EINVAL;

status = __efivars->ops->set_variable(entry->var.VariableName,
&entry->var.VendorGuid,
0, 0, NULL);

return efi_status_to_err(status);
}
Expand All @@ -607,12 +624,17 @@ EXPORT_SYMBOL_GPL(__efivar_entry_delete);
*/
int efivar_entry_delete(struct efivar_entry *entry)
{
const struct efivar_operations *ops = __efivars->ops;
const struct efivar_operations *ops;
efi_status_t status;

if (down_interruptible(&efivars_lock))
return -EINTR;

if (!__efivars) {
up(&efivars_lock);
return -EINVAL;
}
ops = __efivars->ops;
status = ops->set_variable(entry->var.VariableName,
&entry->var.VendorGuid,
0, 0, NULL);
Expand Down Expand Up @@ -650,13 +672,19 @@ EXPORT_SYMBOL_GPL(efivar_entry_delete);
int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
unsigned long size, void *data, struct list_head *head)
{
const struct efivar_operations *ops = __efivars->ops;
const struct efivar_operations *ops;
efi_status_t status;
efi_char16_t *name = entry->var.VariableName;
efi_guid_t vendor = entry->var.VendorGuid;

if (down_interruptible(&efivars_lock))
return -EINTR;

if (!__efivars) {
up(&efivars_lock);
return -EINVAL;
}
ops = __efivars->ops;
if (head && efivar_entry_find(name, vendor, head, false)) {
up(&efivars_lock);
return -EEXIST;
Expand Down Expand Up @@ -687,19 +715,25 @@ static int
efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
u32 attributes, unsigned long size, void *data)
{
const struct efivar_operations *ops = __efivars->ops;
const struct efivar_operations *ops;
efi_status_t status;

if (down_trylock(&efivars_lock))
return -EBUSY;

if (!__efivars) {
up(&efivars_lock);
return -EINVAL;
}

status = check_var_size_nonblocking(attributes,
size + ucs2_strsize(name, 1024));
if (status != EFI_SUCCESS) {
up(&efivars_lock);
return -ENOSPC;
}

ops = __efivars->ops;
status = ops->set_variable_nonblocking(name, &vendor, attributes,
size, data);

Expand Down Expand Up @@ -727,9 +761,13 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
bool block, unsigned long size, void *data)
{
const struct efivar_operations *ops = __efivars->ops;
const struct efivar_operations *ops;
efi_status_t status;

if (!__efivars)
return -EINVAL;

ops = __efivars->ops;
if (!ops->query_variable_store)
return -ENOSYS;

Expand Down Expand Up @@ -829,13 +867,18 @@ EXPORT_SYMBOL_GPL(efivar_entry_find);
*/
int efivar_entry_size(struct efivar_entry *entry, unsigned long *size)
{
const struct efivar_operations *ops = __efivars->ops;
const struct efivar_operations *ops;
efi_status_t status;

*size = 0;

if (down_interruptible(&efivars_lock))
return -EINTR;
if (!__efivars) {
up(&efivars_lock);
return -EINVAL;
}
ops = __efivars->ops;
status = ops->get_variable(entry->var.VariableName,
&entry->var.VendorGuid, NULL, size, NULL);
up(&efivars_lock);
Expand All @@ -861,12 +904,14 @@ EXPORT_SYMBOL_GPL(efivar_entry_size);
int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
unsigned long *size, void *data)
{
const struct efivar_operations *ops = __efivars->ops;
efi_status_t status;

status = ops->get_variable(entry->var.VariableName,
&entry->var.VendorGuid,
attributes, size, data);
if (!__efivars)
return -EINVAL;

status = __efivars->ops->get_variable(entry->var.VariableName,
&entry->var.VendorGuid,
attributes, size, data);

return efi_status_to_err(status);
}
Expand All @@ -882,14 +927,19 @@ EXPORT_SYMBOL_GPL(__efivar_entry_get);
int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
unsigned long *size, void *data)
{
const struct efivar_operations *ops = __efivars->ops;
efi_status_t status;

if (down_interruptible(&efivars_lock))
return -EINTR;
status = ops->get_variable(entry->var.VariableName,
&entry->var.VendorGuid,
attributes, size, data);

if (!__efivars) {
up(&efivars_lock);
return -EINVAL;
}

status = __efivars->ops->get_variable(entry->var.VariableName,
&entry->var.VendorGuid,
attributes, size, data);
up(&efivars_lock);

return efi_status_to_err(status);
Expand Down Expand Up @@ -921,7 +971,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_get);
int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
unsigned long *size, void *data, bool *set)
{
const struct efivar_operations *ops = __efivars->ops;
const struct efivar_operations *ops;
efi_char16_t *name = entry->var.VariableName;
efi_guid_t *vendor = &entry->var.VendorGuid;
efi_status_t status;
Expand All @@ -940,6 +990,11 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
if (down_interruptible(&efivars_lock))
return -EINTR;

if (!__efivars) {
err = -EINVAL;
goto out;
}

/*
* Ensure that the available space hasn't shrunk below the safe level
*/
Expand All @@ -956,6 +1011,8 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
}
}

ops = __efivars->ops;

status = ops->set_variable(name, vendor, attributes, *size, data);
if (status != EFI_SUCCESS) {
err = efi_status_to_err(status);
Expand Down

0 comments on commit eca31fc

Please sign in to comment.