Patchwork [08/11] firmware: efi: add NULL pointer checks in efivars api functions

login
register
mail settings
Submitter Ard Biesheuvel
Date Nov. 29, 2018, 5:12 p.m.
Message ID <20181129171230.18699-9-ard.biesheuvel@linaro.org>
Download mbox | patch
Permalink /patch/668363/
State New
Headers show

Comments

Ard Biesheuvel - Nov. 29, 2018, 5:12 p.m.
From: Arend van Spriel <arend.vanspriel@broadcom.com>

Since commit:

   ce2e6db554fa ("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.

Cc: Hans de Goede <hdegoede@redhat.com>
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>
---
 drivers/firmware/efi/vars.c | 99 +++++++++++++++++++++++++++++--------
 1 file changed, 78 insertions(+), 21 deletions(-)
Ingo Molnar - Nov. 30, 2018, 8:11 a.m.
* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> From: Arend van Spriel <arend.vanspriel@broadcom.com>
> 
> Since commit:
> 
>    ce2e6db554fa ("brcmfmac: Add support for getting nvram contents from
>                  EFI variables")

This commit ID is not upstream AFAICS. Which tree is it from? Mentioning 
non-upstream sha1's is discouraged in changelogs, as there's no guarantee 
that the sha1 will make it upstream.

> 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.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> 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>

Will that new commit be backported? If yes I suppose we could mark this 
fix -stable too? If not then it's fine for a v4.21 merge.

Thanks,

	Ingo
Ard Biesheuvel - Nov. 30, 2018, 8:37 a.m.
On Fri, 30 Nov 2018 at 09:12, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> > From: Arend van Spriel <arend.vanspriel@broadcom.com>
> >
> > Since commit:
> >
> >    ce2e6db554fa ("brcmfmac: Add support for getting nvram contents from
> >                  EFI variables")
>
> This commit ID is not upstream AFAICS. Which tree is it from? Mentioning
> non-upstream sha1's is discouraged in changelogs, as there's no guarantee
> that the sha1 will make it upstream.
>

This is a commit ID from Arend's own tree which is pulled into -next,
so I assumed that he'd only include commit IDs like this if they are
stable.

In any case, the fix itself is rather obvious, so much of the context
provided by the commit log could be summarized as '__efivars may be
NULL so check for that before you dereference it'

> > 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.
> >
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > 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>
>
> Will that new commit be backported? If yes I suppose we could mark this
> fix -stable too? If not then it's fine for a v4.21 merge.
>

That commit is not -stable material at all, as far as I can tell.

Patch

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 9336ffdf6e2c..fceaafd67ec6 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -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;
@@ -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;
@@ -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");
@@ -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);
 }
@@ -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);
@@ -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;
@@ -687,12 +715,17 @@  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) {
@@ -700,6 +733,7 @@  efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
 		return -ENOSPC;
 	}
 
+	ops = __efivars->ops;
 	status = ops->set_variable_nonblocking(name, &vendor, attributes,
 					       size, data);
 
@@ -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;
 
@@ -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);
@@ -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);
 }
@@ -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);
@@ -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;
@@ -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
 	 */
@@ -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);