All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ardb@kernel.org>
To: linux-efi@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org, catalin.marinas@arm.com,
	will@kernel.org, Ard Biesheuvel <ardb@kernel.org>,
	Alexandru Elisei <alexandru.elisei@arm.com>
Subject: [PATCH] arm64: efi: Disable only the misbehaving runtime service on sync exceptions
Date: Tue,  8 Nov 2022 16:15:09 +0100	[thread overview]
Message-ID: <20221108151509.2250968-1-ardb@kernel.org> (raw)

Alexandru reports that his Ampere Altra machine, whose buggy firmware
triggers a synchronous exception in its implementation of SetTime() when
called without SetVirtualAddressMap() having been called first, doesn't
quite recover from this, and starts spewing error messages into the log
that are unrelated to the buggy runtime service.

The driver in question is the EFI RTC driver, which should be fixed in
any case, as flooding the log like that (or doing any logging to the
kernel log at all on something whuch is not a severe issue) is not ok.

However, in this particular case, it would be beneficial for both
ordinary use as well as diagnostics regarding broken firmware if we only
prevent the broken runtime service from being called again, and permit
others (such as GetTime() which triggers the logging or the variable
services) from being used as normal.

So wire up the existing efi.runtime_supported_mask, and clear the
service's bit in the mask if the generic runtime service wrapper
observes a return value of EFI_ABORTED, which only happens if a service
call is aborted due to an exception. (EFI_ABORTED is not documented as a
valid error code for any of the EFI runtime services).

Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/kernel/efi.c                 |  1 -
 arch/x86/platform/efi/quirks.c          |  2 +-
 drivers/firmware/efi/runtime-wrappers.c |  6 ++++--
 include/linux/efi.h                     | 24 ++++++++++++------------
 4 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index ee53f2a0aa03f54a..a3b8852f2698d9f7 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -153,7 +153,6 @@ asmlinkage efi_status_t __efi_rt_asm_recover(void);
 asmlinkage efi_status_t efi_handle_runtime_exception(const char *f)
 {
 	pr_err(FW_BUG "Synchronous exception occurred in EFI runtime service %s()\n", f);
-	clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 	return EFI_ABORTED;
 }
 
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index b0b848d6933afbcf..3f48fdf42d97dc1f 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -757,9 +757,9 @@ void efi_crash_gracefully_on_page_fault(unsigned long phys_addr)
 
 	/* Signal error status to the efi caller process */
 	efi_rts_work.status = EFI_ABORTED;
+	efi.runtime_supported_mask = 0;
 	complete(&efi_rts_work.efi_rts_comp);
 
