[PATCH 0/2] UEFI panic notification mechanism

[PATCH 0/2] UEFI panic notification mechanism

[Guilherme G. Piccoli]

Hi folks, this patch set is about a mechanism to notify the UEFI
firmware about a panic event in the kernel, so the firmware is
enabled to take some action. The specific use case for us is to
show an alternative splash screen if a panic happened.
The base for the idea is to explore the panic notifiers mechanism,
that allows a callback to execute in the panic path.

Patch 1 is kind of a "clean-up" in a way; just taking a helper out
of efibc and adding it on generic efi.h header, so we have common
code used by 3 modules (efibc, efi-pstore and efi-panic).

Patch 2 is the efi-panic module itself; it is *strongly* based on
efibc, and for that I'd like to hereby thank to the authors.
The efibc code is very clear!


Some design decisions to be discussed:

(1) The variable name and value - I called it PanicWarn, and the
data it holds is just a byte, set by default to 0xFF (though users
can change that via module parameter). I have no attachment to
these, we can improve naming and the size of the data for example,
suggestions are appreciated!


(2) The 3 modules (efibc, efi-pstore and efi-panic) share a lot of
ideas or code; both efi-{pstore,panic} deal with panic events, and
both efi{bc,-panic} rely on notifiers and share code.
Should we unify some of them or anything like that? It seemed to me
the proper approach would be a simple and small standalone module,
but I'm open for suggestions.


(3) I've noticed a behavior that also happens in efi-pstore, I'm not
sure if that's something to care about. In the efi-panic module, after
a fresh boot, we check if PanicWarn is there an if so, we print a
message and _delete_ that variable from the NVRAM. But...the variable
is still present in sysfs, in the list created by efivars. Same happens
with efi-pstore, if we delete the dumps generated from /sys/fs/pstore.

In my case, I've used the __efivar_entry_delete() variant, so it doesn't
delete from any list, only from the firmware area. Should this be handled?
Or we just don't care with the empty variable reference in the sysfs tree?


I appreciate feedbacks and suggestions, thanks in advance for the attention!
Cheers,


Guilherme


Guilherme G. Piccoli (2):
  efi: Add a generic helper to convert strings to unicode
  efi-panic: Introduce the UEFI panic notification module

 drivers/firmware/efi/Kconfig      | 10 ++++
 drivers/firmware/efi/Makefile     |  1 +
 drivers/firmware/efi/efi-panic.c  | 97 +++++++++++++++++++++++++++++++
 drivers/firmware/efi/efi-pstore.c |  5 +-
 drivers/firmware/efi/efibc.c      | 16 ++---
 include/linux/efi.h               | 16 +++++
 6 files changed, 130 insertions(+), 15 deletions(-)
 create mode 100644 drivers/firmware/efi/efi-panic.c

-- 
2.36.0

[PATCH 1/2] efi: Add a generic helper to convert strings to unicode

[Guilherme G. Piccoli]

Currently both efi-pstore and efibc rely in simple for-loops
to convert from regular char pointers to u16/unicode strings;
efibc has even a nice helper to perform such work.

So, let's export this helper to common EFI code to prevent
code duplication (like in efi-pstore); this helper will also
be used in a subsequent patch (adding a new module).

Notice that efi-pstore didn't write the end NULL char in the
unicode string before this patch, but this should not change
anything. No functional change is expected here.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 drivers/firmware/efi/efi-pstore.c |  5 ++---
 drivers/firmware/efi/efibc.c      | 16 ++++------------
 include/linux/efi.h               | 15 +++++++++++++++
 3 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 7e771c56c13c..299116ecfb4e 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -249,7 +249,7 @@ static int efi_pstore_write(struct pstore_record *record)
 	char name[DUMP_NAME_LEN];
 	efi_char16_t efi_name[DUMP_NAME_LEN];
 	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
-	int i, ret = 0;
+	int ret = 0;
 
 	record->id = generic_id(record->time.tv_sec, record->part,
 				record->count);
@@ -262,8 +262,7 @@ static int efi_pstore_write(struct pstore_record *record)
 		 (long long)record->time.tv_sec,
 		 record->compressed ? 'C' : 'D');
 
