Re: [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
On Fri, May 03, 2019 at 04:23:51PM +0200, Ard Biesheuvel wrote:
> On Fri, 3 May 2019 at 10:59, joeyli wrote:
> >
> > On Fri, May 03, 2019 at 10:07:59AM +0200, Ard Biesheuvel wrote:
> > > On Fri, 3 May 2019 at 09:18, joeyli wrote:
> > > >
> > > > Hi Ard,
> > > >
> > > > On Thu, May 02, 2019 at 11:04:34AM +0200, Ard Biesheuvel wrote:
> > > > > On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi
> > > > > wrote:
> > > > > >
> > > > > > When loading certificates list from UEFI variable, the original
> > > > > > error
> > > > > > message direct shows the efi status code from UEFI firmware. It
> > > > > > looks
> > > > > > ugly:
> > > > > >
> > > > > > [2.335031] Couldn't get size: 0x800e
> > > > > > [2.335032] Couldn't get UEFI MokListRT
> > > > > > [2.339985] Couldn't get size: 0x800e
> > > > > > [2.339987] Couldn't get UEFI dbx list
> > > > > >
> > > > > > So, this patch shows the status string instead of status code.
> > > > > >
> > > > > > On the other hand, the "Couldn't get UEFI" message doesn't need
> > > > > > to be exposed when db/dbx/mok variable do not exist. So, this
> > > > > > patch set the message level to debug.
> > > > > >
> > > > > > v3.
> > > > > > - Print messages similar to db/mok when loading dbx hash to
> > > > > > blacklist:
> > > > > > [1.500952] EFI: Blacklisting hash of an executable: UEFI:dbx
> > > > > > [1.501773] blacklist: Loaded blacklisting hash
> > > > > > 'bin:80b4d96931bf0d02fd91a61e19d14f1da452e66db2408ca8604d411f92659f0a'
> > > > > >
> > > > > > - Setting messages for the existence of db/mok/dbx lists to debug
> > > > > > level.
> > > > > >
> > > > > > v2.
> > > > > > Setting the MODSIGN messages level to debug.
> > > > > >
> > > > > > Link:
> > > > > > https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516
> > > > > > Cc: James Morris
> > > > > > Cc: Serge E. Hallyn"
> > > > > > Cc: David Howells
> > > > > > Cc: Nayna Jain
> > > > > > Cc: Josh Boyer
> > > > > > Cc: Mimi Zohar
> > > > > > Signed-off-by: "Lee, Chun-Yi"
> > > > > > ---
> > > > > > certs/blacklist.c | 3 +-
> > > > > > security/integrity/platform_certs/load_uefi.c | 40
> > > > > > +++
> > > > > > 2 files changed, 31 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > > > > index 3a507b9e2568..f91437e39e44 100644
> > > > > > --- a/certs/blacklist.c
> > > > > > +++ b/certs/blacklist.c
> > > > > > @@ -100,7 +100,8 @@ int mark_hash_blacklisted(const char *hash)
> > > > > > if (IS_ERR(key)) {
> > > > > > pr_err("Problem blacklisting hash (%ld)\n",
> > > > > > PTR_ERR(key));
> > > > > > return PTR_ERR(key);
> > > > > > - }
> > > > > > + } else
> > > > > > + pr_notice("Loaded blacklisting hash '%s'\n", hash);
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > diff --git a/security/integrity/platform_certs/load_uefi.c
> > > > > > b/security/integrity/platform_certs/load_uefi.c
> > > > > > index 81b19c52832b..6b6996e5bc27 100644
> > > > > > --- a/security/integrity/platform_certs/load_uefi.c
> > > > > > +++ b/security/integrity/platform_certs/load_uefi.c
> > > > > > @@ -1,5 +1,7 @@
> > > > > > // SPDX-License-Identifier: GPL-2.0
> > > > > >
> > > > > > +#define pr_fmt(fmt) "EFI: "fmt
> > > > > > +
> > > > > > #include
> > > > > > #include
> > > > > > #include
> > > > > > @@ -35,6 +37,18 @@ static __init bool uefi_check_ignore_db(void)
> > > > > > return status == EFI_SUCCESS;
> > > > > > }
> > > > > >
> > > > > > +static void str16_to_str(efi_char16_t *str16, char *str, int
> > > > > > str_size)
> > > > > > +{
> > > > > > + int i = 0;
> > > > > > +
> > > > > > + while (str16[i] != '\0' && i < (str_size - 1)) {
> > > > > > + str[i] = str16[i];
> > > > > > + i++;
> > > > > > + }
> > > > > > +
> > > > > > + str[i] = '\0';
> > > > > > +}
> > > > > > +
> > > > > > /*
> > > > > > * Get a certificate list blob from the named EFI variable.
> > > > > > */
> > > > > > @@ -44,13 +58,20 @@ static __init void *get_cert_list(efi_char16_t
> > > > > > *name, efi_guid_t *guid,
> > > > > > efi_status_t status;
> > > > > > unsigned long lsize = 4;
> > > > > > unsigned long tmpdb[4];
> > > > > > + char namestr[16];
> > > > > > void *db;
> > > > > >
> > > > > > + str16_to_str(name, namestr, ARRAY_SIZE(namestr));
> > > > >
> > > > > Please drop this (and the function above) - instead, just return NULL
> > > > > if the variable is not found (without reporting an error).
> > > > >
> > > >
> > > > This name string is for printing debug level message, not error message.
> > > > This function already returns NULL when EFI_NOT_FOUND be returned by
> > > > firmware.
> > > >
> > > > > > status = efi.get_variable(name, guid,
Re: [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
On Fri, 3 May 2019 at 10:59, joeyli wrote:
>
> On Fri, May 03, 2019 at 10:07:59AM +0200, Ard Biesheuvel wrote:
> > On Fri, 3 May 2019 at 09:18, joeyli wrote:
> > >
> > > Hi Ard,
> > >
> > > On Thu, May 02, 2019 at 11:04:34AM +0200, Ard Biesheuvel wrote:
> > > > On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi
> > > > wrote:
> > > > >
> > > > > When loading certificates list from UEFI variable, the original error
> > > > > message direct shows the efi status code from UEFI firmware. It looks
> > > > > ugly:
> > > > >
> > > > > [2.335031] Couldn't get size: 0x800e
> > > > > [2.335032] Couldn't get UEFI MokListRT
> > > > > [2.339985] Couldn't get size: 0x800e
> > > > > [2.339987] Couldn't get UEFI dbx list
> > > > >
> > > > > So, this patch shows the status string instead of status code.
> > > > >
> > > > > On the other hand, the "Couldn't get UEFI" message doesn't need
> > > > > to be exposed when db/dbx/mok variable do not exist. So, this
> > > > > patch set the message level to debug.
> > > > >
> > > > > v3.
> > > > > - Print messages similar to db/mok when loading dbx hash to blacklist:
> > > > > [1.500952] EFI: Blacklisting hash of an executable: UEFI:dbx
> > > > > [1.501773] blacklist: Loaded blacklisting hash
> > > > > 'bin:80b4d96931bf0d02fd91a61e19d14f1da452e66db2408ca8604d411f92659f0a'
> > > > >
> > > > > - Setting messages for the existence of db/mok/dbx lists to debug
> > > > > level.
> > > > >
> > > > > v2.
> > > > > Setting the MODSIGN messages level to debug.
> > > > >
> > > > > Link:
> > > > > https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516
> > > > > Cc: James Morris
> > > > > Cc: Serge E. Hallyn"
> > > > > Cc: David Howells
> > > > > Cc: Nayna Jain
> > > > > Cc: Josh Boyer
> > > > > Cc: Mimi Zohar
> > > > > Signed-off-by: "Lee, Chun-Yi"
> > > > > ---
> > > > > certs/blacklist.c | 3 +-
> > > > > security/integrity/platform_certs/load_uefi.c | 40
> > > > > +++
> > > > > 2 files changed, 31 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > > > index 3a507b9e2568..f91437e39e44 100644
> > > > > --- a/certs/blacklist.c
> > > > > +++ b/certs/blacklist.c
> > > > > @@ -100,7 +100,8 @@ int mark_hash_blacklisted(const char *hash)
> > > > > if (IS_ERR(key)) {
> > > > > pr_err("Problem blacklisting hash (%ld)\n",
> > > > > PTR_ERR(key));
> > > > > return PTR_ERR(key);
> > > > > - }
> > > > > + } else
> > > > > + pr_notice("Loaded blacklisting hash '%s'\n", hash);
> > > > > return 0;
> > > > > }
> > > > >
> > > > > diff --git a/security/integrity/platform_certs/load_uefi.c
> > > > > b/security/integrity/platform_certs/load_uefi.c
> > > > > index 81b19c52832b..6b6996e5bc27 100644
> > > > > --- a/security/integrity/platform_certs/load_uefi.c
> > > > > +++ b/security/integrity/platform_certs/load_uefi.c
> > > > > @@ -1,5 +1,7 @@
> > > > > // SPDX-License-Identifier: GPL-2.0
> > > > >
> > > > > +#define pr_fmt(fmt) "EFI: "fmt
> > > > > +
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > @@ -35,6 +37,18 @@ static __init bool uefi_check_ignore_db(void)
> > > > > return status == EFI_SUCCESS;
> > > > > }
> > > > >
> > > > > +static void str16_to_str(efi_char16_t *str16, char *str, int
> > > > > str_size)
> > > > > +{
> > > > > + int i = 0;
> > > > > +
> > > > > + while (str16[i] != '\0' && i < (str_size - 1)) {
> > > > > + str[i] = str16[i];
> > > > > + i++;
> > > > > + }
> > > > > +
> > > > > + str[i] = '\0';
> > > > > +}
> > > > > +
> > > > > /*
> > > > > * Get a certificate list blob from the named EFI variable.
> > > > > */
> > > > > @@ -44,13 +58,20 @@ static __init void *get_cert_list(efi_char16_t
> > > > > *name, efi_guid_t *guid,
> > > > > efi_status_t status;
> > > > > unsigned long lsize = 4;
> > > > > unsigned long tmpdb[4];
> > > > > + char namestr[16];
> > > > > void *db;
> > > > >
> > > > > + str16_to_str(name, namestr, ARRAY_SIZE(namestr));
> > > >
> > > > Please drop this (and the function above) - instead, just return NULL
> > > > if the variable is not found (without reporting an error).
> > > >
> > >
> > > This name string is for printing debug level message, not error message.
> > > This function already returns NULL when EFI_NOT_FOUND be returned by
> > > firmware.
> > >
> > > > > 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);
> > > > > + if (status == EFI_NOT_FOUND)
> > > > > + pr_debug("UEFI %s list doesn't exist\n",
> > > > > namestr);
> > > > > + else
> > > > >
Re: [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
On Fri, May 03, 2019 at 10:07:59AM +0200, Ard Biesheuvel wrote:
> On Fri, 3 May 2019 at 09:18, joeyli wrote:
> >
> > Hi Ard,
> >
> > On Thu, May 02, 2019 at 11:04:34AM +0200, Ard Biesheuvel wrote:
> > > On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi wrote:
> > > >
> > > > When loading certificates list from UEFI variable, the original error
> > > > message direct shows the efi status code from UEFI firmware. It looks
> > > > ugly:
> > > >
> > > > [2.335031] Couldn't get size: 0x800e
> > > > [2.335032] Couldn't get UEFI MokListRT
> > > > [2.339985] Couldn't get size: 0x800e
> > > > [2.339987] Couldn't get UEFI dbx list
> > > >
> > > > So, this patch shows the status string instead of status code.
> > > >
> > > > On the other hand, the "Couldn't get UEFI" message doesn't need
> > > > to be exposed when db/dbx/mok variable do not exist. So, this
> > > > patch set the message level to debug.
> > > >
> > > > v3.
> > > > - Print messages similar to db/mok when loading dbx hash to blacklist:
> > > > [1.500952] EFI: Blacklisting hash of an executable: UEFI:dbx
> > > > [1.501773] blacklist: Loaded blacklisting hash
> > > > 'bin:80b4d96931bf0d02fd91a61e19d14f1da452e66db2408ca8604d411f92659f0a'
> > > >
> > > > - Setting messages for the existence of db/mok/dbx lists to debug level.
> > > >
> > > > v2.
> > > > Setting the MODSIGN messages level to debug.
> > > >
> > > > Link:
> > > > https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516
> > > > Cc: James Morris
> > > > Cc: Serge E. Hallyn"
> > > > Cc: David Howells
> > > > Cc: Nayna Jain
> > > > Cc: Josh Boyer
> > > > Cc: Mimi Zohar
> > > > Signed-off-by: "Lee, Chun-Yi"
> > > > ---
> > > > certs/blacklist.c | 3 +-
> > > > security/integrity/platform_certs/load_uefi.c | 40
> > > > +++
> > > > 2 files changed, 31 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > > index 3a507b9e2568..f91437e39e44 100644
> > > > --- a/certs/blacklist.c
> > > > +++ b/certs/blacklist.c
> > > > @@ -100,7 +100,8 @@ int mark_hash_blacklisted(const char *hash)
> > > > if (IS_ERR(key)) {
> > > > pr_err("Problem blacklisting hash (%ld)\n",
> > > > PTR_ERR(key));
> > > > return PTR_ERR(key);
> > > > - }
> > > > + } else
> > > > + pr_notice("Loaded blacklisting hash '%s'\n", hash);
> > > > return 0;
> > > > }
> > > >
> > > > diff --git a/security/integrity/platform_certs/load_uefi.c
> > > > b/security/integrity/platform_certs/load_uefi.c
> > > > index 81b19c52832b..6b6996e5bc27 100644
> > > > --- a/security/integrity/platform_certs/load_uefi.c
> > > > +++ b/security/integrity/platform_certs/load_uefi.c
> > > > @@ -1,5 +1,7 @@
> > > > // SPDX-License-Identifier: GPL-2.0
> > > >
> > > > +#define pr_fmt(fmt) "EFI: "fmt
> > > > +
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -35,6 +37,18 @@ static __init bool uefi_check_ignore_db(void)
> > > > return status == EFI_SUCCESS;
> > > > }
> > > >
> > > > +static void str16_to_str(efi_char16_t *str16, char *str, int str_size)
> > > > +{
> > > > + int i = 0;
> > > > +
> > > > + while (str16[i] != '\0' && i < (str_size - 1)) {
> > > > + str[i] = str16[i];
> > > > + i++;
> > > > + }
> > > > +
> > > > + str[i] = '\0';
> > > > +}
> > > > +
> > > > /*
> > > > * Get a certificate list blob from the named EFI variable.
> > > > */
> > > > @@ -44,13 +58,20 @@ static __init void *get_cert_list(efi_char16_t
> > > > *name, efi_guid_t *guid,
> > > > efi_status_t status;
> > > > unsigned long lsize = 4;
> > > > unsigned long tmpdb[4];
> > > > + char namestr[16];
> > > > void *db;
> > > >
> > > > + str16_to_str(name, namestr, ARRAY_SIZE(namestr));
> > >
> > > Please drop this (and the function above) - instead, just return NULL
> > > if the variable is not found (without reporting an error).
> > >
> >
> > This name string is for printing debug level message, not error message.
> > This function already returns NULL when EFI_NOT_FOUND be returned by
> > firmware.
> >
> > > > 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);
> > > > + if (status == EFI_NOT_FOUND)
> > > > + pr_debug("UEFI %s list doesn't exist\n",
> > > > namestr);
> > > > + else
> > > > + pr_err("Couldn't get size for UEFI %s list:
> > > > %s\n",
> > > > + namestr, efi_status_to_str(status));
> > > > return NULL;
> >
> > here returns NULL when EFI_NOT_FOUND. The message of existence is for
> > debugging.
> >
>
> I understand that. B
Re: [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
On Fri, 3 May 2019 at 09:18, joeyli wrote:
>
> Hi Ard,
>
> On Thu, May 02, 2019 at 11:04:34AM +0200, Ard Biesheuvel wrote:
> > On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi wrote:
> > >
> > > When loading certificates list from UEFI variable, the original error
> > > message direct shows the efi status code from UEFI firmware. It looks
> > > ugly:
> > >
> > > [2.335031] Couldn't get size: 0x800e
> > > [2.335032] Couldn't get UEFI MokListRT
> > > [2.339985] Couldn't get size: 0x800e
> > > [2.339987] Couldn't get UEFI dbx list
> > >
> > > So, this patch shows the status string instead of status code.
> > >
> > > On the other hand, the "Couldn't get UEFI" message doesn't need
> > > to be exposed when db/dbx/mok variable do not exist. So, this
> > > patch set the message level to debug.
> > >
> > > v3.
> > > - Print messages similar to db/mok when loading dbx hash to blacklist:
> > > [1.500952] EFI: Blacklisting hash of an executable: UEFI:dbx
> > > [1.501773] blacklist: Loaded blacklisting hash
> > > 'bin:80b4d96931bf0d02fd91a61e19d14f1da452e66db2408ca8604d411f92659f0a'
> > >
> > > - Setting messages for the existence of db/mok/dbx lists to debug level.
> > >
> > > v2.
> > > Setting the MODSIGN messages level to debug.
> > >
> > > Link:
> > > https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516
> > > Cc: James Morris
> > > Cc: Serge E. Hallyn"
> > > Cc: David Howells
> > > Cc: Nayna Jain
> > > Cc: Josh Boyer
> > > Cc: Mimi Zohar
> > > Signed-off-by: "Lee, Chun-Yi"
> > > ---
> > > certs/blacklist.c | 3 +-
> > > security/integrity/platform_certs/load_uefi.c | 40
> > > +++
> > > 2 files changed, 31 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > index 3a507b9e2568..f91437e39e44 100644
> > > --- a/certs/blacklist.c
> > > +++ b/certs/blacklist.c
> > > @@ -100,7 +100,8 @@ int mark_hash_blacklisted(const char *hash)
> > > if (IS_ERR(key)) {
> > > pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key));
> > > return PTR_ERR(key);
> > > - }
> > > + } else
> > > + pr_notice("Loaded blacklisting hash '%s'\n", hash);
> > > return 0;
> > > }
> > >
> > > diff --git a/security/integrity/platform_certs/load_uefi.c
> > > b/security/integrity/platform_certs/load_uefi.c
> > > index 81b19c52832b..6b6996e5bc27 100644
> > > --- a/security/integrity/platform_certs/load_uefi.c
> > > +++ b/security/integrity/platform_certs/load_uefi.c
> > > @@ -1,5 +1,7 @@
> > > // SPDX-License-Identifier: GPL-2.0
> > >
> > > +#define pr_fmt(fmt) "EFI: "fmt
> > > +
> > > #include
> > > #include
> > > #include
> > > @@ -35,6 +37,18 @@ static __init bool uefi_check_ignore_db(void)
> > > return status == EFI_SUCCESS;
> > > }
> > >
> > > +static void str16_to_str(efi_char16_t *str16, char *str, int str_size)
> > > +{
> > > + int i = 0;
> > > +
> > > + while (str16[i] != '\0' && i < (str_size - 1)) {
> > > + str[i] = str16[i];
> > > + i++;
> > > + }
> > > +
> > > + str[i] = '\0';
> > > +}
> > > +
> > > /*
> > > * Get a certificate list blob from the named EFI variable.
> > > */
> > > @@ -44,13 +58,20 @@ static __init void *get_cert_list(efi_char16_t *name,
> > > efi_guid_t *guid,
> > > efi_status_t status;
> > > unsigned long lsize = 4;
> > > unsigned long tmpdb[4];
> > > + char namestr[16];
> > > void *db;
> > >
> > > + str16_to_str(name, namestr, ARRAY_SIZE(namestr));
> >
> > Please drop this (and the function above) - instead, just return NULL
> > if the variable is not found (without reporting an error).
> >
>
> This name string is for printing debug level message, not error message.
> This function already returns NULL when EFI_NOT_FOUND be returned by
> firmware.
>
> > > 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);
> > > + if (status == EFI_NOT_FOUND)
> > > + pr_debug("UEFI %s list doesn't exist\n", namestr);
> > > + else
> > > + pr_err("Couldn't get size for UEFI %s list: %s\n",
> > > + namestr, efi_status_to_str(status));
> > > return NULL;
>
> here returns NULL when EFI_NOT_FOUND. The message of existence is for
> debugging.
>
I understand that. But I don't think we need it.
> > > }
> > > + pr_debug("UEFI %s list exists\n", namestr);
> > >
> > > db = kmalloc(lsize, GFP_KERNEL);
> > > if (!db)
> > > @@ -59,7 +80,8 @@ static __init void *get_cert_list(efi_char16_t *name,
> > > efi_guid_t *guid,
> > > status = efi.get_variable(name, guid, NULL, &lsi
Re: [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
Hi Ard,
On Thu, May 02, 2019 at 11:04:34AM +0200, Ard Biesheuvel wrote:
> On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi wrote:
> >
> > When loading certificates list from UEFI variable, the original error
> > message direct shows the efi status code from UEFI firmware. It looks
> > ugly:
> >
> > [2.335031] Couldn't get size: 0x800e
> > [2.335032] Couldn't get UEFI MokListRT
> > [2.339985] Couldn't get size: 0x800e
> > [2.339987] Couldn't get UEFI dbx list
> >
> > So, this patch shows the status string instead of status code.
> >
> > On the other hand, the "Couldn't get UEFI" message doesn't need
> > to be exposed when db/dbx/mok variable do not exist. So, this
> > patch set the message level to debug.
> >
> > v3.
> > - Print messages similar to db/mok when loading dbx hash to blacklist:
> > [1.500952] EFI: Blacklisting hash of an executable: UEFI:dbx
> > [1.501773] blacklist: Loaded blacklisting hash
> > 'bin:80b4d96931bf0d02fd91a61e19d14f1da452e66db2408ca8604d411f92659f0a'
> >
> > - Setting messages for the existence of db/mok/dbx lists to debug level.
> >
> > v2.
> > Setting the MODSIGN messages level to debug.
> >
> > Link:
> > https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516
> > Cc: James Morris
> > Cc: Serge E. Hallyn"
> > Cc: David Howells
> > Cc: Nayna Jain
> > Cc: Josh Boyer
> > Cc: Mimi Zohar
> > Signed-off-by: "Lee, Chun-Yi"
> > ---
> > certs/blacklist.c | 3 +-
> > security/integrity/platform_certs/load_uefi.c | 40
> > +++
> > 2 files changed, 31 insertions(+), 12 deletions(-)
> >
> > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > index 3a507b9e2568..f91437e39e44 100644
> > --- a/certs/blacklist.c
> > +++ b/certs/blacklist.c
> > @@ -100,7 +100,8 @@ int mark_hash_blacklisted(const char *hash)
> > if (IS_ERR(key)) {
> > pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key));
> > return PTR_ERR(key);
> > - }
> > + } else
> > + pr_notice("Loaded blacklisting hash '%s'\n", hash);
> > return 0;
> > }
> >
> > diff --git a/security/integrity/platform_certs/load_uefi.c
> > b/security/integrity/platform_certs/load_uefi.c
> > index 81b19c52832b..6b6996e5bc27 100644
> > --- a/security/integrity/platform_certs/load_uefi.c
> > +++ b/security/integrity/platform_certs/load_uefi.c
> > @@ -1,5 +1,7 @@
> > // SPDX-License-Identifier: GPL-2.0
> >
> > +#define pr_fmt(fmt) "EFI: "fmt
> > +
> > #include
> > #include
> > #include
> > @@ -35,6 +37,18 @@ static __init bool uefi_check_ignore_db(void)
> > return status == EFI_SUCCESS;
> > }
> >
> > +static void str16_to_str(efi_char16_t *str16, char *str, int str_size)
> > +{
> > + int i = 0;
> > +
> > + while (str16[i] != '\0' && i < (str_size - 1)) {
> > + str[i] = str16[i];
> > + i++;
> > + }
> > +
> > + str[i] = '\0';
> > +}
> > +
> > /*
> > * Get a certificate list blob from the named EFI variable.
> > */
> > @@ -44,13 +58,20 @@ static __init void *get_cert_list(efi_char16_t *name,
> > efi_guid_t *guid,
> > efi_status_t status;
> > unsigned long lsize = 4;
> > unsigned long tmpdb[4];
> > + char namestr[16];
> > void *db;
> >
> > + str16_to_str(name, namestr, ARRAY_SIZE(namestr));
>
> Please drop this (and the function above) - instead, just return NULL
> if the variable is not found (without reporting an error).
>
This name string is for printing debug level message, not error message.
This function already returns NULL when EFI_NOT_FOUND be returned by
firmware.
> > 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);
> > + if (status == EFI_NOT_FOUND)
> > + pr_debug("UEFI %s list doesn't exist\n", namestr);
> > + else
> > + pr_err("Couldn't get size for UEFI %s list: %s\n",
> > + namestr, efi_status_to_str(status));
> > return NULL;
here returns NULL when EFI_NOT_FOUND. The message of existence is for
debugging.
> > }
> > + pr_debug("UEFI %s list exists\n", namestr);
> >
> > db = kmalloc(lsize, GFP_KERNEL);
> > if (!db)
> > @@ -59,7 +80,8 @@ static __init void *get_cert_list(efi_char16_t *name,
> > efi_guid_t *guid,
> > 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 UEFI %s list: %s\n",
> > + namestr, efi_status_to_str(status));
> > return NULL;
> > }
> >
> > @@ -95,6 +117,7 @@
Re: [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi wrote:
>
> When loading certificates list from UEFI variable, the original error
> message direct shows the efi status code from UEFI firmware. It looks
> ugly:
>
> [2.335031] Couldn't get size: 0x800e
> [2.335032] Couldn't get UEFI MokListRT
> [2.339985] Couldn't get size: 0x800e
> [2.339987] Couldn't get UEFI dbx list
>
> So, this patch shows the status string instead of status code.
>
> On the other hand, the "Couldn't get UEFI" message doesn't need
> to be exposed when db/dbx/mok variable do not exist. So, this
> patch set the message level to debug.
>
> v3.
> - Print messages similar to db/mok when loading dbx hash to blacklist:
> [1.500952] EFI: Blacklisting hash of an executable: UEFI:dbx
> [1.501773] blacklist: Loaded blacklisting hash
> 'bin:80b4d96931bf0d02fd91a61e19d14f1da452e66db2408ca8604d411f92659f0a'
>
> - Setting messages for the existence of db/mok/dbx lists to debug level.
>
> v2.
> Setting the MODSIGN messages level to debug.
>
> Link:
> https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516
> Cc: James Morris
> Cc: Serge E. Hallyn"
> Cc: David Howells
> Cc: Nayna Jain
> Cc: Josh Boyer
> Cc: Mimi Zohar
> Signed-off-by: "Lee, Chun-Yi"
> ---
> certs/blacklist.c | 3 +-
> security/integrity/platform_certs/load_uefi.c | 40
> +++
> 2 files changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index 3a507b9e2568..f91437e39e44 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -100,7 +100,8 @@ int mark_hash_blacklisted(const char *hash)
> if (IS_ERR(key)) {
> pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key));
> return PTR_ERR(key);
> - }
> + } else
> + pr_notice("Loaded blacklisting hash '%s'\n", hash);
> return 0;
> }
>
> diff --git a/security/integrity/platform_certs/load_uefi.c
> b/security/integrity/platform_certs/load_uefi.c
> index 81b19c52832b..6b6996e5bc27 100644
> --- a/security/integrity/platform_certs/load_uefi.c
> +++ b/security/integrity/platform_certs/load_uefi.c
> @@ -1,5 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
>
> +#define pr_fmt(fmt) "EFI: "fmt
> +
> #include
> #include
> #include
> @@ -35,6 +37,18 @@ static __init bool uefi_check_ignore_db(void)
> return status == EFI_SUCCESS;
> }
>
> +static void str16_to_str(efi_char16_t *str16, char *str, int str_size)
> +{
> + int i = 0;
> +
> + while (str16[i] != '\0' && i < (str_size - 1)) {
> + str[i] = str16[i];
> + i++;
> + }
> +
> + str[i] = '\0';
> +}
> +
> /*
> * Get a certificate list blob from the named EFI variable.
> */
> @@ -44,13 +58,20 @@ static __init void *get_cert_list(efi_char16_t *name,
> efi_guid_t *guid,
> efi_status_t status;
> unsigned long lsize = 4;
> unsigned long tmpdb[4];
> + char namestr[16];
> void *db;
>
> + str16_to_str(name, namestr, ARRAY_SIZE(namestr));
Please drop this (and the function above) - instead, just return NULL
if the variable is not found (without reporting an error).
> 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);
> + if (status == EFI_NOT_FOUND)
> + pr_debug("UEFI %s list doesn't exist\n", namestr);
> + else
> + pr_err("Couldn't get size for UEFI %s list: %s\n",
> + namestr, efi_status_to_str(status));
> return NULL;
> }
> + pr_debug("UEFI %s list exists\n", namestr);
>
> db = kmalloc(lsize, GFP_KERNEL);
> if (!db)
> @@ -59,7 +80,8 @@ static __init void *get_cert_list(efi_char16_t *name,
> efi_guid_t *guid,
> 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 UEFI %s list: %s\n",
> + namestr, efi_status_to_str(status));
> return NULL;
> }
>
> @@ -95,6 +117,7 @@ static __init void uefi_blacklist_hash(const char *source,
> const void *data,
> static __init void uefi_blacklist_x509_tbs(const char *source,
>const void *data, size_t len)
> {
> + pr_info("Blacklisting X.509 TBS hash: %s\n", source);
> uefi_blacklist_hash(source, data, len, "tbs:", 4);
> }
>
> @@ -104,6 +127,7 @@ static __init void uefi_blacklist_x509_tbs(const char
> *source,
> static __init void uefi_blacklist_binary(const char *source,
> const void *d
[PATCH 2/2 v3] efi: print appropriate status message when loading certificates
When loading certificates list from UEFI variable, the original error
message direct shows the efi status code from UEFI firmware. It looks
ugly:
[2.335031] Couldn't get size: 0x800e
[2.335032] Couldn't get UEFI MokListRT
[2.339985] Couldn't get size: 0x800e
[2.339987] Couldn't get UEFI dbx list
So, this patch shows the status string instead of status code.
On the other hand, the "Couldn't get UEFI" message doesn't need
to be exposed when db/dbx/mok variable do not exist. So, this
patch set the message level to debug.
v3.
- Print messages similar to db/mok when loading dbx hash to blacklist:
[1.500952] EFI: Blacklisting hash of an executable: UEFI:dbx
[1.501773] blacklist: Loaded blacklisting hash
'bin:80b4d96931bf0d02fd91a61e19d14f1da452e66db2408ca8604d411f92659f0a'
- Setting messages for the existence of db/mok/dbx lists to debug level.
v2.
Setting the MODSIGN messages level to debug.
Link:
https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516
Cc: James Morris
Cc: Serge E. Hallyn"
Cc: David Howells
Cc: Nayna Jain
Cc: Josh Boyer
Cc: Mimi Zohar
Signed-off-by: "Lee, Chun-Yi"
---
certs/blacklist.c | 3 +-
security/integrity/platform_certs/load_uefi.c | 40 +++
2 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/certs/blacklist.c b/certs/blacklist.c
index 3a507b9e2568..f91437e39e44 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -100,7 +100,8 @@ int mark_hash_blacklisted(const char *hash)
if (IS_ERR(key)) {
pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key));
return PTR_ERR(key);
- }
+ } else
+ pr_notice("Loaded blacklisting hash '%s'\n", hash);
return 0;
}
diff --git a/security/integrity/platform_certs/load_uefi.c
b/security/integrity/platform_certs/load_uefi.c
index 81b19c52832b..6b6996e5bc27 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
+#define pr_fmt(fmt) "EFI: "fmt
+
#include
#include
#include
@@ -35,6 +37,18 @@ static __init bool uefi_check_ignore_db(void)
return status == EFI_SUCCESS;
}
+static void str16_to_str(efi_char16_t *str16, char *str, int str_size)
+{
+ int i = 0;
+
+ while (str16[i] != '\0' && i < (str_size - 1)) {
+ str[i] = str16[i];
+ i++;
+ }
+
+ str[i] = '\0';
+}
+
/*
* Get a certificate list blob from the named EFI variable.
*/
@@ -44,13 +58,20 @@ static __init void *get_cert_list(efi_char16_t *name,
efi_guid_t *guid,
efi_status_t status;
unsigned long lsize = 4;
unsigned long tmpdb[4];
+ char namestr[16];
void *db;
+ str16_to_str(name, namestr, ARRAY_SIZE(namestr));
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);
+ if (status == EFI_NOT_FOUND)
+ pr_debug("UEFI %s list doesn't exist\n", namestr);
+ else
+ pr_err("Couldn't get size for UEFI %s list: %s\n",
+ namestr, efi_status_to_str(status));
return NULL;
}
+ pr_debug("UEFI %s list exists\n", namestr);
db = kmalloc(lsize, GFP_KERNEL);
if (!db)
@@ -59,7 +80,8 @@ static __init void *get_cert_list(efi_char16_t *name,
efi_guid_t *guid,
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 UEFI %s list: %s\n",
+ namestr, efi_status_to_str(status));
return NULL;
}
@@ -95,6 +117,7 @@ static __init void uefi_blacklist_hash(const char *source,
const void *data,
static __init void uefi_blacklist_x509_tbs(const char *source,
const void *data, size_t len)
{
+ pr_info("Blacklisting X.509 TBS hash: %s\n", source);
uefi_blacklist_hash(source, data, len, "tbs:", 4);
}
@@ -104,6 +127,7 @@ static __init void uefi_blacklist_x509_tbs(const char
*source,
static __init void uefi_blacklist_binary(const char *source,
const void *data, size_t len)
{
+ pr_info("Blacklisting hash of an executable: %s\n", source);
uefi_blacklist_hash(source, data, len, "bin:", 4);
}
@@ -154,9 +178,7 @@ static int __init load_uefi_certs(void)
*/
if (!uefi_check_ignore_db()) {
db = get_cert_list(L"db", &secure_var, &dbsize);
- if (!db) {
- pr_err("MODSIGN: Couldn't get UEFI db list\n");
-