-	clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 	pr_info("Froze efi_rts_wq and disabled EFI Runtime Services\n");
 
 	/*
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index f3e54f6616f02475..336b8bcec86d0127 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -60,8 +60,8 @@ struct efi_runtime_work efi_rts_work;
 ({									\
 	efi_rts_work.status = EFI_ABORTED;				\
 									\
-	if (!efi_enabled(EFI_RUNTIME_SERVICES)) {			\
-		pr_warn_once("EFI Runtime Services are disabled!\n");	\
+	if (!efi_rt_services_supported(efi_rts_work.efi_rts_id)) {	\
+		pr_warn_ratelimited("EFI Runtime Service is disabled!\n");\
 		goto exit;						\
 	}								\
 									\
@@ -83,6 +83,8 @@ struct efi_runtime_work efi_rts_work;
 	else								\
 		pr_err("Failed to queue work to efi_rts_wq.\n");	\
 									\
+	if (efi_rts_work.status == EFI_ABORTED)				\
+		efi.runtime_supported_mask &= ~efi_rts_work.efi_rts_id;	\
 exit:									\
 	efi_rts_work.efi_rts_id = EFI_NONE;				\
 	efi_rts_work.status;						\
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 929d559ad41d29c6..61b252386d61cc4d 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1254,18 +1254,18 @@ extern unsigned long rci2_table_phys;
  */
 enum efi_rts_ids {
 	EFI_NONE,
-	EFI_GET_TIME,
-	EFI_SET_TIME,
-	EFI_GET_WAKEUP_TIME,
-	EFI_SET_WAKEUP_TIME,
-	EFI_GET_VARIABLE,
-	EFI_GET_NEXT_VARIABLE,
-	EFI_SET_VARIABLE,
-	EFI_QUERY_VARIABLE_INFO,
-	EFI_GET_NEXT_HIGH_MONO_COUNT,
-	EFI_RESET_SYSTEM,
-	EFI_UPDATE_CAPSULE,
-	EFI_QUERY_CAPSULE_CAPS,
+	EFI_GET_TIME			= EFI_RT_SUPPORTED_GET_TIME,
+	EFI_SET_TIME			= EFI_RT_SUPPORTED_SET_TIME,
+	EFI_GET_WAKEUP_TIME		= EFI_RT_SUPPORTED_GET_WAKEUP_TIME,
+	EFI_SET_WAKEUP_TIME		= EFI_RT_SUPPORTED_SET_WAKEUP_TIME,
+	EFI_GET_VARIABLE		= EFI_RT_SUPPORTED_GET_VARIABLE,
+	EFI_GET_NEXT_VARIABLE		= EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME,
+	EFI_SET_VARIABLE		= EFI_RT_SUPPORTED_SET_VARIABLE,
+	EFI_GET_NEXT_HIGH_MONO_COUNT	= EFI_RT_SUPPORTED_GET_NEXT_HIGH_MONOTONIC_COUNT,
+	EFI_RESET_SYSTEM		= EFI_RT_SUPPORTED_RESET_SYSTEM,
+	EFI_UPDATE_CAPSULE		= EFI_RT_SUPPORTED_UPDATE_CAPSULE,
+	EFI_QUERY_CAPSULE_CAPS		= EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES,
+	EFI_QUERY_VARIABLE_INFO		= EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO,
 };
 
 /*
-- 
2.35.1


WARNING: multiple messages have this Message-ID (diff)
From: Ard Biesheuvel <ardb@kernel.org>
To: linux-efi@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org, catalin.marinas@arm.com,
	will@kernel.org, Ard Biesheuvel <ardb@kernel.org>,
	Alexandru Elisei <alexandru.elisei@arm.com>
Subject: [PATCH] arm64: efi: Disable only the misbehaving runtime service on sync exceptions
Date: Tue,  8 Nov 2022 16:15:09 +0100	[thread overview]
Message-ID: <20221108151509.2250968-1-ardb@kernel.org> (raw)

Alexandru reports that his Ampere Altra machine, whose buggy firmware
triggers a synchronous exception in its implementation of SetTime() when
called without SetVirtualAddressMap() having been called first, doesn't
quite recover from this, and starts spewing error messages into the log
that are unrelated to the buggy runtime service.

The driver in question is the EFI RTC driver, which should be fixed in
any case, as flooding the log like that (or doing any logging to the
kernel log at all on something whuch is not a severe issue) is not ok.

However, in this particular case, it would be beneficial for both
ordinary use as well as diagnostics regarding broken firmware if we only
prevent the broken runtime service from being called again, and permit
others (such as GetTime() which triggers the logging or the variable
services) from being used as normal.

So wire up the existing efi.runtime_supported_mask, and clear the
service's bit in the mask if the generic runtime service wrapper
observes a return value of EFI_ABORTED, which only happens if a service
call is aborted due to an exception. (EFI_ABORTED is not documented as a
valid error code for any of the EFI runtime services).

Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/kernel/efi.c                 |  1 -
 arch/x86/platform/efi/quirks.c          |  2 +-
 drivers/firmware/efi/runtime-wrappers.c |  6 ++++--
 include/linux/efi.h                     | 24 ++++++++++++------------
 4 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index ee53f2a0aa03f54a..a3b8852f2698d9f7 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -153,7 +153,6 @@ asmlinkage efi_status_t __efi_rt_asm_recover(void);
 asmlinkage efi_status_t efi_handle_runtime_exception(const char *f)
 {
 	pr_err(FW_BUG "Synchronous exception occurred in EFI runtime service %s()\n", f);
-	clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 	return EFI_ABORTED;
 }
 
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index b0b848d6933afbcf..3f48fdf42d97dc1f 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -757,9 +757,9 @@ void efi_crash_gracefully_on_page_fault(unsigned long phys_addr)
 
 	/* Signal error status to the efi caller process */
 	efi_rts_work.status = EFI_ABORTED;
