| Message ID | 20210510141448.45341-1-dimitri.ledkov@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [UPSTREAM,RFC] integrity: Load mokx certs from the EFI MOK config table | expand |
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@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. > { > 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. Best regards, Krzysztof
On Mon, May 10, 2021 at 3:33 PM Krzysztof Kozlowski <krzysztof.kozlowski@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@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
On 10/05/2021 10:48, Dimitri John Ledkov wrote: > On Mon, May 10, 2021 at 3:33 PM Krzysztof Kozlowski > <krzysztof.kozlowski@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@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. I understand now, you gave a meaning to the "const". To me it looks unusual and I would prefer the proper documentation, but such patter indeed appears in few places, so I don't mind keeping it. Best regards, Krzysztof
Hi Dmitri, very nice idea of using the list as RFC for upstream! I have a small suggestion inline below, regarding the commit message. Cheers, Guilherme On Mon, May 10, 2021 at 11:15 AM Dimitri John Ledkov <dimitri.ledkov@canonical.com> 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 The fixes tag is set usually with a 12-char SHA + commit name. In your case, it'd be: Fixes: ebd9c2ae369a ("integrity: Load mokx variables into the blacklist keyring")
On 10/05/2021 11:00, Guilherme Piccoli wrote: > Hi Dmitri, very nice idea of using the list as RFC for upstream! > I have a small suggestion inline below, regarding the commit message. > Cheers, > > > Guilherme > > On Mon, May 10, 2021 at 11:15 AM Dimitri John Ledkov > <dimitri.ledkov@canonical.com> 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 > > The fixes tag is set usually with a 12-char SHA + commit name. In your > case, it'd be: > Fixes: ebd9c2ae369a ("integrity: Load mokx variables into the > blacklist keyring") Good catch. You just need to run scripts/checkpatch 0001-* Best regards, Krzysztof
On Mon, May 10, 2021 at 4:04 PM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 10/05/2021 11:00, Guilherme Piccoli wrote: > > Hi Dmitri, very nice idea of using the list as RFC for upstream! > > I have a small suggestion inline below, regarding the commit message. > > Cheers, > > > > > > Guilherme > > > > On Mon, May 10, 2021 at 11:15 AM Dimitri John Ledkov > > <dimitri.ledkov@canonical.com> 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 > > > > The fixes tag is set usually with a 12-char SHA + commit name. In your > > case, it'd be: > > Fixes: ebd9c2ae369a ("integrity: Load mokx variables into the > > blacklist keyring") > > > Good catch. You just need to run scripts/checkpatch 0001-* > And yet.... $ ./scripts/checkpatch.pl 0001-integrity-Load-mokx-certs-from-the-EFI-MOK-config-ta.patch total: 0 errors, 0 warnings, 129 lines checked 0001-integrity-Load-mokx-certs-from-the-EFI-MOK-config-ta.patch has no obvious style problems and is ready for submission. So I don't know how my commit message is not tripping up that check that clearly is there in checkpatch =/ Thanks for this, will fix.
On 10/05/2021 11:13, Dimitri John Ledkov wrote: > On Mon, May 10, 2021 at 4:04 PM Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: >> >> On 10/05/2021 11:00, Guilherme Piccoli wrote: >>> Hi Dmitri, very nice idea of using the list as RFC for upstream! >>> I have a small suggestion inline below, regarding the commit message. >>> Cheers, >>> >>> >>> Guilherme >>> >>> On Mon, May 10, 2021 at 11:15 AM Dimitri John Ledkov >>> <dimitri.ledkov@canonical.com> 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 >>> >>> The fixes tag is set usually with a 12-char SHA + commit name. In your >>> case, it'd be: >>> Fixes: ebd9c2ae369a ("integrity: Load mokx variables into the >>> blacklist keyring") >> >> >> Good catch. You just need to run scripts/checkpatch 0001-* >> > > And yet.... > > $ ./scripts/checkpatch.pl > 0001-integrity-Load-mokx-certs-from-the-EFI-MOK-config-ta.patch > total: 0 errors, 0 warnings, 129 lines checked > > 0001-integrity-Load-mokx-certs-from-the-EFI-MOK-config-ta.patch has no > obvious style problems and is ready for submission. > > So I don't know how my commit message is not tripping up that check > that clearly is there in checkpatch =/ > > Thanks for this, will fix. I had impression it checks for it, but it turns out it looks only for commit IDs mentioned in the message. If it only was not written in Perl... :) https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L127 Best regards, Krzysztof
On Mon, May 10, 2021 at 4:24 PM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > > On 10/05/2021 11:13, Dimitri John Ledkov wrote: > > On Mon, May 10, 2021 at 4:04 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@canonical.com> wrote: > >> > >> On 10/05/2021 11:00, Guilherme Piccoli wrote: > >>> Hi Dmitri, very nice idea of using the list as RFC for upstream! > >>> I have a small suggestion inline below, regarding the commit message. > >>> Cheers, > >>> > >>> > >>> Guilherme > >>> > >>> On Mon, May 10, 2021 at 11:15 AM Dimitri John Ledkov > >>> <dimitri.ledkov@canonical.com> 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 > >>> > >>> The fixes tag is set usually with a 12-char SHA + commit name. In your > >>> case, it'd be: > >>> Fixes: ebd9c2ae369a ("integrity: Load mokx variables into the > >>> blacklist keyring") > >> > >> > >> Good catch. You just need to run scripts/checkpatch 0001-* > >> > > > > And yet.... > > > > $ ./scripts/checkpatch.pl > > 0001-integrity-Load-mokx-certs-from-the-EFI-MOK-config-ta.patch > > total: 0 errors, 0 warnings, 129 lines checked > > > > 0001-integrity-Load-mokx-certs-from-the-EFI-MOK-config-ta.patch has no > > obvious style problems and is ready for submission. > > > > So I don't know how my commit message is not tripping up that check > > that clearly is there in checkpatch =/ > > > > Thanks for this, will fix. > > I had impression it checks for it, but it turns out it looks only for > commit IDs mentioned in the message. If it only was not written in > Perl... :) > > https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L127 > I need to _modify_ my git config?! the tool written by kernel engineers is not suitable for kernel engineering by default?! Maybe I should dput git into Ubuntu such that everyones's /etc/gitconfig is suitable for kernel development ?! =))))) Adding to my todo to do that in git upstream by default because it is silly to require everyone to modify their gitconfig for this.
On 10/05/2021 11:28, Dimitri John Ledkov wrote: >> >> I had impression it checks for it, but it turns out it looks only for >> commit IDs mentioned in the message. If it only was not written in >> Perl... :) >> >> https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L127 >> > > I need to _modify_ my git config?! the tool written by kernel > engineers is not suitable for kernel engineering by default?! Maybe I > should dput git into Ubuntu such that everyones's /etc/gitconfig is > suitable for kernel development ?! =))))) Adding to my todo to do that > in git upstream by default because it is silly to require everyone to > modify their gitconfig for this. The tool is suitable, we just don't like the default configuration. :) The configuration is per-repository, so you don't need to adjust entire system. There are not that many Git projects, except Linux, which have so many commits thus can hit duplicated SHA. Best regards, Krzysztof
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) { 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"; + 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 +146,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 +189,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; }
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@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(-)