[UPSTREAM][RFC PATCH] integrity: Load mokx certs from the EFI MOK config table

[Dimitri John Ledkov]

On Mon, May 10, 2021 at 3:33 PM Krzysztof Kozlowski
<krzysztof.kozlowski at canonical.com> wrote:
>
> On 10/05/2021 10:14, Dimitri John Ledkov wrote:
> > Refactor load_moklist_certs() to load either MokListRT into db, or
> > MokListXRT into dbx. Call load_moklist_certs() twice - first to load
> > mokx certs into dbx, then mok certs into db.
> >
> > This thus now attempts to load mokx certs via the EFI MOKvar config
> > table first, and if that fails, via the EFI variable. Previously mokx
> > certs were only loaded via the EFI variable. Which fails when
> > MokListXRT is large and instead of MokListXRT is only available as
> > MokListXRT{1,2,3}. This is the case with Ubuntu's 15.4 based
> > shim. This patch is required to address CVE-2020-26541 when
> > certificates are revoked via MokListXRT.
> >
> > Fixes: ebd9c2ae369a45bdd9f8615484db09be58fc242b
> >
> > Signed-off-by: Dimitri John Ledkov <dimitri.ledkov at canonical.com>
> > ---
> >  @Ubuntu kernel-team / security-team
> >
> >  Presubmitting this patch internally, to see if it's ok to submit
> >  upstream. Intention is to send this to keyrings@
> >  linux-security-module@ linux-kernel@ Eric Snowberg etc that have
> >  introduced the referenced feature into v5.13-rc1 which on ubuntu is
> >  buggy and doesn't address the CVE in question.
> >
> >  security/integrity/platform_certs/load_uefi.c | 73 ++++++++++---------
> >  1 file changed, 39 insertions(+), 34 deletions(-)
> >
> > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > index f290f78c3f301..e1f4ebb71abe0 100644
> > --- a/security/integrity/platform_certs/load_uefi.c
> > +++ b/security/integrity/platform_certs/load_uefi.c
> > @@ -66,17 +66,18 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> >  }
> >
> >  /*
> > - * load_moklist_certs() - Load MokList certs
> > + * load_moklist_certs() - Load Mok(X)List certs
> > + * @load_db: Load MokListRT into db when true; MokListXRT into dbx when false
> >   *
> > - * Load the certs contained in the UEFI MokListRT database into the
> > - * platform trusted keyring.
> > + * Load the certs contained in the UEFI MokList(X)RT database into the
> > + * platform trusted/denied keyring.
> >   *
> >   * This routine checks the EFI MOK config table first. If and only if
> > - * that fails, this routine uses the MokListRT ordinary UEFI variable.
> > + * that fails, this routine uses the MokList(X)RT ordinary UEFI variable.
> >   *
> >   * Return:   Status
> >   */
> > -static int __init load_moklist_certs(void)
> > +static int __init load_moklist_certs(const bool load_db)
>
> What's the point of having "const" value argument? It's a noop, I think,
> and confusing one.
>

In my mind, it means "the function will not arbitrarily decide to
override the caller's decision as to which certs to load". And I see
in other places in the kernel that static functions have const bool
arguments. E.g. see the charming functions in
arch/x86/net/bpf_jit_comp32.c

But yeah, I can drop that, if it really is confusing.

> >  {
> >       struct efi_mokvar_table_entry *mokvar_entry;
> >       efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
> > @@ -84,41 +85,54 @@ static int __init load_moklist_certs(void)
> >       unsigned long moksize;
> >       efi_status_t status;
> >       int rc;
> > +     char *mokvar_name = "MokListRT";
> > +     efi_char16_t *efivar_name = L"MokListRT";
> > +     char *parse_mokvar_name = "UEFI:MokListRT (MOKvar table)";
> > +     char *parse_efivar_name = "UEFI:MokListRT";
>
> OTOH, this should be pointer to const. Here it is important because code
> should not modify the contents.
>

Agree, will fix.

> Best regards,
> Krzysztof

-- 
Regards,

Dimitri.