Re: [PATCH V2 2/3] integrity: Move import of MokListRT certs to a separate routine

[Lenny Szubowicz <lszubowi@xxxxxxxxxx>]

On 9/11/20 11:02 AM, Ard Biesheuvel wrote:
> On Sat, 5 Sep 2020 at 04:31, Lenny Szubowicz <lszubowi@xxxxxxxxxx> wrote:
> > Move the loading of certs from the UEFI MokListRT into a separate
> > routine to facilitate additional MokList functionality.
> >
> > There is no visible functional change as a result of this patch.
> > Although the UEFI dbx certs are now loaded before the MokList certs,
> > they are loaded onto different key rings. So the order of the keys
> > on their respective key rings is the same.
> >
> > Signed-off-by: Lenny Szubowicz <lszubowi@xxxxxxxxxx>
>
> Why did you drop Mimi's reviewed-by from this patch?

It was not intentional. I was just not aware that I needed to propagate
Mimi Zohar's reviewed-by from V1 of the patch to V2.

Reviewed-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>

V2 includes changes in that patch to incorporate suggestions from
Andy Shevchenko. My assumption was that the maintainer would
gather up the reviewed-by and add any signed-off-by as appropriate,
but it sounds like my assumption was incorrect. In retrospect, I
could see that having the maintainer dig through prior versions
of a patch set for prior reviewed-by tags could be burdensome.

Advice on the expected handling of this would be appreciated.

                    -Lenny.

> > ---
> >   security/integrity/platform_certs/load_uefi.c | 63 +++++++++++++------
> >   1 file changed, 44 insertions(+), 19 deletions(-)
> >
> > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > index 253fb9a7fc98..c1c622b4dc78 100644
> > --- a/security/integrity/platform_certs/load_uefi.c
> > +++ b/security/integrity/platform_certs/load_uefi.c
> > @@ -66,6 +66,43 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> >   }
> >
> >   /*
> > + * load_moklist_certs() - Load MokList certs
> > + *
> > + * Load the certs contained in the UEFI MokListRT database into the
> > + * platform trusted keyring.
> > + *
> > + * Return:     Status
> > + */
> > +static int __init load_moklist_certs(void)
> > +{
> > +       efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
> > +       void *mok;
> > +       unsigned long moksize;
> > +       efi_status_t status;
> > +       int rc;
> > +
> > +       /* Get MokListRT. It might not exist, so it isn't an error
> > +        * if we can't get it.
> > +        */
> > +       mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
> > +       if (mok) {
> > +               rc = parse_efi_signature_list("UEFI:MokListRT",
> > +                                             mok, moksize, get_handler_for_db);
> > +               kfree(mok);
> > +               if (rc)
> > +                       pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
> > +               return rc;
> > +       }
> > +       if (status == EFI_NOT_FOUND)
> > +               pr_debug("MokListRT variable wasn't found\n");
> > +       else
> > +               pr_info("Couldn't get UEFI MokListRT\n");
> > +       return 0;
> > +}
> > +
> > +/*
> > + * load_uefi_certs() - Load certs from UEFI sources
> > + *
> >    * Load the certs contained in the UEFI databases into the platform trusted
> >    * keyring and the UEFI blacklisted X.509 cert SHA256 hashes into the blacklist
> >    * keyring.
> > @@ -73,17 +110,16 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> >   static int __init load_uefi_certs(void)
> >   {
> >          efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
> > -       efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
> > -       void *db = NULL, *dbx = NULL, *mok = NULL;
> > -       unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
> > +       void *db = NULL, *dbx = NULL;
> > +       unsigned long dbsize = 0, dbxsize = 0;
> >          efi_status_t status;
> >          int rc = 0;
> >
> >          if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
> >                  return false;
> >
> > -       /* Get db, MokListRT, and dbx.  They might not exist, so it isn't
> > -        * an error if we can't get them.
> > +       /* Get db and dbx.  They might not exist, so it isn't an error
> > +        * if we can't get them.
> >           */
> >          if (!uefi_check_ignore_db()) {
> >                  db = get_cert_list(L"db", &secure_var, &dbsize, &status);
> > @@ -102,20 +138,6 @@ static int __init load_uefi_certs(void)
> >                  }
> >          }
> >
> > -       mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
> > -       if (!mok) {
> > -               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);
> > -               if (rc)
> > -                       pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
> > -               kfree(mok);
> > -       }
> > -
> >          dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, &status);
> >          if (!dbx) {
> >                  if (status == EFI_NOT_FOUND)
> > @@ -131,6 +153,9 @@ static int __init load_uefi_certs(void)
> >                  kfree(dbx);
> >          }
> >
> > +       /* Load the MokListRT certs */
> > +       rc = load_moklist_certs();
> > +
> >          return rc;
> >   }
> >   late_initcall(load_uefi_certs);
> > --
> > 2.27.0