Patchwork firmware: efi: add NULL pointer checks in efivars api functions

login
register
mail settings
Submitter Ard Biesheuvel
Date Nov. 15, 2018, 1:54 p.m.
Message ID <CAKv+Gu8oQeMLZo=DtvS+D=7GcadSrqhSX2pndHOr-KTQQ2fxPw@mail.gmail.com>
Download mbox | patch
Permalink /patch/657599/
State New
Headers show

Comments

Ard Biesheuvel - Nov. 15, 2018, 1:54 p.m.
On Tue, 13 Nov 2018 at 15:04, Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On 11/13/2018 11:50 PM, Ard Biesheuvel wrote:
> > Hi Arend,
> >
> > On Tue, 13 Nov 2018 at 12:51, Arend van Spriel
> > <arend.vanspriel@broadcom.com> 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 <hdegoede@redhat.com>
> >> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> >> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> >> ---
> >>  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



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.
Arend Van Spriel - Nov. 16, 2018, 9:30 p.m.
On 11/15/2018 2:54 PM, Ard Biesheuvel wrote:
> On Tue, 13 Nov 2018 at 15:04, Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>>
>> On 11/13/2018 11:50 PM, Ard Biesheuvel wrote:
>>> Hi Arend,
>>>
>>> On Tue, 13 Nov 2018 at 12:51, Arend van Spriel
>>> <arend.vanspriel@broadcom.com> 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 <hdegoede@redhat.com>
>>>> Reported-by: Jon Hunter <jonathanh@nvidia.com>
>>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>>>> ---
>>>>  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

Patch

--- 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)
 {