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

[Ingo Molnar]

* Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:

> From: Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Reported-by: Jon Hunter <jonathanh@xxxxxxxxxx>
> Signed-off-by: Arend van Spriel <arend.vanspriel@xxxxxxxxxxxx>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>

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