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

From: Serge E. Hallyn
Date: Tue May 18 2021 - 10:20:01 EST

Next message: Rob Herring: "Re: [PATCH v5 1/3] dt-bindings: soc: tegra-pmc: Document core power domain"
Previous message: Dafna Hirschfeld: "[PATCH v6 RESEND 2/2] drm/bridge: Add ChromeOS EC ANX7688 bridge driver support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, May 12, 2021 at 04:31:00PM +0100, 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. Instead of large MokListXRT variable, only
> MokListXRT{1,2,3} are available which are not loaded. 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.

Hi Dimitri,

I don't see any problems with this technically, and based on
https://bugs.launchpad.net/bugs/1928679 it looks like it is in fact needed.

I suggest a change below, but I'm not sure you should make the change
without confirmation from one of the maintainers that it matches what
they would want.

> Fixes: ebd9c2ae369a ("integrity: Load mokx variables into the blacklist keyring")
>
> Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@xxxxxxxxxxxxx>
> cc: keyrings@xxxxxxxxxxxxxxx
> cc: Eric Snowberg <eric.snowberg@xxxxxxxxxx>
> cc: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> cc: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> cc: David Howells <dhowells@xxxxxxxxxx>

Acked-by: Serge Hallyn <serge@xxxxxxxxxx>

> ---
> security/integrity/platform_certs/load_uefi.c | 74 ++++++++++---------
> 1 file changed, 40 insertions(+), 34 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> index f290f78c3f30..f4b913ec76e4 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)

Rather than passing in a bool here, I would suggest, above this function,
doing something like:

struct moklist_desc {
const char *mokvar_name;
efi_char16_t *efivar_name;
const char *parse_mokvar_name;
const char *parse_efivar_name;
efi_element_handler_t (*get_handler_for_guid)(const efi_guid_t *);
};

struct moklist_desc moklist_descriptor = {
.mokvar_name = "MokListRT",
.efivar_name = L"MokListRT",
.parse_mokvar_name = "UEFI:MokListRT (MOKvar table)",
.parse_efivar_name = "UEFI:MokListRT",
.get_handler_for_guid = get_handler_for_db,
};

struct moklist_desc moxklist_descriptor = {
.mokvar_name = "MokListXRT",
.efivar_name = L"MokListXRT",
.parse_mokvar_name = "UEFI:MokListXRT (MOKvar table)",
.parse_efivar_name = "UEFI:MokListXRT",
.get_handler_for_guid = get_handler_for_dbx,
};

And pass each of those into load_moklist_certs() once.

> {
> struct efi_mokvar_table_entry *mokvar_entry;
> efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
> @@ -84,41 +85,55 @@ static int __init load_moklist_certs(void)
> unsigned long moksize;
> efi_status_t status;
> int rc;
> + const char *mokvar_name = "MokListRT";
> + /* Should be const, but get_cert_list() doesn't have it as const yet */
> + efi_char16_t *efivar_name = L"MokListRT";
> + const char *parse_mokvar_name = "UEFI:MokListRT (MOKvar table)";
> + const char *parse_efivar_name = "UEFI:MokListRT";
> + efi_element_handler_t (*get_handler_for_guid)(const efi_guid_t *) = get_handler_for_db;
> +
> + if (!load_db) {
> + mokvar_name = "MokListXRT";
> + efivar_name = L"MokListXRT";
> + parse_mokvar_name = "UEFI:MokListXRT (MOKvar table)";
> + parse_efivar_name = "UEFI:MokListXRT";
> + get_handler_for_guid = get_handler_for_dbx;
> + }
>
> /* First try to load certs from the EFI MOKvar config table.
> * It's not an error if the MOKvar config table doesn't exist
> * or the MokListRT entry is not found in it.
> */
> - mokvar_entry = efi_mokvar_entry_find("MokListRT");
> + mokvar_entry = efi_mokvar_entry_find(mokvar_name);
> if (mokvar_entry) {
> - rc = parse_efi_signature_list("UEFI:MokListRT (MOKvar table)",
> + rc = parse_efi_signature_list(parse_mokvar_name,
> mokvar_entry->data,
> mokvar_entry->data_size,
> - get_handler_for_db);
> + get_handler_for_guid);
> /* All done if that worked. */
> if (!rc)
> return rc;
>
> - pr_err("Couldn't parse MokListRT signatures from EFI MOKvar config table: %d\n",
> - rc);
> + pr_err("Couldn't parse %s signatures from EFI MOKvar config table: %d\n",
> + mokvar_name, 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);
> + mok = get_cert_list(efivar_name, &mok_var, &moksize, &status);
> if (mok) {
> - rc = parse_efi_signature_list("UEFI:MokListRT",
> - mok, moksize, get_handler_for_db);
> + rc = parse_efi_signature_list(parse_efivar_name,
> + mok, moksize, get_handler_for_guid);
> kfree(mok);
> if (rc)
> - pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
> + pr_err("Couldn't parse %s signatures: %d\n", mokvar_name, rc);
> return rc;
> }
> if (status == EFI_NOT_FOUND)
> - pr_debug("MokListRT variable wasn't found\n");
> + pr_debug("%s variable wasn't found\n", mokvar_name);
> else
> - pr_info("Couldn't get UEFI MokListRT\n");
> + pr_info("Couldn't get UEFI %s\n", mokvar_name);
> return 0;
> }
>
> @@ -132,9 +147,8 @@ static int __init load_moklist_certs(void)
> 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, *mokx = NULL;
> - unsigned long dbsize = 0, dbxsize = 0, mokxsize = 0;
> + void *db = NULL, *dbx = NULL;
> + unsigned long dbsize = 0, dbxsize = 0;
> efi_status_t status;
> int rc = 0;
>
> @@ -176,23 +190,15 @@ static int __init load_uefi_certs(void)
> kfree(dbx);
> }
>
> - mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize, &status);
> - if (!mokx) {
> - if (status == EFI_NOT_FOUND)
> - pr_debug("mokx variable wasn't found\n");
> - else
> - pr_info("Couldn't get mokx list\n");
> - } else {
> - rc = parse_efi_signature_list("UEFI:MokListXRT",
> - mokx, mokxsize,
> - get_handler_for_dbx);
> - if (rc)
> - pr_err("Couldn't parse mokx signatures %d\n", rc);
> - kfree(mokx);
> - }
> + /* Load the MokListXRT certs */
> + rc = load_moklist_certs(false);
> + if (rc)
> + pr_err("Couldn't parse mokx signatures: %d\n", rc);
>
> /* Load the MokListRT certs */
> - rc = load_moklist_certs();
> + rc = load_moklist_certs(true);
> + if (rc)
> + pr_err("Couldn't parse mok signatures: %d\n", rc);
>
> return rc;
> }
> --
> 2.27.0
>

Next message: Rob Herring: "Re: [PATCH v5 1/3] dt-bindings: soc: tegra-pmc: Document core power domain"
Previous message: Dafna Hirschfeld: "[PATCH v6 RESEND 2/2] drm/bridge: Add ChromeOS EC ANX7688 bridge driver support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]