-	for (i = 0; i < DUMP_NAME_LEN; i++)
-		efi_name[i] = name[i];
+	efi_str8_to_str16(efi_name, name, DUMP_NAME_LEN - 1);
 
 	ret = efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
 			      false, record->size, record->psi->buf);
diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
index 15a47539dc56..63fe2bf753cb 100644
--- a/drivers/firmware/efi/efibc.c
+++ b/drivers/firmware/efi/efibc.c
@@ -11,16 +11,6 @@
 #include <linux/reboot.h>
 #include <linux/slab.h>
 
-static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
-{
-	size_t i;
-
-	for (i = 0; i < strlen(str); i++)
-		str16[i] = str[i];
-
-	str16[i] = '\0';
-}
-
 static int efibc_set_variable(const char *name, const char *value)
 {
 	int ret;
@@ -39,8 +29,10 @@ static int efibc_set_variable(const char *name, const char *value)
 		return -ENOMEM;
 	}
 
-	efibc_str_to_str16(name, entry->var.VariableName);
-	efibc_str_to_str16(value, (efi_char16_t *)entry->var.Data);
+	efi_str8_to_str16(entry->var.VariableName, name, strlen(name));
+	efi_str8_to_str16((efi_char16_t *)entry->var.Data, value,
+			  strlen(value));
+
 	memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
 
 	ret = efivar_entry_set_safe(entry->var.VariableName,
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ccd4d3f91c98..066ebc5bcb2a 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1030,6 +1030,21 @@ efivar_unregister(struct efivar_entry *var)
 	kobject_put(&var->kobj);
 }
 
+/*
+ * Helper that converts regular char buffer to u16 unicode-like strings;
+ * notice that the unicode buffer requires to be at least len+1 in size.
+ */
+static inline void
+efi_str8_to_str16(efi_char16_t *str16, const char *str8, size_t len)
+{
+	size_t i;
+
+	for (i = 0; i < len; i++)
+		str16[i] = str8[i];
+
+	str16[i] = '\0';
+}
+
 int efivars_register(struct efivars *efivars,
 		     const struct efivar_operations *ops,
 		     struct kobject *kobject);
-- 
2.36.0

[PATCH 2/2] efi-panic: Introduce the UEFI panic notification module

[Guilherme G. Piccoli]

Despite we have pstore, kdump and other panic-time  mechanisms,
there's no generic way to let the firmware know a panic event
happened. Some hypervisors might have special code for that, but
that's not generic.

The UEFI panic notification module hereby proposed aims to fill
this need: once we face a panic, through the panic notifier
infrastructure we set a UEFI variable named PanicWarn with a
value - default is 0xFF, but it's adjustable via module parameter.
The firmware is then enabled to take a proper action.

The module also let the users know that last execution likely
panicked if the UEFI variable is present on module load - then
it clears that to avoid confusing users, only the last panic
event is recorded.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 drivers/firmware/efi/Kconfig     | 10 ++++
 drivers/firmware/efi/Makefile    |  1 +
 drivers/firmware/efi/efi-panic.c | 97 ++++++++++++++++++++++++++++++++
 include/linux/efi.h              |  1 +
 4 files changed, 109 insertions(+)
 create mode 100644 drivers/firmware/efi/efi-panic.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 2c3dac5ecb36..12f2d963333e 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -145,6 +145,16 @@ config EFI_BOOTLOADER_CONTROL
 	  bootloader reads this reboot reason and takes particular
 	  action according to its policy.
 
+config EFI_PANIC_NOTIFIER
+	tristate "UEFI Panic Notifier"
+	depends on EFI
+	default n
+	help
+	  With this module, kernel creates the UEFI variable "PanicWarn" if
+	  the system faces a panic event. With that, the firmware is entitled
+	  to take an action if the last kernel panicked; it also shows a
+	  message during boot time if last run faced a panic event.
+
 config EFI_CAPSULE_LOADER
 	tristate "EFI capsule loader"
 	depends on EFI && !IA64
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index c02ff25dd477..abbc7d9af2da 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
 subdir-$(CONFIG_EFI_STUB)		+= libstub
 obj-$(CONFIG_EFI_FAKE_MEMMAP)		+= fake_map.o
 obj-$(CONFIG_EFI_BOOTLOADER_CONTROL)	+= efibc.o
