Re: [PATCH 1/2] efi: add a function for transferring status to string
Hi Mimi,
On Wed, Mar 27, 2019 at 03:04:02PM -0400, Mimi Zohar wrote:
> On Wed, 2019-03-27 at 19:58 +0100, Ard Biesheuvel wrote:
> > On Sun, 24 Mar 2019 at 01:26, Lee, Chun-Yi wrote:
> > >
> > > This function can be used to transfer EFI status code to string
> > > for printing out debug message. Using this function can improve
> > > the readability of log.
>
> Maybe instead of "for transferring status" use "to convert the status
> value to a" in the Subject line and here in the patch description.
>
Thanks for your suggestion. I will change subject and description.
> > >
> > > Cc: Ard Biesheuvel
> > > Cc: Kees Cook
> > > Cc: Anton Vorontsov
> > > Cc: Colin Cross
> > > Cc: Tony Luck
> > > Signed-off-by: "Lee, Chun-Yi"
> > > ---
> > > include/linux/efi.h | 28
> > > 1 file changed, 28 insertions(+)
> > >
> > > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > > index 54357a258b35..a43cb0dc37af 100644
> > > --- a/include/linux/efi.h
> > > +++ b/include/linux/efi.h
> > > @@ -1768,4 +1768,32 @@ struct linux_efi_memreserve {
> > > #define EFI_MEMRESERVE_COUNT(size) (((size) - sizeof(struct
> > > linux_efi_memreserve)) \
> > > / sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
> > >
> > > +#define EFI_STATUS_STR(_status) \
> > > +case EFI_##_status: \
> > > + return "EFI_" __stringify(_status);
> > > +
> > > +static inline char *
> > > +efi_status_to_str(efi_status_t status)
> > > +{
> > > + switch (status) {
> > > + EFI_STATUS_STR(SUCCESS)
> > > + EFI_STATUS_STR(LOAD_ERROR)
> > > + EFI_STATUS_STR(INVALID_PARAMETER)
> > > + EFI_STATUS_STR(UNSUPPORTED)
> > > + EFI_STATUS_STR(BAD_BUFFER_SIZE)
> > > + EFI_STATUS_STR(BUFFER_TOO_SMALL)
> > > + EFI_STATUS_STR(NOT_READY)
> > > + EFI_STATUS_STR(DEVICE_ERROR)
> > > + EFI_STATUS_STR(WRITE_PROTECTED)
> > > + EFI_STATUS_STR(OUT_OF_RESOURCES)
> > > + EFI_STATUS_STR(NOT_FOUND)
> > > + EFI_STATUS_STR(ABORTED)
> > > + EFI_STATUS_STR(SECURITY_VIOLATION)
> > > + default:
> > > + pr_warn("Unknown efi status: 0x%lx", status);
> > > + }
> > > +
> > > + return "Unknown efi status";
> > > +}
> > > +
> > > #endif /* _LINUX_EFI_H */
> > > --
> > > 2.16.4
> > >
> >
> > Please turn this into a proper function so that not every calling
> > object has to duplicate all these strings.
>
> Hi Ard,
>
> Keeping the status values and strings in sync is difficult. I was
> going to suggest moving the macro immediately after the status value
> definitions.
>
I will move the code to after the status value definitions.
Thanks
Joey Lee
Re: [PATCH 1/2] efi: add a function for transferring status to string
Hi Ard,
On Wed, Mar 27, 2019 at 07:58:03PM +0100, Ard Biesheuvel wrote:
> On Sun, 24 Mar 2019 at 01:26, Lee, Chun-Yi wrote:
> >
> > This function can be used to transfer EFI status code to string
> > for printing out debug message. Using this function can improve
> > the readability of log.
> >
> > Cc: Ard Biesheuvel
> > Cc: Kees Cook
> > Cc: Anton Vorontsov
> > Cc: Colin Cross
> > Cc: Tony Luck
> > Signed-off-by: "Lee, Chun-Yi"
> > ---
> > include/linux/efi.h | 28
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 54357a258b35..a43cb0dc37af 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1768,4 +1768,32 @@ struct linux_efi_memreserve {
> > #define EFI_MEMRESERVE_COUNT(size) (((size) - sizeof(struct
> > linux_efi_memreserve)) \
> > / sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
> >
> > +#define EFI_STATUS_STR(_status) \
> > +case EFI_##_status: \
> > + return "EFI_" __stringify(_status);
> > +
> > +static inline char *
> > +efi_status_to_str(efi_status_t status)
> > +{
> > + switch (status) {
> > + EFI_STATUS_STR(SUCCESS)
> > + EFI_STATUS_STR(LOAD_ERROR)
> > + EFI_STATUS_STR(INVALID_PARAMETER)
> > + EFI_STATUS_STR(UNSUPPORTED)
> > + EFI_STATUS_STR(BAD_BUFFER_SIZE)
> > + EFI_STATUS_STR(BUFFER_TOO_SMALL)
> > + EFI_STATUS_STR(NOT_READY)
> > + EFI_STATUS_STR(DEVICE_ERROR)
> > + EFI_STATUS_STR(WRITE_PROTECTED)
> > + EFI_STATUS_STR(OUT_OF_RESOURCES)
> > + EFI_STATUS_STR(NOT_FOUND)
> > + EFI_STATUS_STR(ABORTED)
> > + EFI_STATUS_STR(SECURITY_VIOLATION)
> > + default:
> > + pr_warn("Unknown efi status: 0x%lx", status);
> > + }
> > +
> > + return "Unknown efi status";
> > +}
> > +
> > #endif /* _LINUX_EFI_H */
> > --
> > 2.16.4
> >
>
> Please turn this into a proper function so that not every calling
> object has to duplicate all these strings.
OK! I will move them to function.
Thanks for your review!
Joey Lee
[PATCH] efi/arm: enable CP15 DMB instructions before cleaning the cache
The EFI stub is entered with the caches and MMU enabled by the firmware, and once the stub is ready to hand over to the decompressor, we clean and disable the caches. The cache clean routines use CP15 barrier instructions, which can be disabled via SCTLR. Normally, when using the provided cache handling routines to enable the caches and MMU, this bit is enabled as well. However, but since we entered the stub with the caches already enabled, this routine is not executed before we call the cache clean routines, resulting in undefined instruction exceptions if the firmware never enabled this bit. So set the bit explicitly in the EFI entry code. Cc: Marc Zyngier Signed-off-by: Ard Biesheuvel --- arch/arm/boot/compressed/head.S | 10 ++ 1 file changed, 10 insertions(+) diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S index 6c7ccb428c07..62a49356fca3 100644 --- a/arch/arm/boot/compressed/head.S +++ b/arch/arm/boot/compressed/head.S @@ -1438,6 +1438,16 @@ ENTRY(efi_stub_entry) @ Preserve return value of efi_entry() in r4 mov r4, r0 + + @ our cache maintenance code relies on CP15 barrier instructions + @ but since we arrived here with the MMU and caches configured + @ by UEFI, we must ensure that the use of those instructions is + @ enabled in the SCTLR register, since we never executed our own + @ cache enable routine, which is normally in charge of this. + mrc p15, 0, r1, c1, c0, 0 @ read SCTLR + orr r1, r1, #(1 << 5) @ CP15 barrier instructions + mcr p15, 0, r1, c1, c0, 0 @ write SCTLR + bl cache_clean_flush bl cache_off -- 2.20.1
Re: [PATCH 2/2 v2] efi: print appropriate status message when loading certificates
On Wed, Mar 27, 2019 at 03:23:55PM -0400, Mimi Zohar wrote:
> On Sun, 2019-03-24 at 08:26 +0800, 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.
> >
> > v2.
> > Setting the MODSIGN messagse 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"
> > ---
> > security/integrity/platform_certs/load_uefi.c | 13 -
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/security/integrity/platform_certs/load_uefi.c
> > b/security/integrity/platform_certs/load_uefi.c
> > index 81b19c52832b..e65244b31f04 100644
> > --- a/security/integrity/platform_certs/load_uefi.c
> > +++ b/security/integrity/platform_certs/load_uefi.c
> > @@ -48,7 +48,9 @@ static __init void *get_cert_list(efi_char16_t *name,
> > efi_guid_t *guid,
> >
> > status = efi.get_variable(name, guid, NULL, , );
> > if (status != EFI_BUFFER_TOO_SMALL) {
> > - pr_err("Couldn't get size: 0x%lx\n", status);
> > + if (status != EFI_NOT_FOUND)
> > + pr_err("Couldn't get size: %s\n",
> > + efi_status_to_str(status));
> > return NULL;
> > }
> >
> > @@ -59,7 +61,8 @@ static __init void *get_cert_list(efi_char16_t *name,
> > efi_guid_t *guid,
> > status = efi.get_variable(name, guid, NULL, , db);
> > if (status != EFI_SUCCESS) {
> > kfree(db);
> > - pr_err("Error reading db var: 0x%lx\n", status);
> > + pr_err("Error reading db var: %s\n",
> > + efi_status_to_str(status));
> > return NULL;
> > }
> >
> > @@ -155,7 +158,7 @@ static int __init load_uefi_certs(void)
> > if (!uefi_check_ignore_db()) {
> > db = get_cert_list(L"db", _var, );
> > if (!db) {
> > - pr_err("MODSIGN: Couldn't get UEFI db list\n");
> > + pr_debug("MODSIGN: Couldn't get UEFI db list\n");
>
> Sure, this is fine.
>
> > } else {
> > rc = parse_efi_signature_list("UEFI:db",
> > db, dbsize, get_handler_for_db);
> > @@ -168,7 +171,7 @@ static int __init load_uefi_certs(void)
> >
> > mok = get_cert_list(L"MokListRT", _var, );
> > if (!mok) {
> > - pr_info("Couldn't get UEFI MokListRT\n");
> > + pr_debug("Couldn't get UEFI MokListRT\n");
>
> This is fine too.
>
> > } else {
> > rc = parse_efi_signature_list("UEFI:MokListRT",
> > mok, moksize, get_handler_for_db);
> > @@ -179,7 +182,7 @@ static int __init load_uefi_certs(void)
> >
> > dbx = get_cert_list(L"dbx", _var, );
> > if (!dbx) {
> > - pr_info("Couldn't get UEFI dbx list\n");
> > + pr_debug("Couldn't get UEFI dbx list\n");
>
> If there isn't a db or moklist, then this is fine. My concern is not
> having an indication that the dbx wasn't installed, when it should
> have been.
>
> Perhaps similar to the "Loading compiled-in X.509 certificates"
> informational message there should informational messages for db, mok,
> and dbx as well.
>
OK. I will add message when kernel found db, dbx and mok. It will
just like the informaton message for ACPI S0,S3,S4 support.
Thanks
Joey Lee
[tip:x86/urgent] x86/realmode: Make set_real_mode_mem() static inline
Commit-ID: f560bd19d2fe0e54851d706b72acbc6f2eed3567
Gitweb: https://git.kernel.org/tip/f560bd19d2fe0e54851d706b72acbc6f2eed3567
Author: Matteo Croce
AuthorDate: Thu, 28 Mar 2019 12:42:33 +0100
Committer: Borislav Petkov
CommitDate: Fri, 29 Mar 2019 10:16:27 +0100
x86/realmode: Make set_real_mode_mem() static inline
Remove the unused @size argument and move it into a header file, so it
can be inlined.
[ bp: Massage. ]
Signed-off-by: Matteo Croce
Signed-off-by: Borislav Petkov
Reviewed-by: Mukesh Ojha
Cc: Ard Biesheuvel
Cc: "H. Peter Anvin"
Cc: Ingo Molnar
Cc: linux-efi
Cc: platform-driver-...@vger.kernel.org
Cc: Thomas Gleixner
Cc: x86-ml
Link: https://lkml.kernel.org/r/20190328114233.27835-1-mcr...@redhat.com
---
arch/x86/include/asm/realmode.h | 6 +-
arch/x86/platform/efi/quirks.c | 2 +-
arch/x86/realmode/init.c| 9 +
3 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index 63b3393bd98e..c53682303c9c 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -77,7 +77,11 @@ static inline size_t real_mode_size_needed(void)
return ALIGN(real_mode_blob_end - real_mode_blob, PAGE_SIZE);
}
-void set_real_mode_mem(phys_addr_t mem, size_t size);
+static inline void set_real_mode_mem(phys_addr_t mem)
+{
+ real_mode_header = (struct real_mode_header *) __va(mem);
+}
+
void reserve_real_mode(void);
#endif /* __ASSEMBLY__ */
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 458a0e2bcc57..a25a9fd987a9 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -449,7 +449,7 @@ void __init efi_free_boot_services(void)
*/
rm_size = real_mode_size_needed();
if (rm_size && (start + rm_size) < (1<<20) && size >= rm_size) {
- set_real_mode_mem(start, rm_size);
+ set_real_mode_mem(start);
start += rm_size;
size -= rm_size;
}
diff --git a/arch/x86/realmode/init.c b/arch/x86/realmode/init.c
index 47d097946872..7dce39c8c034 100644
--- a/arch/x86/realmode/init.c
+++ b/arch/x86/realmode/init.c
@@ -15,13 +15,6 @@ u32 *trampoline_cr4_features;
/* Hold the pgd entry used on booting additional CPUs */
pgd_t trampoline_pgd_entry;
-void __init set_real_mode_mem(phys_addr_t mem, size_t size)
-{
- void *base = __va(mem);
-
- real_mode_header = (struct real_mode_header *) base;
-}
-
void __init reserve_real_mode(void)
{
phys_addr_t mem;
@@ -40,7 +33,7 @@ void __init reserve_real_mode(void)
}
memblock_reserve(mem, size);
- set_real_mode_mem(mem, size);
+ set_real_mode_mem(mem);
}
static void __init setup_real_mode(void)
Re: [GIT PULL 0/5] EFI changes for v5.2
* Ard Biesheuvel wrote: > The following changes since commit 8c2ffd9174779014c3fe1f96d9dc3641d9175f00: > > Linux 5.1-rc2 (2019-03-24 14:02:26 -0700) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-next > > for you to fetch changes up to a72f65180feb446cc59616aab400475679b6ffce: > > efi/libstub/arm: omit unneeded stripping of ksymtab/kcrctab sections > (2019-03-28 09:22:32 +0100) > > > EFI updates for v5.2: > - Squash a spurious warning when using the EFI framebuffer on a non-EFI boot. > - Use DMI data to annotate RAS memory errors on ARM just like we do on Intel. > - Some followup cleanups for DMI. > - Some libstub Makefile cleanups. > > > Ard Biesheuvel (2): > efifb: omit memory map check on legacy boot > efi/libstub/arm: omit unneeded stripping of ksymtab/kcrctab sections > > Marcin Benka (1): > efi/arm: Show SMBIOS bank/device location in cper and ghes error logs > > Masahiro Yamada (1): > efi/libstub: refactor cmd_stubcopy > > Robert Richter (1): > efi: Unify dmi setup code over architectures arm/arm64, ia64 and x86 > > arch/ia64/kernel/setup.c | 4 +--- > arch/x86/kernel/setup.c | 6 ++ > drivers/firmware/dmi_scan.c | 28 +++- > drivers/firmware/efi/arm-runtime.c| 6 ++ > drivers/firmware/efi/libstub/Makefile | 14 +++--- > drivers/video/fbdev/efifb.c | 3 ++- > include/linux/dmi.h | 8 ++-- > 7 files changed, 31 insertions(+), 38 deletions(-) Applied to tip:efi/core, thanks a lot Ard! Ingo