+	efi.runtime_supported_mask = 0;
 	complete(&efi_rts_work.efi_rts_comp);
 
-	clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 	pr_info("Froze efi_rts_wq and disabled EFI Runtime Services\n");
 
 	/*
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index f3e54f6616f02475..336b8bcec86d0127 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -60,8 +60,8 @@ struct efi_runtime_work efi_rts_work;
 ({									\
 	efi_rts_work.status = EFI_ABORTED;				\
 									\
-	if (!efi_enabled(EFI_RUNTIME_SERVICES)) {			\
-		pr_warn_once("EFI Runtime Services are disabled!\n");	\
+	if (!efi_rt_services_supported(efi_rts_work.efi_rts_id)) {	\
+		pr_warn_ratelimited("EFI Runtime Service is disabled!\n");\
 		goto exit;						\
 	}								\
 									\
@@ -83,6 +83,8 @@ struct efi_runtime_work efi_rts_work;
 	else								\
 		pr_err("Failed to queue work to efi_rts_wq.\n");	\
 									\
+	if (efi_rts_work.status == EFI_ABORTED)				\
+		efi.runtime_supported_mask &= ~efi_rts_work.efi_rts_id;	\
 exit:									\
 	efi_rts_work.efi_rts_id = EFI_NONE;				\
 	efi_rts_work.status;						\
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 929d559ad41d29c6..61b252386d61cc4d 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1254,18 +1254,18 @@ extern unsigned long rci2_table_phys;
  */
 enum efi_rts_ids {
 	EFI_NONE,
-	EFI_GET_TIME,
-	EFI_SET_TIME,
-	EFI_GET_WAKEUP_TIME,
-	EFI_SET_WAKEUP_TIME,
-	EFI_GET_VARIABLE,
-	EFI_GET_NEXT_VARIABLE,
-	EFI_SET_VARIABLE,
-	EFI_QUERY_VARIABLE_INFO,
-	EFI_GET_NEXT_HIGH_MONO_COUNT,
-	EFI_RESET_SYSTEM,
-	EFI_UPDATE_CAPSULE,
-	EFI_QUERY_CAPSULE_CAPS,
+	EFI_GET_TIME			= EFI_RT_SUPPORTED_GET_TIME,
+	EFI_SET_TIME			= EFI_RT_SUPPORTED_SET_TIME,
+	EFI_GET_WAKEUP_TIME		= EFI_RT_SUPPORTED_GET_WAKEUP_TIME,
+	EFI_SET_WAKEUP_TIME		= EFI_RT_SUPPORTED_SET_WAKEUP_TIME,
+	EFI_GET_VARIABLE		= EFI_RT_SUPPORTED_GET_VARIABLE,
+	EFI_GET_NEXT_VARIABLE		= EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME,
+	EFI_SET_VARIABLE		= EFI_RT_SUPPORTED_SET_VARIABLE,
+	EFI_GET_NEXT_HIGH_MONO_COUNT	= EFI_RT_SUPPORTED_GET_NEXT_HIGH_MONOTONIC_COUNT,
+	EFI_RESET_SYSTEM		= EFI_RT_SUPPORTED_RESET_SYSTEM,
+	EFI_UPDATE_CAPSULE		= EFI_RT_SUPPORTED_UPDATE_CAPSULE,
+	EFI_QUERY_CAPSULE_CAPS		= EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES,
+	EFI_QUERY_VARIABLE_INFO		= EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO,
 };
 
 /*
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

             reply	other threads:[~2022-11-08 15:15 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-08 15:15 Ard Biesheuvel [this message]
2022-11-08 15:15 ` [PATCH] arm64: efi: Disable only the misbehaving runtime service on sync exceptions Ard Biesheuvel
2022-11-09 14:42 ` Alexandru Elisei
2022-11-09 14:42   ` Alexandru Elisei
2022-11-09 15:57   ` Ard Biesheuvel
2022-11-09 15:57     ` Ard Biesheuvel
2022-11-09 16:30     ` Alexandru Elisei
2022-11-09 16:30       ` Alexandru Elisei
2022-11-09 17:26       ` Ard Biesheuvel
2022-11-09 17:26         ` Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221108151509.2250968-1-ardb@kernel.org \
    --to=ardb@kernel.org \
    --cc=alexandru.elisei@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.