+obj-$(CONFIG_EFI_PANIC_NOTIFIER)	+= efi-panic.o
 obj-$(CONFIG_EFI_TEST)			+= test/
 obj-$(CONFIG_EFI_DEV_PATH_PARSER)	+= dev-path-parser.o
 obj-$(CONFIG_APPLE_PROPERTIES)		+= apple-properties.o
diff --git a/drivers/firmware/efi/efi-panic.c b/drivers/firmware/efi/efi-panic.c
new file mode 100644
index 000000000000..c59655e22fc7
--- /dev/null
+++ b/drivers/firmware/efi/efi-panic.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * efi-panic: UEFI panic notification mechanism
+ */
+
+#define pr_fmt(fmt) "efi-panic: " fmt
+
+#include <linux/efi.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/panic_notifier.h>
+
+#define EFI_PANIC_ATTR		(EFI_VARIABLE_NON_VOLATILE |\
+				 EFI_VARIABLE_BOOTSERVICE_ACCESS |\
+				 EFI_VARIABLE_RUNTIME_ACCESS)
+
+#define EFI_DATA_SIZE		(2 * sizeof(efi_char16_t))
+
+static struct efivar_entry *entry;
+
+static u8 panic_warn_val = 0xFF;
+module_param(panic_warn_val, byte, 0644);
+MODULE_PARM_DESC(panic_warn_val,
+		 "Value written to PanicWarn efivar on panic event [default=0xFF]");
+
+static int efi_panic_cb(struct notifier_block *nb, unsigned long ev,
+			void *unused)
+{
+	int ret;
+
+	efi_str8_to_str16((efi_char16_t *)entry->var.Data, &panic_warn_val, 1);
+	ret = efivar_entry_set_safe(entry->var.VariableName,
+				    entry->var.VendorGuid,
+				    EFI_PANIC_ATTR, false, EFI_DATA_SIZE,
+				    entry->var.Data);
+	if (ret)
+		pr_err("failed to notify panic to UEFI\n");
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block efi_panic_notifier = {
+	.notifier_call = efi_panic_cb,
+};
+
+static int __init efi_panic_init(void)
+{
+	efi_guid_t guid = LINUX_EFI_PANIC_WARN_GUID;
+	unsigned long sz = EFI_DATA_SIZE;
+	const char *name = "PanicWarn";
+	u8 data[EFI_DATA_SIZE];
+	int err;
+
+	if (!efivars_kobject() || !efivar_supports_writes()) {
+		pr_err("UEFI vars not available (or not writable)\n");
+		return -ENODEV;
+	}
+
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+
+	memcpy(&entry->var.VendorGuid, &guid, sizeof(guid));
+	efi_str8_to_str16(entry->var.VariableName, name, strlen(name));
+
+	err = efivar_entry_get(entry, NULL, &sz, (void *)data);
+
+	if (!err) {
+		pr_info("previous kernel (likely) had a panic\n");
+
+		if (__efivar_entry_delete(entry))
+			pr_err("cannot delete %s UEFI variable\n", name);
+
+		memset(entry->var.Data, 0, EFI_DATA_SIZE);
+	} else {
+		if (err != -ENOENT)
+			pr_err("can't read the UEFI variables (err=%d)\n", err);
+	}
+
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &efi_panic_notifier);
+
+	return 0;
+}
+module_init(efi_panic_init);
+
+static void __exit efi_panic_exit(void)
+{
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &efi_panic_notifier);
+	kfree(entry);
+}
+module_exit(efi_panic_exit);
+
+MODULE_AUTHOR("Guilherme G. Piccoli <gpiccoli@igalia.com>");
+MODULE_DESCRIPTION("UEFI Panic Notification Mechanism");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 066ebc5bcb2a..4219f3d44183 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -365,6 +365,7 @@ void efi_native_runtime_setup(void);
 #define EFI_GLOBAL_VARIABLE_GUID		EFI_GUID(0x8be4df61, 0x93ca, 0x11d2,  0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, 0x2b, 0x8c)
 #define UV_SYSTEM_TABLE_GUID			EFI_GUID(0x3b13a7d4, 0x633e, 0x11dd,  0x93, 0xec, 0xda, 0x25, 0x56, 0xd8, 0x95, 0x93)
 #define LINUX_EFI_CRASH_GUID			EFI_GUID(0xcfc8fc79, 0xbe2e, 0x4ddc,  0x97, 0xf0, 0x9f, 0x98, 0xbf, 0xe2, 0x98, 0xa0)
