diff mbox series

[RESEND,v2] efi: Only print errors about failing to get certs if EFI vars are found

Message ID 20200217113947.2070436-1-javierm@redhat.com (mailing list archive)
State New, archived
Headers show
Series [RESEND,v2] efi: Only print errors about failing to get certs if EFI vars are found | expand

Commit Message

Javier Martinez Canillas Feb. 17, 2020, 11:39 a.m. UTC 
If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
from the db, dbx and MokListRT EFI variables into the appropriate keyrings.

But it just assumes that the variables will be present and prints an error
if the certs can't be loaded, even when is possible that the variables may
not exist. For example the MokListRT variable will only be present if shim
is used.

So only print an error message about failing to get the certs list from an
EFI variable if this is found. Otherwise these printed errors just pollute
the kernel log ring buffer with confusing messages like the following:

[    5.427251] Couldn't get size: 0x800000000000000e
[    5.427261] MODSIGN: Couldn't get UEFI db list
[    5.428012] Couldn't get size: 0x800000000000000e
[    5.428023] Couldn't get UEFI MokListRT

Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>

---

Changes in v2:
- Fix flaws in the logic, that caused the signature list was parsed if
  the return code was EFI_NOT_FOUND that pointed out Hans de Goede.
- Print debug messages if the variables are not found.

 security/integrity/platform_certs/load_uefi.c | 40 ++++++++++++-------
 1 file changed, 26 insertions(+), 14 deletions(-)

Comments

Ard Biesheuvel Feb. 17, 2020, 1:37 p.m. UTC | #1 
On Mon, 17 Feb 2020 at 12:40, Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
>
> But it just assumes that the variables will be present and prints an error
> if the certs can't be loaded, even when is possible that the variables may
> not exist. For example the MokListRT variable will only be present if shim
> is used.
>
> So only print an error message about failing to get the certs list from an
> EFI variable if this is found. Otherwise these printed errors just pollute
> the kernel log ring buffer with confusing messages like the following:
>
> [    5.427251] Couldn't get size: 0x800000000000000e
> [    5.427261] MODSIGN: Couldn't get UEFI db list
> [    5.428012] Couldn't get size: 0x800000000000000e
> [    5.428023] Couldn't get UEFI MokListRT
>
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

