Since commit ce2e6db554fa ("brcmfmac: Add support for getting nvram
contents from EFI variables") we have a device driver accessing the
efivars api (since next-20181107). Several functions in efivars api
assume __efivars is set, ie. will be accessed after efivars_register()
has been called. However, following NULL pointer access was reported
calling efivar_entry_size() from the brcmfmac device driver.
[ 14.177769] Unable to handle kernel NULL pointer dereference at
virtual address 00000008
[ 14.197303] pgd = 60bfa5f1
[ 14.211842] [00000008] *pgd=00000000
[ 14.227373] Internal error: Oops: 5 [#1] SMP ARM
[ 14.244244] Modules linked in: brcmfmac sha256_generic sha256_arm snd cfg80211 brcmutil soundcore snd_soc_tegra30_ahub tegra_wdt
[ 14.269109] CPU: 1 PID: 114 Comm: kworker/1:2 Not tainted 4.20.0-rc1-next-20181107-gd881de3 #1
[ 14.269114] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[ 14.269154] Workqueue: events request_firmware_work_func
[ 14.269177] PC is at efivar_entry_size+0x28/0x90
[ 14.269362] LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
[ 14.269369] pc : [<c0c40718>] lr : [<bf2a3ef4>] psr: a00d0113
[ 14.269374] sp : ede7fe28 ip : ee983410 fp : c1787f30
[ 14.269378] r10: 00000000 r9 : 00000000 r8 : bf2b2258
[ 14.269384] r7 : ee983000 r6 : c1604c48 r5 : ede7fe88 r4 : edf337c0
[ 14.269389] r3 : 00000000 r2 : 00000000 r1 : ede7fe88 r0 : c17712c8
[ 14.269398] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
[ 14.269404] Control: 10c5387d Table: ad16804a DAC: 00000051
Disassembly showed that the local static variable __efivars is NULL. Likely
because efivars_register() is not called on this Tegra platform. So adding
a NULL pointer check in efivar_entry_size() and similar functions while at
it. In efivars_register() a couple of sanity checks have been added.
Cc: Hans de Goede <[email protected]>
Reported-by: Jon Hunter <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
drivers/firmware/efi/vars.c | 108 +++++++++++++++++++++++++++++++++++---------
1 file changed, 87 insertions(+), 21 deletions(-)
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 9336ffd..5fbd8e7 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -318,7 +318,12 @@ struct variable_validate {
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 @@ struct variable_validate {
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 @@ int __efivar_entry_delete(struct efivar_entry *entry)
*/
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 @@ int efivar_entry_delete(struct efivar_entry *entry)
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 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
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 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
return -ENOSPC;
}
+ ops = __efivars->ops;
status = ops->set_variable_nonblocking(name, &vendor, attributes,
size, data);
@@ -727,9 +761,13 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
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 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
*/
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 @@ int efivar_entry_size(struct efivar_entry *entry, unsigned long *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 @@ int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
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 @@ int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
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);
@@ -1138,6 +1195,15 @@ int efivars_register(struct efivars *efivars,
if (down_interruptible(&efivars_lock))
return -EINTR;
+ if (WARN(!efivars || !ops, "Invalid parameters\n"))
+ return -EINVAL;
+
+ /* check mandatory efivars operations */
+ if (WARN_ON(!ops->get_variable) ||
+ WARN_ON(!ops->set_variable) ||
+ WARN_ON(!ops->get_next_variable))
+ return -EINVAL;
+
efivars->ops = ops;
efivars->kobject = kobject;
--
1.9.1
Hi Arend,
On Tue, 13 Nov 2018 at 12:51, Arend van Spriel
<[email protected]> wrote:
>
> Since commit ce2e6db554fa ("brcmfmac: Add support for getting nvram
> contents from EFI variables") we have a device driver accessing the
> efivars api (since next-20181107). Several functions in efivars api
> assume __efivars is set, ie. will be accessed after efivars_register()
> has been called. However, following NULL pointer access was reported
> calling efivar_entry_size() from the brcmfmac device driver.
>
> [ 14.177769] Unable to handle kernel NULL pointer dereference at
> virtual address 00000008
> [ 14.197303] pgd = 60bfa5f1
> [ 14.211842] [00000008] *pgd=00000000
> [ 14.227373] Internal error: Oops: 5 [#1] SMP ARM
> [ 14.244244] Modules linked in: brcmfmac sha256_generic sha256_arm snd cfg80211 brcmutil soundcore snd_soc_tegra30_ahub tegra_wdt
> [ 14.269109] CPU: 1 PID: 114 Comm: kworker/1:2 Not tainted 4.20.0-rc1-next-20181107-gd881de3 #1
> [ 14.269114] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> [ 14.269154] Workqueue: events request_firmware_work_func
> [ 14.269177] PC is at efivar_entry_size+0x28/0x90
> [ 14.269362] LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
> [ 14.269369] pc : [<c0c40718>] lr : [<bf2a3ef4>] psr: a00d0113
> [ 14.269374] sp : ede7fe28 ip : ee983410 fp : c1787f30
> [ 14.269378] r10: 00000000 r9 : 00000000 r8 : bf2b2258
> [ 14.269384] r7 : ee983000 r6 : c1604c48 r5 : ede7fe88 r4 : edf337c0
> [ 14.269389] r3 : 00000000 r2 : 00000000 r1 : ede7fe88 r0 : c17712c8
> [ 14.269398] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> [ 14.269404] Control: 10c5387d Table: ad16804a DAC: 00000051
>
> Disassembly showed that the local static variable __efivars is NULL. Likely
> because efivars_register() is not called on this Tegra platform. So adding
> a NULL pointer check in efivar_entry_size() and similar functions while at
> it. In efivars_register() a couple of sanity checks have been added.
>
> Cc: Hans de Goede <[email protected]>
> Reported-by: Jon Hunter <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
> ---
> drivers/firmware/efi/vars.c | 108 +++++++++++++++++++++++++++++++++++---------
> 1 file changed, 87 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 9336ffd..5fbd8e7 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -318,7 +318,12 @@ struct variable_validate {
> 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 @@ struct variable_validate {
> 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 @@ int __efivar_entry_delete(struct efivar_entry *entry)
> */
> 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;
>
Any reason in particular you put the check after the lock is taken? Is
this to ensure that __efivars does not change under your feet?
> + 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 @@ int efivar_entry_delete(struct efivar_entry *entry)
> 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 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
> 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 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
> return -ENOSPC;
> }
>
> + ops = __efivars->ops;
> status = ops->set_variable_nonblocking(name, &vendor, attributes,
> size, data);
>
> @@ -727,9 +761,13 @@ int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
> 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 @@ struct efivar_entry *efivar_entry_find(efi_char16_t *name, efi_guid_t guid,
> */
> 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 @@ int efivar_entry_size(struct efivar_entry *entry, unsigned long *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 @@ int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
> 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 @@ int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
> 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);
> @@ -1138,6 +1195,15 @@ int efivars_register(struct efivars *efivars,
> if (down_interruptible(&efivars_lock))
> return -EINTR;
>
> + if (WARN(!efivars || !ops, "Invalid parameters\n"))
> + return -EINVAL;
> +
> + /* check mandatory efivars operations */
> + if (WARN_ON(!ops->get_variable) ||
> + WARN_ON(!ops->set_variable) ||
> + WARN_ON(!ops->get_next_variable))
> + return -EINVAL;
> +
> efivars->ops = ops;
> efivars->kobject = kobject;
>
> --
> 1.9.1
>
On 11/13/2018 11:50 PM, Ard Biesheuvel wrote:
> Hi Arend,
>
> On Tue, 13 Nov 2018 at 12:51, Arend van Spriel
> <[email protected]> wrote:
>>
>> Since commit ce2e6db554fa ("brcmfmac: Add support for getting nvram
>> contents from EFI variables") we have a device driver accessing the
>> efivars api (since next-20181107). Several functions in efivars api
>> assume __efivars is set, ie. will be accessed after efivars_register()
>> has been called. However, following NULL pointer access was reported
>> calling efivar_entry_size() from the brcmfmac device driver.
>>
>> [ 14.177769] Unable to handle kernel NULL pointer dereference at
>> virtual address 00000008
>> [ 14.197303] pgd = 60bfa5f1
>> [ 14.211842] [00000008] *pgd=00000000
>> [ 14.227373] Internal error: Oops: 5 [#1] SMP ARM
>> [ 14.244244] Modules linked in: brcmfmac sha256_generic sha256_arm snd cfg80211 brcmutil soundcore snd_soc_tegra30_ahub tegra_wdt
>> [ 14.269109] CPU: 1 PID: 114 Comm: kworker/1:2 Not tainted 4.20.0-rc1-next-20181107-gd881de3 #1
>> [ 14.269114] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>> [ 14.269154] Workqueue: events request_firmware_work_func
>> [ 14.269177] PC is at efivar_entry_size+0x28/0x90
>> [ 14.269362] LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
>> [ 14.269369] pc : [<c0c40718>] lr : [<bf2a3ef4>] psr: a00d0113
>> [ 14.269374] sp : ede7fe28 ip : ee983410 fp : c1787f30
>> [ 14.269378] r10: 00000000 r9 : 00000000 r8 : bf2b2258
>> [ 14.269384] r7 : ee983000 r6 : c1604c48 r5 : ede7fe88 r4 : edf337c0
>> [ 14.269389] r3 : 00000000 r2 : 00000000 r1 : ede7fe88 r0 : c17712c8
>> [ 14.269398] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
>> [ 14.269404] Control: 10c5387d Table: ad16804a DAC: 00000051
>>
>> Disassembly showed that the local static variable __efivars is NULL. Likely
>> because efivars_register() is not called on this Tegra platform. So adding
>> a NULL pointer check in efivar_entry_size() and similar functions while at
>> it. In efivars_register() a couple of sanity checks have been added.
>>
>> Cc: Hans de Goede <[email protected]>
>> Reported-by: Jon Hunter <[email protected]>
>> Signed-off-by: Arend van Spriel <[email protected]>
>> ---
>> drivers/firmware/efi/vars.c | 108 +++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 87 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
>> index 9336ffd..5fbd8e7 100644
>> --- a/drivers/firmware/efi/vars.c
>> +++ b/drivers/firmware/efi/vars.c
>> @@ -318,7 +318,12 @@ struct variable_validate {
>> 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 @@ struct variable_validate {
>> 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 @@ int __efivar_entry_delete(struct efivar_entry *entry)
>> */
>> 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;
>>
>
> Any reason in particular you put the check after the lock is taken? Is
> this to ensure that __efivars does not change under your feet?
That was my reasoning although I am not sure if that is likely. Is
efivars_register() always called before any drivers are started?
Regards,
Arend
On Tue, 13 Nov 2018 at 15:04, Arend van Spriel
<[email protected]> wrote:
>
> On 11/13/2018 11:50 PM, Ard Biesheuvel wrote:
> > Hi Arend,
> >
> > On Tue, 13 Nov 2018 at 12:51, Arend van Spriel
> > <[email protected]> wrote:
> >>
> >> Since commit ce2e6db554fa ("brcmfmac: Add support for getting nvram
> >> contents from EFI variables") we have a device driver accessing the
> >> efivars api (since next-20181107). Several functions in efivars api
> >> assume __efivars is set, ie. will be accessed after efivars_register()
> >> has been called. However, following NULL pointer access was reported
> >> calling efivar_entry_size() from the brcmfmac device driver.
> >>
> >> [ 14.177769] Unable to handle kernel NULL pointer dereference at
> >> virtual address 00000008
> >> [ 14.197303] pgd = 60bfa5f1
> >> [ 14.211842] [00000008] *pgd=00000000
> >> [ 14.227373] Internal error: Oops: 5 [#1] SMP ARM
> >> [ 14.244244] Modules linked in: brcmfmac sha256_generic sha256_arm snd cfg80211 brcmutil soundcore snd_soc_tegra30_ahub tegra_wdt
> >> [ 14.269109] CPU: 1 PID: 114 Comm: kworker/1:2 Not tainted 4.20.0-rc1-next-20181107-gd881de3 #1
> >> [ 14.269114] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> >> [ 14.269154] Workqueue: events request_firmware_work_func
> >> [ 14.269177] PC is at efivar_entry_size+0x28/0x90
> >> [ 14.269362] LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
> >> [ 14.269369] pc : [<c0c40718>] lr : [<bf2a3ef4>] psr: a00d0113
> >> [ 14.269374] sp : ede7fe28 ip : ee983410 fp : c1787f30
> >> [ 14.269378] r10: 00000000 r9 : 00000000 r8 : bf2b2258
> >> [ 14.269384] r7 : ee983000 r6 : c1604c48 r5 : ede7fe88 r4 : edf337c0
> >> [ 14.269389] r3 : 00000000 r2 : 00000000 r1 : ede7fe88 r0 : c17712c8
> >> [ 14.269398] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> >> [ 14.269404] Control: 10c5387d Table: ad16804a DAC: 00000051
> >>
> >> Disassembly showed that the local static variable __efivars is NULL. Likely
> >> because efivars_register() is not called on this Tegra platform. So adding
> >> a NULL pointer check in efivar_entry_size() and similar functions while at
> >> it. In efivars_register() a couple of sanity checks have been added.
> >>
> >> Cc: Hans de Goede <[email protected]>
> >> Reported-by: Jon Hunter <[email protected]>
> >> Signed-off-by: Arend van Spriel <[email protected]>
> >> ---
> >> drivers/firmware/efi/vars.c | 108 +++++++++++++++++++++++++++++++++++---------
> >> 1 file changed, 87 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> >> index 9336ffd..5fbd8e7 100644
> >> --- a/drivers/firmware/efi/vars.c
> >> +++ b/drivers/firmware/efi/vars.c
> >> @@ -318,7 +318,12 @@ struct variable_validate {
> >> 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 @@ struct variable_validate {
> >> 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 @@ int __efivar_entry_delete(struct efivar_entry *entry)
> >> */
> >> 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;
> >>
> >
> > Any reason in particular you put the check after the lock is taken? Is
> > this to ensure that __efivars does not change under your feet?
>
> That was my reasoning although I am not sure if that is likely. Is
> efivars_register() always called before any drivers are started?
>
First of all, efivars_register() is only called on EFI systems, and
this Tegra is not an EFI system. So in general, it would be better if
your code checks efi_enabled(EFI_RUNTIME_SERVICES) before attempting
to access any EFI APIs, especially since CONFIG_EFI support can be
compiled out (in which case efi_enabled() resolves to a build time
constant 'false') Something like
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -446,7 +446,6 @@ struct brcmf_fw {
static void brcmf_fw_request_done(const struct firmware *fw, void *ctx);
-#ifdef CONFIG_EFI
/* In some cases the EFI-var stored nvram contains "ccode=ALL" or "ccode=XV"
* to specify "worldwide" compatible settings, but these 2 ccode-s do not work
* properly. "ccode=ALL" causes channels 12 and 13 to not be available,
@@ -478,6 +477,9 @@ static u8 *brcmf_fw_nvram_from_efi(size_t *data_len_ret)
u8 *data = NULL;
int err;
+ if (!efi_enabled(EFI_RUNTIME_SERVICES))
+ return NULL;
+
nvram_efivar = kzalloc(sizeof(*nvram_efivar), GFP_KERNEL);
if (!nvram_efivar)
return NULL;
@@ -511,9 +513,6 @@ static u8 *brcmf_fw_nvram_from_efi(size_t *data_len_ret)
kfree(nvram_efivar);
return NULL;
}
-#else
-static u8 *brcmf_fw_nvram_from_efi(size_t *data_len) { return NULL; }
-#endif
static void brcmf_fw_free_request(struct brcmf_fw_request *req)
{
Of course, that doesn't mean we should crash if you access the API
anyway, so I will queue this patch in efi/next as well. (I'll drop the
last hunk, though, if you don't mind - it returns with the lock held)
Thanks,
Ard.
On 11/15/2018 2:54 PM, Ard Biesheuvel wrote:
> On Tue, 13 Nov 2018 at 15:04, Arend van Spriel
> <[email protected]> wrote:
>>
>> On 11/13/2018 11:50 PM, Ard Biesheuvel wrote:
>>> Hi Arend,
>>>
>>> On Tue, 13 Nov 2018 at 12:51, Arend van Spriel
>>> <[email protected]> wrote:
>>>>
>>>> Since commit ce2e6db554fa ("brcmfmac: Add support for getting nvram
>>>> contents from EFI variables") we have a device driver accessing the
>>>> efivars api (since next-20181107). Several functions in efivars api
>>>> assume __efivars is set, ie. will be accessed after efivars_register()
>>>> has been called. However, following NULL pointer access was reported
>>>> calling efivar_entry_size() from the brcmfmac device driver.
>>>>
>>>> [ 14.177769] Unable to handle kernel NULL pointer dereference at
>>>> virtual address 00000008
>>>> [ 14.197303] pgd = 60bfa5f1
>>>> [ 14.211842] [00000008] *pgd=00000000
>>>> [ 14.227373] Internal error: Oops: 5 [#1] SMP ARM
>>>> [ 14.244244] Modules linked in: brcmfmac sha256_generic sha256_arm snd cfg80211 brcmutil soundcore snd_soc_tegra30_ahub tegra_wdt
>>>> [ 14.269109] CPU: 1 PID: 114 Comm: kworker/1:2 Not tainted 4.20.0-rc1-next-20181107-gd881de3 #1
>>>> [ 14.269114] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
>>>> [ 14.269154] Workqueue: events request_firmware_work_func
>>>> [ 14.269177] PC is at efivar_entry_size+0x28/0x90
>>>> [ 14.269362] LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
>>>> [ 14.269369] pc : [<c0c40718>] lr : [<bf2a3ef4>] psr: a00d0113
>>>> [ 14.269374] sp : ede7fe28 ip : ee983410 fp : c1787f30
>>>> [ 14.269378] r10: 00000000 r9 : 00000000 r8 : bf2b2258
>>>> [ 14.269384] r7 : ee983000 r6 : c1604c48 r5 : ede7fe88 r4 : edf337c0
>>>> [ 14.269389] r3 : 00000000 r2 : 00000000 r1 : ede7fe88 r0 : c17712c8
>>>> [ 14.269398] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
>>>> [ 14.269404] Control: 10c5387d Table: ad16804a DAC: 00000051
>>>>
>>>> Disassembly showed that the local static variable __efivars is NULL. Likely
>>>> because efivars_register() is not called on this Tegra platform. So adding
>>>> a NULL pointer check in efivar_entry_size() and similar functions while at
>>>> it. In efivars_register() a couple of sanity checks have been added.
>>>>
>>>> Cc: Hans de Goede <[email protected]>
>>>> Reported-by: Jon Hunter <[email protected]>
>>>> Signed-off-by: Arend van Spriel <[email protected]>
>>>> ---
>>>> drivers/firmware/efi/vars.c | 108 +++++++++++++++++++++++++++++++++++---------
>>>> 1 file changed, 87 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
>>>> index 9336ffd..5fbd8e7 100644
>>>> --- a/drivers/firmware/efi/vars.c
>>>> +++ b/drivers/firmware/efi/vars.c
>>>> @@ -318,7 +318,12 @@ struct variable_validate {
>>>> 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 @@ struct variable_validate {
>>>> 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 @@ int __efivar_entry_delete(struct efivar_entry *entry)
>>>> */
>>>> 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;
>>>>
>>>
>>> Any reason in particular you put the check after the lock is taken? Is
>>> this to ensure that __efivars does not change under your feet?
>>
>> That was my reasoning although I am not sure if that is likely. Is
>> efivars_register() always called before any drivers are started?
>>
>
> First of all, efivars_register() is only called on EFI systems, and
> this Tegra is not an EFI system. So in general, it would be better if
> your code checks efi_enabled(EFI_RUNTIME_SERVICES) before attempting
> to access any EFI APIs, especially since CONFIG_EFI support can be
> compiled out (in which case efi_enabled() resolves to a build time
> constant 'false') Something like
That was my first stab at it, but felt that the API could be more robust
as well. I will create a patch for my driver as well.
>
> Of course, that doesn't mean we should crash if you access the API
> anyway, so I will queue this patch in efi/next as well. (I'll drop the
> last hunk, though, if you don't mind - it returns with the lock held)
Works for me.
Thanks,
Arend