+#define LINUX_EFI_PANIC_WARN_GUID		EFI_GUID(0x9e85b665, 0x08d4, 0x42c9,  0x83, 0x24, 0x47, 0xed, 0x5f, 0xe5, 0x0b, 0xf3)
 #define LOADED_IMAGE_PROTOCOL_GUID		EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2,  0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
 #define EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID	EFI_GUID(0x9042a9de, 0x23dc, 0x4a38,  0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 0x51, 0x6a)
 #define EFI_UGA_PROTOCOL_GUID			EFI_GUID(0x982c298b, 0xf4fa, 0x41cb,  0xb8, 0x38, 0x77, 0xaa, 0x68, 0x8f, 0xb8, 0x39)
-- 
2.36.0

Re: [PATCH 0/2] UEFI panic notification mechanism

[Guilherme G. Piccoli]

Hi Ard, apologies for annoying!

Just a friendly ping asking if you have any opinions about these patches.

Thanks in advance! Cheers,


Guilherme

Re: [PATCH 0/2] UEFI panic notification mechanism

[Ard Biesheuvel]

On Thu, 2 Jun 2022 at 19:40, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>
> Hi Ard, apologies for annoying!
>

No worries, just very busy :-)

> Just a friendly ping asking if you have any opinions about these patches.
>

Honestly, I'm not sure I see the value of this. You want to 'notify
the UEFI firmware' but the firmware doesn't really care about these
variables, only your bespoke tooling does. EFI pstore captures far
more data, so if you just wipe /sys/fs/pstore after each clean boot,
you already have all the data you need, no?

Also, I'm in the process of removing the public efivar_entry_xxx() API
- please look at the efi/next tree for a peek. This is related to your
point 3), i.e., the efivar layer is a disaster in terms of consistency
between different observers of the EFI variable store. Switching to
efivar_set_variable() [the new API] should fix that.

Re: [PATCH 0/2] UEFI panic notification mechanism

[Guilherme G. Piccoli]

On 28/06/2022 09:49, Ard Biesheuvel wrote:
> On Thu, 2 Jun 2022 at 19:40, Guilherme G. Piccoli <gpiccoli@igalia.com> wrote:
>>
>> Hi Ard, apologies for annoying!
>>
> 
> No worries, just very busy :-)
> 
>> Just a friendly ping asking if you have any opinions about these patches.
>>
> 
> Honestly, I'm not sure I see the value of this. You want to 'notify
> the UEFI firmware' but the firmware doesn't really care about these
> variables, only your bespoke tooling does. EFI pstore captures far
> more data, so if you just wipe /sys/fs/pstore after each clean boot,
> you already have all the data you need, no?
> 
> Also, I'm in the process of removing the public efivar_entry_xxx() API
> - please look at the efi/next tree for a peek. This is related to your
> point 3), i.e., the efivar layer is a disaster in terms of consistency
> between different observers of the EFI variable store. Switching to
> efivar_set_variable() [the new API] should fix that.

Hi Ard, thanks a lot for your review! Lemme split my points in some
bullets below:

a) What about patch 1, is it good as-is?


b) Cool about efivar_set_variable(), could fix that in V2.


c) Now, to the most relevant thing, the value of this. I might be
mistaken, but is there any known way to let UEFI know a panic happened?
For now, maybe only my UEFI fw might use that (and the usage is very
nice, showing a panic logo), but this opens a myriad of potential uses.
We can think in RAM preserving mechanism conditional to panic scenarios,
special resets for panic vs. cold boot, and even maybe a firmware vmcore
capturing.

If there is any other way to let UEFI know about a panic, let me know
and that will likely be more than enough for me. Otherwise, do you think
such small code would be a big burden to carry, considering the
cost/benefit for my use case?

Thanks in advance for your analysis.
Cheers,


Guilherme