>
> ---
>
> Changes in v2:
> - Fix flaws in the logic, that caused the signature list was parsed if
>   the return code was EFI_NOT_FOUND that pointed out Hans de Goede.
> - Print debug messages if the variables are not found.
>
>  security/integrity/platform_certs/load_uefi.c | 40 ++++++++++++-------
>  1 file changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> index 111898aad56..f0c90824196 100644
> --- a/security/integrity/platform_certs/load_uefi.c
> +++ b/security/integrity/platform_certs/load_uefi.c
> @@ -35,16 +35,18 @@ static __init bool uefi_check_ignore_db(void)
>   * Get a certificate list blob from the named EFI variable.
>   */
>  static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> -                                 unsigned long *size)
> +                                 unsigned long *size, efi_status_t *status)
>  {
> -       efi_status_t status;
>         unsigned long lsize = 4;
>         unsigned long tmpdb[4];
>         void *db;
>
> -       status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> -       if (status != EFI_BUFFER_TOO_SMALL) {
> -               pr_err("Couldn't get size: 0x%lx\n", status);
> +       *status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> +       if (*status == EFI_NOT_FOUND)
> +               return NULL;
> +
> +       if (*status != EFI_BUFFER_TOO_SMALL) {
> +               pr_err("Couldn't get size: 0x%lx\n", *status);
>                 return NULL;
>         }
>
> @@ -52,10 +54,10 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
>         if (!db)
>                 return NULL;
>
> -       status = efi.get_variable(name, guid, NULL, &lsize, db);
> -       if (status != EFI_SUCCESS) {
> +       *status = efi.get_variable(name, guid, NULL, &lsize, db);
> +       if (*status != EFI_SUCCESS) {
>                 kfree(db);
> -               pr_err("Error reading db var: 0x%lx\n", status);
> +               pr_err("Error reading db var: 0x%lx\n", *status);
>                 return NULL;
>         }
>
> @@ -74,6 +76,7 @@ static int __init load_uefi_certs(void)
>         efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
>         void *db = NULL, *dbx = NULL, *mok = NULL;
>         unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
> +       efi_status_t status;
>         int rc = 0;
>
>         if (!efi.get_variable)
> @@ -83,9 +86,12 @@ static int __init load_uefi_certs(void)
>          * an error if we can't get them.
>          */
>         if (!uefi_check_ignore_db()) {
> -               db = get_cert_list(L"db", &secure_var, &dbsize);
> +               db = get_cert_list(L"db", &secure_var, &dbsize, &status);
>                 if (!db) {
> -                       pr_err("MODSIGN: Couldn't get UEFI db list\n");
> +                       if (status == EFI_NOT_FOUND)
> +                               pr_debug("MODSIGN: db variable wasn't found\n");
> +                       else
> +                               pr_err("MODSIGN: Couldn't get UEFI db list\n");
>                 } else {
>                         rc = parse_efi_signature_list("UEFI:db",
>                                         db, dbsize, get_handler_for_db);
> @@ -96,9 +102,12 @@ static int __init load_uefi_certs(void)
>                 }
>         }
>
> -       mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
> +       mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
>         if (!mok) {
> -               pr_info("Couldn't get UEFI MokListRT\n");
> +               if (status == EFI_NOT_FOUND)
> +                       pr_debug("MokListRT variable wasn't found\n");
> +               else
> +                       pr_info("Couldn't get UEFI MokListRT\n");
>         } else {
>                 rc = parse_efi_signature_list("UEFI:MokListRT",
>                                               mok, moksize, get_handler_for_db);
> @@ -107,9 +116,12 @@ static int __init load_uefi_certs(void)
>                 kfree(mok);
>         }
>
> -       dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
> +       dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, &status);
>         if (!dbx) {
> -               pr_info("Couldn't get UEFI dbx list\n");
> +               if (status == EFI_NOT_FOUND)
> +                       pr_debug("dbx variable wasn't found\n");
> +               else
> +                       pr_info("Couldn't get UEFI dbx list\n");
>         } else {
>                 rc = parse_efi_signature_list("UEFI:dbx",
>                                               dbx, dbxsize,
> --
> 2.24.1
>
David Hildenbrand Feb. 28, 2020, 9:19 a.m. UTC | #2 
On 17.02.20 12:39, Javier Martinez Canillas wrote:
> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
> 
> But it just assumes that the variables will be present and prints an error
> if the certs can't be loaded, even when is possible that the variables may
> not exist. For example the MokListRT variable will only be present if shim
> is used.
> 
> So only print an error message about failing to get the certs list from an
> EFI variable if this is found. Otherwise these printed errors just pollute
> the kernel log ring buffer with confusing messages like the following:
> 
> [    5.427251] Couldn't get size: 0x800000000000000e
> [    5.427261] MODSIGN: Couldn't get UEFI db list
> [    5.428012] Couldn't get size: 0x800000000000000e
> [    5.428023] Couldn't get UEFI MokListRT
> 
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>

This patch seems to break a very basic x86-64 QEMU setup (booting
upstream kernel with a F31 initrd - are you running basic boot tests?).
Luckily, it only took me 5 minutes to identify this patch. Reverting
this patch from linux-next fixes it for me.


[    1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
[    1.044314] zswap: loaded using pool lzo/zbud
[    1.045663] Key type ._fscrypt registered
[    1.046154] Key type .fscrypt registered
[    1.046524] Key type fscrypt-provisioning registered
[    1.051178] Key type big_key registered
[    1.055108] Key type encrypted registered
[    1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
[    1.056172] #PF: supervisor instruction fetch in kernel mode
[    1.056706] #PF: error_code(0x0010) - not-present page
[    1.057367] PGD 0 P4D 0 
[    1.057729] Oops: 0010 [#1] SMP NOPTI
[    1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
[    1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
[    1.060230] RIP: 0010:0x0
[    1.060478] Code: Bad RIP value.
[    1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
[    1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
[    1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
[    1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
[    1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
[    1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
[    1.064609] FS:  0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
[    1.065360] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
[    1.066562] Call Trace:
[    1.066803]  load_uefi_certs+0xc8/0x2bb
[    1.067171]  ? get_cert_list+0xfb/0xfb
[    1.067523]  do_one_initcall+0x5d/0x2f0
[    1.067894]  ? rcu_read_lock_sched_held+0x52/0x80
[    1.068337]  kernel_init_freeable+0x243/0x2c2
[    1.068751]  ? rest_init+0x23a/0x23a
[    1.069095]  kernel_init+0xa/0x106
[    1.069416]  ret_from_fork+0x27/0x50
[    1.069759] Modules linked in:
[    1.070050] CR2: 0000000000000000
[    1.070361] ---[ end trace fcce9bb4feb21d99 ]---
David Hildenbrand Feb. 28, 2020, 9:31 a.m. UTC | #3 
On 28.02.20 10:19, David Hildenbrand wrote:
> On 17.02.20 12:39, Javier Martinez Canillas wrote:
>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
>>
>> But it just assumes that the variables will be present and prints an error
>> if the certs can't be loaded, even when is possible that the variables may
>> not exist. For example the MokListRT variable will only be present if shim
>> is used.
>>
>> So only print an error message about failing to get the certs list from an
>> EFI variable if this is found. Otherwise these printed errors just pollute
>> the kernel log ring buffer with confusing messages like the following:
>>
>> [    5.427251] Couldn't get size: 0x800000000000000e
>> [    5.427261] MODSIGN: Couldn't get UEFI db list
>> [    5.428012] Couldn't get size: 0x800000000000000e
>> [    5.428023] Couldn't get UEFI MokListRT
>>
>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> Tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> This patch seems to break a very basic x86-64 QEMU setup (booting
> upstream kernel with a F31 initrd - are you running basic boot tests?).
> Luckily, it only took me 5 minutes to identify this patch. Reverting
> this patch from linux-next fixes it for me.
> 
> 
> [    1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
> [    1.044314] zswap: loaded using pool lzo/zbud
> [    1.045663] Key type ._fscrypt registered
> [    1.046154] Key type .fscrypt registered
> [    1.046524] Key type fscrypt-provisioning registered
> [    1.051178] Key type big_key registered
> [    1.055108] Key type encrypted registered
> [    1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [    1.056172] #PF: supervisor instruction fetch in kernel mode
> [    1.056706] #PF: error_code(0x0010) - not-present page
> [    1.057367] PGD 0 P4D 0 
> [    1.057729] Oops: 0010 [#1] SMP NOPTI
> [    1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
> [    1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
> [    1.060230] RIP: 0010:0x0
> [    1.060478] Code: Bad RIP value.
> [    1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
> [    1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
> [    1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
> [    1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
> [    1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
> [    1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
> [    1.064609] FS:  0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
> [    1.065360] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
> [    1.066562] Call Trace:
> [    1.066803]  load_uefi_certs+0xc8/0x2bb
> [    1.067171]  ? get_cert_list+0xfb/0xfb
> [    1.067523]  do_one_initcall+0x5d/0x2f0
> [    1.067894]  ? rcu_read_lock_sched_held+0x52/0x80
> [    1.068337]  kernel_init_freeable+0x243/0x2c2
> [    1.068751]  ? rest_init+0x23a/0x23a
> [    1.069095]  kernel_init+0xa/0x106
> [    1.069416]  ret_from_fork+0x27/0x50
> [    1.069759] Modules linked in:
> [    1.070050] CR2: 0000000000000000
> [    1.070361] ---[ end trace fcce9bb4feb21d99 ]---
> 
> 

Sorry, wrong mail identified, the patch is actually

commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
Author: Ard Biesheuvel <ardb@kernel.org>
Date:   Sun Feb 16 19:46:25 2020 +0100

    integrity: Check properly whether EFI GetVariable() is available

    Testing the value of the efi.get_variable function pointer is not

which made it work. (not even able to find that patch on lkml ...)
Ard Biesheuvel Feb. 28, 2020, 9:35 a.m. UTC | #4 
On Fri, 28 Feb 2020 at 10:31, David Hildenbrand <david@redhat.com> wrote:
>
> On 28.02.20 10:19, David Hildenbrand wrote:
> > On 17.02.20 12:39, Javier Martinez Canillas wrote:
> >> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
> >> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
> >>
> >> But it just assumes that the variables will be present and prints an error
> >> if the certs can't be loaded, even when is possible that the variables may
> >> not exist. For example the MokListRT variable will only be present if shim
> >> is used.
> >>
> >> So only print an error message about failing to get the certs list from an
> >> EFI variable if this is found. Otherwise these printed errors just pollute
> >> the kernel log ring buffer with confusing messages like the following:
> >>
> >> [    5.427251] Couldn't get size: 0x800000000000000e
> >> [    5.427261] MODSIGN: Couldn't get UEFI db list
> >> [    5.428012] Couldn't get size: 0x800000000000000e
> >> [    5.428023] Couldn't get UEFI MokListRT
> >>
> >> Reported-by: Hans de Goede <hdegoede@redhat.com>
> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >> Tested-by: Hans de Goede <hdegoede@redhat.com>
> >
> > This patch seems to break a very basic x86-64 QEMU setup (booting
> > upstream kernel with a F31 initrd - are you running basic boot tests?).
> > Luckily, it only took me 5 minutes to identify this patch. Reverting
> > this patch from linux-next fixes it for me.
> >
> >
> > [    1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
> > [    1.044314] zswap: loaded using pool lzo/zbud
> > [    1.045663] Key type ._fscrypt registered
> > [    1.046154] Key type .fscrypt registered
> > [    1.046524] Key type fscrypt-provisioning registered
> > [    1.051178] Key type big_key registered
> > [    1.055108] Key type encrypted registered
> > [    1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > [    1.056172] #PF: supervisor instruction fetch in kernel mode
> > [    1.056706] #PF: error_code(0x0010) - not-present page
> > [    1.057367] PGD 0 P4D 0
> > [    1.057729] Oops: 0010 [#1] SMP NOPTI
> > [    1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
> > [    1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
> > [    1.060230] RIP: 0010:0x0
> > [    1.060478] Code: Bad RIP value.
> > [    1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
> > [    1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
> > [    1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
> > [    1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
> > [    1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
> > [    1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
> > [    1.064609] FS:  0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
> > [    1.065360] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [    1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
> > [    1.066562] Call Trace:
> > [    1.066803]  load_uefi_certs+0xc8/0x2bb
> > [    1.067171]  ? get_cert_list+0xfb/0xfb
> > [    1.067523]  do_one_initcall+0x5d/0x2f0
> > [    1.067894]  ? rcu_read_lock_sched_held+0x52/0x80
> > [    1.068337]  kernel_init_freeable+0x243/0x2c2
> > [    1.068751]  ? rest_init+0x23a/0x23a
> > [    1.069095]  kernel_init+0xa/0x106
> > [    1.069416]  ret_from_fork+0x27/0x50
> > [    1.069759] Modules linked in:
> > [    1.070050] CR2: 0000000000000000
> > [    1.070361] ---[ end trace fcce9bb4feb21d99 ]---
> >
> >
>
> Sorry, wrong mail identified, the patch is actually
>
> commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
> Author: Ard Biesheuvel <ardb@kernel.org>
> Date:   Sun Feb 16 19:46:25 2020 +0100
>
>     integrity: Check properly whether EFI GetVariable() is available
>
>     Testing the value of the efi.get_variable function pointer is not
>
> which made it work. (not even able to find that patch on lkml ...)
>

https://lore.kernel.org/linux-efi/20200219171907.11894-10-ardb@kernel.org/T/#u

Interesting. So reverting that patch makes things work again?

Could you share your QEMU command line? I assume the bug is caused by
the fact that efi.get_variable == NULL
David Hildenbrand Feb. 28, 2020, 9:35 a.m. UTC | #5 
On 28.02.20 10:31, David Hildenbrand wrote:
> On 28.02.20 10:19, David Hildenbrand wrote:
>> On 17.02.20 12:39, Javier Martinez Canillas wrote:
>>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
>>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
>>>
>>> But it just assumes that the variables will be present and prints an error
>>> if the certs can't be loaded, even when is possible that the variables may
>>> not exist. For example the MokListRT variable will only be present if shim
>>> is used.
>>>
>>> So only print an error message about failing to get the certs list from an
>>> EFI variable if this is found. Otherwise these printed errors just pollute
>>> the kernel log ring buffer with confusing messages like the following:
>>>
>>> [    5.427251] Couldn't get size: 0x800000000000000e
>>> [    5.427261] MODSIGN: Couldn't get UEFI db list
>>> [    5.428012] Couldn't get size: 0x800000000000000e
>>> [    5.428023] Couldn't get UEFI MokListRT
>>>
>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>
>> This patch seems to break a very basic x86-64 QEMU setup (booting
>> upstream kernel with a F31 initrd - are you running basic boot tests?).
>> Luckily, it only took me 5 minutes to identify this patch. Reverting
>> this patch from linux-next fixes it for me.
>>
>>
>> [    1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
>> [    1.044314] zswap: loaded using pool lzo/zbud
>> [    1.045663] Key type ._fscrypt registered
>> [    1.046154] Key type .fscrypt registered
>> [    1.046524] Key type fscrypt-provisioning registered
>> [    1.051178] Key type big_key registered
>> [    1.055108] Key type encrypted registered
>> [    1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
>> [    1.056172] #PF: supervisor instruction fetch in kernel mode
>> [    1.056706] #PF: error_code(0x0010) - not-present page
>> [    1.057367] PGD 0 P4D 0 
>> [    1.057729] Oops: 0010 [#1] SMP NOPTI
>> [    1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
>> [    1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
>> [    1.060230] RIP: 0010:0x0
>> [    1.060478] Code: Bad RIP value.
>> [    1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
>> [    1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
>> [    1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
>> [    1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
>> [    1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
>> [    1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
>> [    1.064609] FS:  0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
>> [    1.065360] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [    1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
>> [    1.066562] Call Trace:
>> [    1.066803]  load_uefi_certs+0xc8/0x2bb
>> [    1.067171]  ? get_cert_list+0xfb/0xfb
>> [    1.067523]  do_one_initcall+0x5d/0x2f0
>> [    1.067894]  ? rcu_read_lock_sched_held+0x52/0x80
>> [    1.068337]  kernel_init_freeable+0x243/0x2c2
>> [    1.068751]  ? rest_init+0x23a/0x23a
>> [    1.069095]  kernel_init+0xa/0x106
>> [    1.069416]  ret_from_fork+0x27/0x50
>> [    1.069759] Modules linked in:
>> [    1.070050] CR2: 0000000000000000
>> [    1.070361] ---[ end trace fcce9bb4feb21d99 ]---
>>
>>
> 
> Sorry, wrong mail identified, the patch is actually
> 
> commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
> Author: Ard Biesheuvel <ardb@kernel.org>
> Date:   Sun Feb 16 19:46:25 2020 +0100
> 
>     integrity: Check properly whether EFI GetVariable() is available
> 
>     Testing the value of the efi.get_variable function pointer is not
> 
> which made it work. (not even able to find that patch on lkml ...)

To clarify for Ard, your patch breaks a basic QEMU setup (see above,
NULL pointer dereference). Reverting your patch from linux-next makes it
work again.
Ard Biesheuvel Feb. 28, 2020, 9:39 a.m. UTC | #6 
On Fri, 28 Feb 2020 at 10:35, David Hildenbrand <david@redhat.com> wrote:
>
> On 28.02.20 10:31, David Hildenbrand wrote:
> > On 28.02.20 10:19, David Hildenbrand wrote:
> >> On 17.02.20 12:39, Javier Martinez Canillas wrote:
> >>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
> >>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
> >>>
> >>> But it just assumes that the variables will be present and prints an error
> >>> if the certs can't be loaded, even when is possible that the variables may
> >>> not exist. For example the MokListRT variable will only be present if shim
> >>> is used.
> >>>
> >>> So only print an error message about failing to get the certs list from an
> >>> EFI variable if this is found. Otherwise these printed errors just pollute
> >>> the kernel log ring buffer with confusing messages like the following:
> >>>
> >>> [    5.427251] Couldn't get size: 0x800000000000000e
> >>> [    5.427261] MODSIGN: Couldn't get UEFI db list
> >>> [    5.428012] Couldn't get size: 0x800000000000000e
> >>> [    5.428023] Couldn't get UEFI MokListRT
> >>>
> >>> Reported-by: Hans de Goede <hdegoede@redhat.com>
> >>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >>> Tested-by: Hans de Goede <hdegoede@redhat.com>
> >>
> >> This patch seems to break a very basic x86-64 QEMU setup (booting
> >> upstream kernel with a F31 initrd - are you running basic boot tests?).
> >> Luckily, it only took me 5 minutes to identify this patch. Reverting
> >> this patch from linux-next fixes it for me.
> >>
> >>
> >> [    1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
> >> [    1.044314] zswap: loaded using pool lzo/zbud
> >> [    1.045663] Key type ._fscrypt registered
> >> [    1.046154] Key type .fscrypt registered
> >> [    1.046524] Key type fscrypt-provisioning registered
> >> [    1.051178] Key type big_key registered
> >> [    1.055108] Key type encrypted registered
> >> [    1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
> >> [    1.056172] #PF: supervisor instruction fetch in kernel mode
> >> [    1.056706] #PF: error_code(0x0010) - not-present page
> >> [    1.057367] PGD 0 P4D 0
> >> [    1.057729] Oops: 0010 [#1] SMP NOPTI
> >> [    1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
> >> [    1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
> >> [    1.060230] RIP: 0010:0x0
> >> [    1.060478] Code: Bad RIP value.
> >> [    1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
> >> [    1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
> >> [    1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
> >> [    1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
> >> [    1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
> >> [    1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
> >> [    1.064609] FS:  0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
> >> [    1.065360] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [    1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
> >> [    1.066562] Call Trace:
> >> [    1.066803]  load_uefi_certs+0xc8/0x2bb
> >> [    1.067171]  ? get_cert_list+0xfb/0xfb
> >> [    1.067523]  do_one_initcall+0x5d/0x2f0
> >> [    1.067894]  ? rcu_read_lock_sched_held+0x52/0x80
> >> [    1.068337]  kernel_init_freeable+0x243/0x2c2
> >> [    1.068751]  ? rest_init+0x23a/0x23a
> >> [    1.069095]  kernel_init+0xa/0x106
> >> [    1.069416]  ret_from_fork+0x27/0x50
> >> [    1.069759] Modules linked in:
> >> [    1.070050] CR2: 0000000000000000
> >> [    1.070361] ---[ end trace fcce9bb4feb21d99 ]---
> >>
> >>
> >
> > Sorry, wrong mail identified, the patch is actually
> >
> > commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
> > Author: Ard Biesheuvel <ardb@kernel.org>
> > Date:   Sun Feb 16 19:46:25 2020 +0100
> >
> >     integrity: Check properly whether EFI GetVariable() is available
> >
> >     Testing the value of the efi.get_variable function pointer is not
> >
> > which made it work. (not even able to find that patch on lkml ...)
>
> To clarify for Ard, your patch breaks a basic QEMU setup (see above,
> NULL pointer dereference). Reverting your patch from linux-next makes it
> work again.
>

Does this fix it?

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 41269a95ff85..d1746a579c99 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -300,12 +300,12 @@ static int __init efisubsys_init(void)
 {
        int error;

-       if (!efi_enabled(EFI_BOOT))
-               return 0;
-
        if (!efi_enabled(EFI_RUNTIME_SERVICES))
                efi.runtime_supported_mask = 0;

+       if (!efi_enabled(EFI_BOOT))
+               return 0;
+
        if (efi.runtime_supported_mask) {
                /*
                 * Since we process only one efi_runtime_service() at a time, an
David Hildenbrand Feb. 28, 2020, 9:43 a.m. UTC | #7 
On 28.02.20 10:39, Ard Biesheuvel wrote:
> On Fri, 28 Feb 2020 at 10:35, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 28.02.20 10:31, David Hildenbrand wrote:
>>> On 28.02.20 10:19, David Hildenbrand wrote:
>>>> On 17.02.20 12:39, Javier Martinez Canillas wrote:
>>>>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
>>>>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
>>>>>
>>>>> But it just assumes that the variables will be present and prints an error
>>>>> if the certs can't be loaded, even when is possible that the variables may
>>>>> not exist. For example the MokListRT variable will only be present if shim
>>>>> is used.
>>>>>
>>>>> So only print an error message about failing to get the certs list from an
>>>>> EFI variable if this is found. Otherwise these printed errors just pollute
>>>>> the kernel log ring buffer with confusing messages like the following:
>>>>>
>>>>> [    5.427251] Couldn't get size: 0x800000000000000e
>>>>> [    5.427261] MODSIGN: Couldn't get UEFI db list
>>>>> [    5.428012] Couldn't get size: 0x800000000000000e
>>>>> [    5.428023] Couldn't get UEFI MokListRT
>>>>>
>>>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>>>
>>>> This patch seems to break a very basic x86-64 QEMU setup (booting
>>>> upstream kernel with a F31 initrd - are you running basic boot tests?).
>>>> Luckily, it only took me 5 minutes to identify this patch. Reverting
>>>> this patch from linux-next fixes it for me.
>>>>
>>>>
>>>> [    1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
>>>> [    1.044314] zswap: loaded using pool lzo/zbud
>>>> [    1.045663] Key type ._fscrypt registered
>>>> [    1.046154] Key type .fscrypt registered
>>>> [    1.046524] Key type fscrypt-provisioning registered
>>>> [    1.051178] Key type big_key registered
>>>> [    1.055108] Key type encrypted registered
>>>> [    1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
>>>> [    1.056172] #PF: supervisor instruction fetch in kernel mode
>>>> [    1.056706] #PF: error_code(0x0010) - not-present page
>>>> [    1.057367] PGD 0 P4D 0
>>>> [    1.057729] Oops: 0010 [#1] SMP NOPTI
>>>> [    1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
>>>> [    1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
>>>> [    1.060230] RIP: 0010:0x0
>>>> [    1.060478] Code: Bad RIP value.
>>>> [    1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
>>>> [    1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
>>>> [    1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
>>>> [    1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
>>>> [    1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
>>>> [    1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
>>>> [    1.064609] FS:  0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
>>>> [    1.065360] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [    1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
>>>> [    1.066562] Call Trace:
>>>> [    1.066803]  load_uefi_certs+0xc8/0x2bb
>>>> [    1.067171]  ? get_cert_list+0xfb/0xfb
>>>> [    1.067523]  do_one_initcall+0x5d/0x2f0
>>>> [    1.067894]  ? rcu_read_lock_sched_held+0x52/0x80
>>>> [    1.068337]  kernel_init_freeable+0x243/0x2c2
>>>> [    1.068751]  ? rest_init+0x23a/0x23a
>>>> [    1.069095]  kernel_init+0xa/0x106
>>>> [    1.069416]  ret_from_fork+0x27/0x50
>>>> [    1.069759] Modules linked in:
>>>> [    1.070050] CR2: 0000000000000000
>>>> [    1.070361] ---[ end trace fcce9bb4feb21d99 ]---
>>>>
>>>>
>>>
>>> Sorry, wrong mail identified, the patch is actually
>>>
>>> commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
>>> Author: Ard Biesheuvel <ardb@kernel.org>
>>> Date:   Sun Feb 16 19:46:25 2020 +0100
>>>
>>>     integrity: Check properly whether EFI GetVariable() is available
>>>
>>>     Testing the value of the efi.get_variable function pointer is not
>>>
>>> which made it work. (not even able to find that patch on lkml ...)
>>
>> To clarify for Ard, your patch breaks a basic QEMU setup (see above,
>> NULL pointer dereference). Reverting your patch from linux-next makes it
>> work again.
>>
> 
> Does this fix it?
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 41269a95ff85..d1746a579c99 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -300,12 +300,12 @@ static int __init efisubsys_init(void)
>  {
>         int error;
> 
> -       if (!efi_enabled(EFI_BOOT))
> -               return 0;
> -
>         if (!efi_enabled(EFI_RUNTIME_SERVICES))
>                 efi.runtime_supported_mask = 0;
> 
> +       if (!efi_enabled(EFI_BOOT))
> +               return 0;
> +
>         if (efi.runtime_supported_mask) {
>                 /*
>                  * Since we process only one efi_runtime_service() at a time, an
> 

Yes, does the trick!
Ard Biesheuvel Feb. 28, 2020, 9:44 a.m. UTC | #8 
On Fri, 28 Feb 2020 at 10:43, David Hildenbrand <david@redhat.com> wrote:
>
> On 28.02.20 10:39, Ard Biesheuvel wrote:
> > On Fri, 28 Feb 2020 at 10:35, David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 28.02.20 10:31, David Hildenbrand wrote:
> >>> On 28.02.20 10:19, David Hildenbrand wrote:
> >>>> On 17.02.20 12:39, Javier Martinez Canillas wrote:
> >>>>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
> >>>>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
> >>>>>
> >>>>> But it just assumes that the variables will be present and prints an error
> >>>>> if the certs can't be loaded, even when is possible that the variables may
> >>>>> not exist. For example the MokListRT variable will only be present if shim
> >>>>> is used.
> >>>>>
> >>>>> So only print an error message about failing to get the certs list from an
> >>>>> EFI variable if this is found. Otherwise these printed errors just pollute
> >>>>> the kernel log ring buffer with confusing messages like the following:
> >>>>>
> >>>>> [    5.427251] Couldn't get size: 0x800000000000000e
> >>>>> [    5.427261] MODSIGN: Couldn't get UEFI db list
> >>>>> [    5.428012] Couldn't get size: 0x800000000000000e
> >>>>> [    5.428023] Couldn't get UEFI MokListRT
> >>>>>
> >>>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
> >>>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> >>>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
> >>>>
> >>>> This patch seems to break a very basic x86-64 QEMU setup (booting
> >>>> upstream kernel with a F31 initrd - are you running basic boot tests?).
> >>>> Luckily, it only took me 5 minutes to identify this patch. Reverting
> >>>> this patch from linux-next fixes it for me.
> >>>>
> >>>>
> >>>> [    1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
> >>>> [    1.044314] zswap: loaded using pool lzo/zbud
> >>>> [    1.045663] Key type ._fscrypt registered
> >>>> [    1.046154] Key type .fscrypt registered
> >>>> [    1.046524] Key type fscrypt-provisioning registered
> >>>> [    1.051178] Key type big_key registered
> >>>> [    1.055108] Key type encrypted registered
> >>>> [    1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
> >>>> [    1.056172] #PF: supervisor instruction fetch in kernel mode
> >>>> [    1.056706] #PF: error_code(0x0010) - not-present page
> >>>> [    1.057367] PGD 0 P4D 0
> >>>> [    1.057729] Oops: 0010 [#1] SMP NOPTI
> >>>> [    1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
> >>>> [    1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
> >>>> [    1.060230] RIP: 0010:0x0
> >>>> [    1.060478] Code: Bad RIP value.
> >>>> [    1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
> >>>> [    1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
> >>>> [    1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
> >>>> [    1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
> >>>> [    1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
> >>>> [    1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
> >>>> [    1.064609] FS:  0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
> >>>> [    1.065360] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>>> [    1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
> >>>> [    1.066562] Call Trace:
> >>>> [    1.066803]  load_uefi_certs+0xc8/0x2bb
> >>>> [    1.067171]  ? get_cert_list+0xfb/0xfb
> >>>> [    1.067523]  do_one_initcall+0x5d/0x2f0
> >>>> [    1.067894]  ? rcu_read_lock_sched_held+0x52/0x80
> >>>> [    1.068337]  kernel_init_freeable+0x243/0x2c2
> >>>> [    1.068751]  ? rest_init+0x23a/0x23a
> >>>> [    1.069095]  kernel_init+0xa/0x106
> >>>> [    1.069416]  ret_from_fork+0x27/0x50
> >>>> [    1.069759] Modules linked in:
> >>>> [    1.070050] CR2: 0000000000000000
> >>>> [    1.070361] ---[ end trace fcce9bb4feb21d99 ]---
> >>>>
> >>>>
> >>>
> >>> Sorry, wrong mail identified, the patch is actually
> >>>
> >>> commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
> >>> Author: Ard Biesheuvel <ardb@kernel.org>
> >>> Date:   Sun Feb 16 19:46:25 2020 +0100
> >>>
> >>>     integrity: Check properly whether EFI GetVariable() is available
> >>>
> >>>     Testing the value of the efi.get_variable function pointer is not
> >>>
> >>> which made it work. (not even able to find that patch on lkml ...)
> >>
> >> To clarify for Ard, your patch breaks a basic QEMU setup (see above,
> >> NULL pointer dereference). Reverting your patch from linux-next makes it
> >> work again.
> >>
> >
> > Does this fix it?
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 41269a95ff85..d1746a579c99 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -300,12 +300,12 @@ static int __init efisubsys_init(void)
> >  {
> >         int error;
> >
> > -       if (!efi_enabled(EFI_BOOT))
> > -               return 0;
> > -
> >         if (!efi_enabled(EFI_RUNTIME_SERVICES))
> >                 efi.runtime_supported_mask = 0;
> >
> > +       if (!efi_enabled(EFI_BOOT))
> > +               return 0;
> > +
> >         if (efi.runtime_supported_mask) {
> >                 /*
> >                  * Since we process only one efi_runtime_service() at a time, an
> >
>
> Yes, does the trick!
>

Thanks, David. I'll get this out and into -next asap.
David Hildenbrand Feb. 28, 2020, 9:58 a.m. UTC | #9 
On 28.02.20 10:44, Ard Biesheuvel wrote:
> On Fri, 28 Feb 2020 at 10:43, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 28.02.20 10:39, Ard Biesheuvel wrote:
>>> On Fri, 28 Feb 2020 at 10:35, David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>> On 28.02.20 10:31, David Hildenbrand wrote:
>>>>> On 28.02.20 10:19, David Hildenbrand wrote:
>>>>>> On 17.02.20 12:39, Javier Martinez Canillas wrote:
>>>>>>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
>>>>>>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
>>>>>>>
>>>>>>> But it just assumes that the variables will be present and prints an error
>>>>>>> if the certs can't be loaded, even when is possible that the variables may
>>>>>>> not exist. For example the MokListRT variable will only be present if shim
>>>>>>> is used.
>>>>>>>
>>>>>>> So only print an error message about failing to get the certs list from an
>>>>>>> EFI variable if this is found. Otherwise these printed errors just pollute
>>>>>>> the kernel log ring buffer with confusing messages like the following:
>>>>>>>
>>>>>>> [    5.427251] Couldn't get size: 0x800000000000000e
>>>>>>> [    5.427261] MODSIGN: Couldn't get UEFI db list
>>>>>>> [    5.428012] Couldn't get size: 0x800000000000000e
>>>>>>> [    5.428023] Couldn't get UEFI MokListRT
>>>>>>>
>>>>>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>>>>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>
>>>>>> This patch seems to break a very basic x86-64 QEMU setup (booting
>>>>>> upstream kernel with a F31 initrd - are you running basic boot tests?).
>>>>>> Luckily, it only took me 5 minutes to identify this patch. Reverting
>>>>>> this patch from linux-next fixes it for me.
>>>>>>
>>>>>>
>>>>>> [    1.042766] Loaded X.509 cert 'Build time autogenerated kernel key: 6625d6e34255935276d2c9851e2458909a4bcd69'
>>>>>> [    1.044314] zswap: loaded using pool lzo/zbud
>>>>>> [    1.045663] Key type ._fscrypt registered
>>>>>> [    1.046154] Key type .fscrypt registered
>>>>>> [    1.046524] Key type fscrypt-provisioning registered
>>>>>> [    1.051178] Key type big_key registered
>>>>>> [    1.055108] Key type encrypted registered
>>>>>> [    1.055513] BUG: kernel NULL pointer dereference, address: 0000000000000000
>>>>>> [    1.056172] #PF: supervisor instruction fetch in kernel mode
>>>>>> [    1.056706] #PF: error_code(0x0010) - not-present page
>>>>>> [    1.057367] PGD 0 P4D 0
>>>>>> [    1.057729] Oops: 0010 [#1] SMP NOPTI
>>>>>> [    1.058249] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc3-next-20200228+ #79
>>>>>> [    1.059167] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.4
>>>>>> [    1.060230] RIP: 0010:0x0
>>>>>> [    1.060478] Code: Bad RIP value.
>>>>>> [    1.060786] RSP: 0018:ffffbc7880637d98 EFLAGS: 00010246
>>>>>> [    1.061281] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffbc7880637dc8
>>>>>> [    1.061954] RDX: 0000000000000000 RSI: ffffbc7880637df0 RDI: ffffffffa73c40be
>>>>>> [    1.062611] RBP: ffffbc7880637e20 R08: ffffbc7880637dac R09: ffffa0238f4ba6c0
>>>>>> [    1.063278] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
>>>>>> [    1.063956] R13: ffffa024bdd6f660 R14: 0000000000000000 R15: 0000000000000000
>>>>>> [    1.064609] FS:  0000000000000000(0000) GS:ffffa023fdd00000(0000) knlGS:0000000000000000
>>>>>> [    1.065360] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>> [    1.065900] CR2: ffffffffffffffd6 CR3: 00000000b1610000 CR4: 00000000000006e0
>>>>>> [    1.066562] Call Trace:
>>>>>> [    1.066803]  load_uefi_certs+0xc8/0x2bb
>>>>>> [    1.067171]  ? get_cert_list+0xfb/0xfb
>>>>>> [    1.067523]  do_one_initcall+0x5d/0x2f0
>>>>>> [    1.067894]  ? rcu_read_lock_sched_held+0x52/0x80
>>>>>> [    1.068337]  kernel_init_freeable+0x243/0x2c2
>>>>>> [    1.068751]  ? rest_init+0x23a/0x23a
>>>>>> [    1.069095]  kernel_init+0xa/0x106
>>>>>> [    1.069416]  ret_from_fork+0x27/0x50
>>>>>> [    1.069759] Modules linked in:
>>>>>> [    1.070050] CR2: 0000000000000000
>>>>>> [    1.070361] ---[ end trace fcce9bb4feb21d99 ]---
>>>>>>
>>>>>>
>>>>>
>>>>> Sorry, wrong mail identified, the patch is actually
>>>>>
>>>>> commit 6b75d54d5258ccd655387a00bbe1b00f92f4d965
>>>>> Author: Ard Biesheuvel <ardb@kernel.org>
>>>>> Date:   Sun Feb 16 19:46:25 2020 +0100
>>>>>
>>>>>     integrity: Check properly whether EFI GetVariable() is available
>>>>>
>>>>>     Testing the value of the efi.get_variable function pointer is not
>>>>>
>>>>> which made it work. (not even able to find that patch on lkml ...)
>>>>
>>>> To clarify for Ard, your patch breaks a basic QEMU setup (see above,
>>>> NULL pointer dereference). Reverting your patch from linux-next makes it
>>>> work again.
>>>>
>>>
>>> Does this fix it?
>>>
>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>> index 41269a95ff85..d1746a579c99 100644
>>> --- a/drivers/firmware/efi/efi.c
>>> +++ b/drivers/firmware/efi/efi.c
>>> @@ -300,12 +300,12 @@ static int __init efisubsys_init(void)
>>>  {
>>>         int error;
>>>
>>> -       if (!efi_enabled(EFI_BOOT))
>>> -               return 0;
>>> -
>>>         if (!efi_enabled(EFI_RUNTIME_SERVICES))
>>>                 efi.runtime_supported_mask = 0;
>>>
>>> +       if (!efi_enabled(EFI_BOOT))
>>> +               return 0;
>>> +
>>>         if (efi.runtime_supported_mask) {
>>>                 /*
>>>                  * Since we process only one efi_runtime_service() at a time, an
>>>
>>
>> Yes, does the trick!
>>
> 
> Thanks, David. I'll get this out and into -next asap.
> 

Thanks for the very fast fix :)
diff mbox series

Patch

diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 111898aad56..f0c90824196 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -35,16 +35,18 @@  static __init bool uefi_check_ignore_db(void)
  * Get a certificate list blob from the named EFI variable.
  */
 static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
-				  unsigned long *size)
+				  unsigned long *size, efi_status_t *status)
 {
-	efi_status_t status;
 	unsigned long lsize = 4;
 	unsigned long tmpdb[4];
 	void *db;
 
-	status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
-	if (status != EFI_BUFFER_TOO_SMALL) {
-		pr_err("Couldn't get size: 0x%lx\n", status);
+	*status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
+	if (*status == EFI_NOT_FOUND)
+		return NULL;
+
+	if (*status != EFI_BUFFER_TOO_SMALL) {
+		pr_err("Couldn't get size: 0x%lx\n", *status);
 		return NULL;
 	}
 
@@ -52,10 +54,10 @@  static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
 	if (!db)
 		return NULL;
 
-	status = efi.get_variable(name, guid, NULL, &lsize, db);
-	if (status != EFI_SUCCESS) {
+	*status = efi.get_variable(name, guid, NULL, &lsize, db);
+	if (*status != EFI_SUCCESS) {
 		kfree(db);
-		pr_err("Error reading db var: 0x%lx\n", status);
+		pr_err("Error reading db var: 0x%lx\n", *status);
 		return NULL;
 	}
 
@@ -74,6 +76,7 @@  static int __init load_uefi_certs(void)
 	efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
 	void *db = NULL, *dbx = NULL, *mok = NULL;
 	unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
+	efi_status_t status;
 	int rc = 0;
 
 	if (!efi.get_variable)
@@ -83,9 +86,12 @@  static int __init load_uefi_certs(void)
 	 * an error if we can't get them.
 	 */
 	if (!uefi_check_ignore_db()) {
-		db = get_cert_list(L"db", &secure_var, &dbsize);
+		db = get_cert_list(L"db", &secure_var, &dbsize, &status);
 		if (!db) {
-			pr_err("MODSIGN: Couldn't get UEFI db list\n");
+			if (status == EFI_NOT_FOUND)
+				pr_debug("MODSIGN: db variable wasn't found\n");
+			else
+				pr_err("MODSIGN: Couldn't get UEFI db list\n");
 		} else {
 			rc = parse_efi_signature_list("UEFI:db",
 					db, dbsize, get_handler_for_db);
@@ -96,9 +102,12 @@  static int __init load_uefi_certs(void)
 		}
 	}
 
-	mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
+	mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
 	if (!mok) {
-		pr_info("Couldn't get UEFI MokListRT\n");
+		if (status == EFI_NOT_FOUND)
+			pr_debug("MokListRT variable wasn't found\n");
+		else
+			pr_info("Couldn't get UEFI MokListRT\n");
 	} else {
 		rc = parse_efi_signature_list("UEFI:MokListRT",
 					      mok, moksize, get_handler_for_db);
@@ -107,9 +116,12 @@  static int __init load_uefi_certs(void)
 		kfree(mok);
 	}
 
-	dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
+	dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, &status);
 	if (!dbx) {
-		pr_info("Couldn't get UEFI dbx list\n");
+		if (status == EFI_NOT_FOUND)
+			pr_debug("dbx variable wasn't found\n");
+		else
+			pr_info("Couldn't get UEFI dbx list\n");
 	} else {
 		rc = parse_efi_signature_list("UEFI:dbx",
 					      dbx, dbxsize,