[PATCH v2 00/16] Signature verification of hibernate snapshot

[PATCH v2 00/16] Signature verification of hibernate snapshot

[Lee, Chun-Yi]

Hi experts,

This patchset is the implementation of signature verification of hibernate
snapshot image. The origin idea is from Jiri Kosina: Let EFI bootloader
generate key-pair in UEFI secure boot environment, then forward it to kernel
for sign/verify hibernate image.

The first patchset for this function was sent in Sep. 2013, the implementation
is base on PKI. This new patchset is base on HMAC-SHA1.

The hibernate function provided by kernel was used to snapshot memory
to be a image for keeping in storage, then restored in appropriate time.
There have potential threat from hacking the memory snapshot image.
Cracker may triggers hibernating process through ioctl to grab snapshot
image, then restoring modified image back to memory. Another situation
is booting to other hacked OS to modify the snapshot image in swap
partition or file, then user may runs malware after image restored to
memory. In addition, the above weakness cause kernel is not fully trusted
in EFI secure boot environment.

So, kernel hibernate function needs a mechanism to verify integrity of
hibernate snapshot image.

For signing hibernate image, kernel need a key for generating signature of
image. The origin idea is using PKI, the EFI bootloader, shim generates key
pair and forward to boot kernel for signing/verifying image. In Linux Plumbers
Conference 2013, we got response from community experts for just using
symmetric key algorithm to generate signature, that's simpler and no EFI
bootloader's involving.

Current solution is using HMAC-SHA1 algorithm, it generating HMAC key in EFI
stub, the HMAC key stored in efi boot service variable, When hibernate
recovering, kernel will verify the image signature before switch whole system
to image kernel and image memory space. When verifying failed, kernel is
tainted or stop recovering and discarding image.

Set HIBERNATE_VERIFICATION compile option to true for enabling hibernate
verification. The default behavior of verifying failed is accept restoring
image but tainting kernel with H taint flag. Using HIBERNATE_VERIFICATION_FORCE
kernel compile option or "sigenforce" kernel parameter to force hibernate
recovery process stop when verification failed. It allows user to trigger the
key re-generating process in EFI stub through SNAPSHOT_REGENERATE_KEY ioctl.

v2:
 - Replaced all SWSUSP naming with HIBERNATION.
 - Moved swsusp_info structure definition only for CONFIG_HIBERNATION.
 - Fixed typo in patch subject and description.
 - Changed name of i8254() to read_i8254()
 - Removed all efi_printk log in efi_random.c
 - Changed the size of key array to be unsigned in efi_random.c
 - Avoid calling cpuid many times.
 - Add line breaks to error log in efi_random.c
 - Removed free_handle label in efi_random.c
 - Moved efi_status_to_str() to efi_random.c, and reduce code duplication.
 - Set rng_handle = NULL in efi_locate_rng()
 - Changed u32 random to bool in efi_rng_supported()
 - Using EFI status codes explicitly.
 - Modified Copyright declaration.
 - Moved set_hibernation_key_regen_flag to user.c


Lee, Chun-Yi (16):
  PM / hibernate: define HMAC algorithm and digest size of hibernation
  x86/efi: Add get and set variable to EFI services pointer table
  x86/boot: Public getting random boot function
  x86/efi: Generating random number in EFI stub
  x86/efi: Get entropy through EFI random number generator protocol
  x86/efi: Generating random HMAC key for siging hibernate image
  efi: Make efi_status_to_err() public
  x86/efi: Carrying hibernation key by setup data
  PM / hibernate: Reserve hibernation key and erase footprints
  PM / hibernate: Generate and verify signature of hibernate snapshot
  PM / hibernate: Avoid including hibernation key to hibernate image
  PM / hibernate: Forward signature verifying result and key to image
    kernel
  PM / hibernate: Add configuration to enforce signature verification
  PM / hibernate: Allow user trigger hibernation key re-generating
  PM / hibernate: Bypass verification logic on legacy BIOS
  PM / hibernate: Document signature verification of hibernate snapshot

 Documentation/kernel-parameters.txt             |   5 +
 Documentation/power/swsusp-signature-verify.txt |  86 +++++++
 arch/x86/boot/compressed/Makefile               |   1 +
 arch/x86/boot/compressed/aslr.c                 |  57 +----
 arch/x86/boot/compressed/eboot.c                |  97 ++++++++
 arch/x86/boot/compressed/efi_random.c           | 289 +++++++++++++++++++++++
 arch/x86/boot/compressed/head_32.S              |   6 +-
 arch/x86/boot/compressed/head_64.S              |   8 +-
 arch/x86/boot/compressed/misc.c                 |  55 +++++
 arch/x86/boot/compressed/misc.h                 |   4 +
 arch/x86/include/asm/efi.h                      |   2 +
 arch/x86/include/asm/suspend.h                  |  13 ++
 arch/x86/include/uapi/asm/bootparam.h           |   1 +
 arch/x86/kernel/setup.c                         |  21 +-
 arch/x86/power/Makefile                         |   1 +
 arch/x86/power/hibernate_keys.c                 | 173 ++++++++++++++
 drivers/firmware/Makefile                       |   1 +
 drivers/firmware/efi/Kconfig                    |   4 +
 drivers/firmware/efi/Makefile                   |   1 +
 drivers/firmware/efi/efi-hibernate_keys.c       |  42 ++++
 drivers/firmware/efi/vars.c                     |  33 ---
 include/linux/efi.h                             |  46 ++++
 include/linux/kernel.h                          |   1 +
 include/linux/suspend.h                         |  27 +++
 include/uapi/linux/suspend_ioctls.h             |   3 +-
 kernel/panic.c                                  |   2 +
 kernel/power/Kconfig                            |  23 ++
 kernel/power/hibernate.c                        |  10 +
 kernel/power/power.h                            |  22 +-
 kernel/power/snapshot.c                         | 293 ++++++++++++++++++++++--
 kernel/power/swap.c                             |   4 +
 kernel/power/user.c                             |  19 ++
 kernel/reboot.c                                 |   3 +
 33 files changed, 1240 insertions(+), 113 deletions(-)
 create mode 100644 Documentation/power/swsusp-signature-verify.txt
 create mode 100644 arch/x86/boot/compressed/efi_random.c
 create mode 100644 arch/x86/power/hibernate_keys.c
 create mode 100644 drivers/firmware/efi/efi-hibernate_keys.c

-- 
2.1.4

[PATCH v2 01/16] PM / hibernate: define HMAC algorithm and digest size of hibernation

[Lee, Chun-Yi]

Using HMAC-SHA1 to be the HMAC algorithm of signing hibernate
snapshot image. The digest size of HMAC-SHA1 is 160 bits (20 bytes),
this size will be also applied to the length of HMAC key.

In addition, moved swsusp_info struct definition into CONFIG_HIBERNATION
ifdef block because only hibernate code uses it.
Add HIBERNATE_VERIFICATION kernel config for using by later hibernate
signature verification code.

Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 include/linux/suspend.h |  5 +++++
 kernel/power/Kconfig    | 13 +++++++++++++
 kernel/power/power.h    |  3 ++-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 5efe743..56c6de9 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -327,6 +327,11 @@ struct platform_hibernation_ops {
 };
 
 #ifdef CONFIG_HIBERNATION
+
+/* HMAC Algorithm of Hibernate Signature */
+#define HIBERNATION_HMAC	"hmac(sha1)"
+#define HIBERNATION_DIGEST_SIZE	20
+
 /* kernel/power/snapshot.c */
 extern void __register_nosave_region(unsigned long b, unsigned long e, int km);
 static inline void __init register_nosave_region(unsigned long b, unsigned long e)
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 9e30231..8608b3b 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -66,6 +66,19 @@ config HIBERNATION
 
 	  For more information take a look at <file:Documentation/power/swsusp.txt>.
 
+config HIBERNATE_VERIFICATION
+	bool "Hibernate verification"
+	depends on HIBERNATION
+	depends on EFI_STUB
+	depends on X86
+	select CRYPTO_HMAC
+	select CRYPTO_SHA1
+	help
+	  This option provides support for generating and verifying the
+	  signature of memory snapshot image by HMAC-SHA1. Current mechanism
+	  relies on UEFI secure boot environment, EFI stub generates HMAC
+	  key for hibernate verification.
+
 config ARCH_SAVE_PAGE_KEYS
 	bool
 
diff --git a/kernel/power/power.h b/kernel/power/power.h
index caadb56..6ea5c78 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -4,6 +4,7 @@
 #include <linux/freezer.h>
 #include <linux/compiler.h>
 
+#ifdef CONFIG_HIBERNATION
 struct swsusp_info {
 	struct new_utsname	uts;
 	u32			version_code;
@@ -12,9 +13,9 @@ struct swsusp_info {
 	unsigned long		image_pages;
 	unsigned long		pages;
 	unsigned long		size;
+	u8                      signature[HIBERNATION_DIGEST_SIZE];
 } __aligned(PAGE_SIZE);
 
-#ifdef CONFIG_HIBERNATION
 /* kernel/power/snapshot.c */
 extern void __init hibernate_reserved_size_init(void);
 extern void __init hibernate_image_size_init(void);
-- 
2.1.4

[PATCH v2 02/16] x86/efi: Add get and set variable to EFI services pointer table

[Lee, Chun-Yi]

Add get variable and set variable function to EFI services pointer
table for supporting later functions of hibernate signature
verification to keep the HMAC key in efi boot service variable.
EFI boot stub needs get/set_variable functions for accessing key.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 arch/x86/boot/compressed/eboot.c   | 4 ++++
 arch/x86/boot/compressed/head_32.S | 6 +++---
 arch/x86/boot/compressed/head_64.S | 8 ++++----
 arch/x86/include/asm/efi.h         | 2 ++
 4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 2c82bd1..0ffb6db 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -30,12 +30,14 @@ static void setup_boot_services##bits(struct efi_config *c)		\
 {									\
 	efi_system_table_##bits##_t *table;				\
 	efi_boot_services_##bits##_t *bt;				\
+	efi_runtime_services_##bits##_t *rt;				\
 									\
 	table = (typeof(table))sys_table;				\
 									\
 	c->text_output = table->con_out;				\
 									\
 	bt = (typeof(bt))(unsigned long)(table->boottime);		\
+	rt = (typeof(rt))(unsigned long)(table->runtime);		\
 									\
 	c->allocate_pool = bt->allocate_pool;				\
 	c->allocate_pages = bt->allocate_pages;				\
@@ -45,6 +47,8 @@ static void setup_boot_services##bits(struct efi_config *c)		\
 	c->locate_handle = bt->locate_handle;				\
 	c->handle_protocol = bt->handle_protocol;			\
 	c->exit_boot_services = bt->exit_boot_services;			\
+	c->get_variable = rt->get_variable;				\
+	c->set_variable = rt->set_variable;				\
 }
 BOOT_SERVICES(32);
 BOOT_SERVICES(64);
diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index 8ef964d..a7db5e3 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -54,7 +54,7 @@ ENTRY(efi_pe_entry)
 
 	/* Relocate efi_config->call() */
 	leal	efi32_config(%esi), %eax
-	add	%esi, 88(%eax)
+	add	%esi, 104(%eax)
 	pushl	%eax
 
 	call	make_boot_params
@@ -80,7 +80,7 @@ ENTRY(efi32_stub_entry)
 
 	/* Relocate efi_config->call() */
 	leal	efi32_config(%esi), %eax
-	add	%esi, 88(%eax)
+	add	%esi, 104(%eax)
 	pushl	%eax
 2:
 	call	efi_main
@@ -230,7 +230,7 @@ relocated:
 #ifdef CONFIG_EFI_STUB
 	.data
 efi32_config:
-	.fill 11,8,0
+	.fill 13,8,0
 	.long efi_call_phys
 	.long 0
 	.byte 0
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index b0c0d16..471b1c1 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -255,7 +255,7 @@ ENTRY(efi_pe_entry)
 	/*
 	 * Relocate efi_config->call().
 	 */
-	addq	%rbp, efi64_config+88(%rip)
+	addq	%rbp, efi64_config+104(%rip)
 
 	movq	%rax, %rdi
 	call	make_boot_params
@@ -275,7 +275,7 @@ handover_entry:
 	 * Relocate efi_config->call().
 	 */
 	movq	efi_config(%rip), %rax
-	addq	%rbp, 88(%rax)
+	addq	%rbp, 104(%rax)
 2:
 	movq	efi_config(%rip), %rdi
 	call	efi_main
@@ -448,14 +448,14 @@ efi_config:
 #ifdef CONFIG_EFI_MIXED
 	.global efi32_config
 efi32_config:
-	.fill	11,8,0
+	.fill	13,8,0
 	.quad	efi64_thunk
 	.byte	0
 #endif
 
 	.global efi64_config
 efi64_config:
-	.fill	11,8,0
+	.fill	13,8,0
 	.quad	efi_call
 	.byte	1
 #endif /* CONFIG_EFI_STUB */
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 155162e..a274aa8 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -175,6 +175,8 @@ struct efi_config {
 	u64 handle_protocol;
 	u64 exit_boot_services;
 	u64 text_output;
+	u64 get_variable;
+	u64 set_variable;
 	efi_status_t (*call)(unsigned long, ...);
 	bool is64;
 } __packed;
-- 
2.1.4

[PATCH v2 03/16] x86/boot: Public getting random boot function

[Lee, Chun-Yi]

This patch moves the getting random boot function from aslr to misc
for later used by EFI stub to generate the first entropy of hmac key
for signing hibernate snapshot image.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 arch/x86/boot/compressed/aslr.c | 57 ++---------------------------------------
 arch/x86/boot/compressed/misc.c | 55 +++++++++++++++++++++++++++++++++++++++
 arch/x86/boot/compressed/misc.h |  4 +++
 3 files changed, 61 insertions(+), 55 deletions(-)

diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
index d7b1f65..d559d07 100644
--- a/arch/x86/boot/compressed/aslr.c
+++ b/arch/x86/boot/compressed/aslr.c
@@ -6,59 +6,6 @@
 
 #include <generated/compile.h>
 #include <linux/module.h>
-#include <linux/uts.h>
-#include <linux/utsname.h>
-#include <generated/utsrelease.h>
-
-/* Simplified build-specific string for starting entropy. */
-static const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
-		LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION;
-
-#define I8254_PORT_CONTROL	0x43
-#define I8254_PORT_COUNTER0	0x40
-#define I8254_CMD_READBACK	0xC0
-#define I8254_SELECT_COUNTER0	0x02
-#define I8254_STATUS_NOTREADY	0x40
-static inline u16 i8254(void)
-{
-	u16 status, timer;
-
-	do {
-		outb(I8254_PORT_CONTROL,
-		     I8254_CMD_READBACK | I8254_SELECT_COUNTER0);
-		status = inb(I8254_PORT_COUNTER0);
-		timer  = inb(I8254_PORT_COUNTER0);
-		timer |= inb(I8254_PORT_COUNTER0) << 8;
-	} while (status & I8254_STATUS_NOTREADY);
-
-	return timer;
-}
-
-static unsigned long rotate_xor(unsigned long hash, const void *area,
-				size_t size)
-{
-	size_t i;
-	unsigned long *ptr = (unsigned long *)area;
-
-	for (i = 0; i < size / sizeof(hash); i++) {
-		/* Rotate by odd number of bits and XOR. */
-		hash = (hash << ((sizeof(hash) * 8) - 7)) | (hash >> 7);
-		hash ^= ptr[i];
-	}
-
-	return hash;
-}
-
-/* Attempt to create a simple but unpredictable starting entropy. */
-static unsigned long get_random_boot(void)
-{
-	unsigned long hash = 0;
-
-	hash = rotate_xor(hash, build_str, sizeof(build_str));
-	hash = rotate_xor(hash, real_mode, sizeof(*real_mode));
-
-	return hash;
-}
 
 static unsigned long get_random_long(void)
 {
@@ -67,7 +14,7 @@ static unsigned long get_random_long(void)
 #else
 	const unsigned long mix_const = 0x3f39e593UL;
 #endif
-	unsigned long raw, random = get_random_boot();
+	unsigned long raw, random = get_random_boot(real_mode);
 	bool use_i8254 = true;
 
 	debug_putstr("KASLR using");
@@ -90,7 +37,7 @@ static unsigned long get_random_long(void)
 
 	if (use_i8254) {
 		debug_putstr(" i8254");
-		random ^= i8254();
+		random ^= read_i8254();
 	}
 
 	/* Circular multiply for better bit diffusion */
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index a107b93..70acd7e 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -12,6 +12,9 @@
 #include "misc.h"
 #include "../string.h"
 
+#include <generated/compile.h>
+#include <generated/utsrelease.h>
+
 /* WARNING!!
  * This code is compiled with -fPIC and it is relocated dynamically
  * at run time, but no relocation processing is performed.
@@ -435,3 +438,55 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
 	debug_putstr("done.\nBooting the kernel.\n");
 	return output;
 }
+
+#if CONFIG_RANDOMIZE_BASE
+#define I8254_PORT_CONTROL	0x43
+#define I8254_PORT_COUNTER0	0x40
+#define I8254_CMD_READBACK	0xC0
+#define I8254_SELECT_COUNTER0	0x02
+#define I8254_STATUS_NOTREADY	0x40
+u16 read_i8254(void)
+{
+	u16 status, timer;
+
+	do {
+		outb(I8254_PORT_CONTROL,
+		     I8254_CMD_READBACK | I8254_SELECT_COUNTER0);
+		status = inb(I8254_PORT_COUNTER0);
+		timer  = inb(I8254_PORT_COUNTER0);
+		timer |= inb(I8254_PORT_COUNTER0) << 8;
+	} while (status & I8254_STATUS_NOTREADY);
+
+	return timer;
+}
+
+static unsigned long rotate_xor(unsigned long hash, const void *area,
+				size_t size)
+{
+	size_t i;
+	unsigned long *ptr = (unsigned long *)area;
+
+	for (i = 0; i < size / sizeof(hash); i++) {
+		/* Rotate by odd number of bits and XOR. */
+		hash = (hash << ((sizeof(hash) * 8) - 7)) | (hash >> 7);
+		hash ^= ptr[i];
+	}
+
+	return hash;
+}
+
+/* Simplified build-specific string for starting entropy. */
+static const char build_str[] = UTS_RELEASE " (" LINUX_COMPILE_BY "@"
+		LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION;
+
+/* Attempt to create a simple but unpredictable starting entropy. */
+unsigned long get_random_boot(struct boot_params *boot_params)
+{
+	unsigned long hash = 0;
+
+	hash = rotate_xor(hash, build_str, sizeof(build_str));
+	hash = rotate_xor(hash, boot_params, sizeof(*boot_params));
+
+	return hash;
+}
+#endif /* CONFIG_RANDOMIZE_BASE */
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 805d25c..60e4893 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -53,6 +53,10 @@ int cmdline_find_option(const char *option, char *buffer, int bufsize);
 int cmdline_find_option_bool(const char *option);
 #endif
 
+#if CONFIG_RANDOMIZE_BASE
+extern u16 read_i8254(void);
+extern unsigned long get_random_boot(struct boot_params *boot_params);
+#endif
 
 #if CONFIG_RANDOMIZE_BASE
 /* aslr.c */
-- 
2.1.4

[PATCH v2 04/16] x86/efi: Generating random number in EFI stub

[Lee, Chun-Yi]

This patch adds the codes for generating random number array as the
HMAC key that will used by later EFI stub codes.

The original codes in efi_random copied from aslr and add the codes
to accept input entropy and EFI debugging. In later patch will add
the codes to get random number by EFI protocol. The separate codes
can avoid impacting aslr function.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 arch/x86/boot/compressed/Makefile     |  1 +
 arch/x86/boot/compressed/efi_random.c | 80 +++++++++++++++++++++++++++++++++++
 arch/x86/boot/compressed/misc.c       |  4 +-
 arch/x86/boot/compressed/misc.h       |  2 +-
 4 files changed, 84 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/boot/compressed/efi_random.c

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 0a291cd..377245b 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -49,6 +49,7 @@ vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/aslr.o
 
 $(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone
 
+vmlinux-objs-$(CONFIG_HIBERNATE_VERIFICATION) += $(obj)/efi_random.o
 vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \
 	$(objtree)/drivers/firmware/efi/libstub/lib.a
 vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
diff --git a/arch/x86/boot/compressed/efi_random.c b/arch/x86/boot/compressed/efi_random.c
new file mode 100644
index 0000000..a69352e
--- /dev/null
+++ b/arch/x86/boot/compressed/efi_random.c
@@ -0,0 +1,80 @@
+#include "misc.h"
+
+#include <linux/efi.h>
+#include <asm/archrandom.h>
+
+#define EDX_TSC		(1 << 4)
+#define ECX_RDRAND	(1 << 30)
+
+static unsigned int cpuid_0x1_ecx, cpuid_0x1_edx;
+
+static void cpuid_ecx_edx(void)
+{
+	unsigned int eax, ebx;
+
+	cpuid(0x1, &eax, &ebx, &cpuid_0x1_ecx, &cpuid_0x1_edx);
+}
+
+static unsigned long get_random_long(unsigned long entropy,
+				     struct boot_params *boot_params,
+				     efi_system_table_t *sys_table)
+{
+#ifdef CONFIG_X86_64
+	const unsigned long mix_const = 0x5d6008cbf3848dd3UL;
+#else
+	const unsigned long mix_const = 0x3f39e593UL;
+#endif
+	unsigned long raw, random;
+	bool use_i8254 = true;
+
+	if (entropy)
+		random = entropy;
+	else
+		random = get_random_boot(boot_params);
+
+	if (cpuid_0x1_ecx & ECX_RDRAND) {
+		if (rdrand_long(&raw)) {
+			random ^= raw;
+			use_i8254 = false;
+		}
+	}
+
+	if (cpuid_0x1_edx & EDX_TSC) {
+		rdtscll(raw);
+
+		random ^= raw;
+		use_i8254 = false;
+	}
+
+	if (use_i8254)
+		random ^= read_i8254();
+
+	/* Circular multiply for better bit diffusion */
+	asm("mul %3"
+	    : "=a" (random), "=d" (raw)
+	    : "a" (random), "rm" (mix_const));
+	random += raw;
+
+	return random;
+}
+
+void efi_get_random_key(efi_system_table_t *sys_table,
+			struct boot_params *params, u8 key[], unsigned int size)
+{
+	unsigned long entropy = 0;
+	unsigned int bfill = size;
+
+	if (key == NULL || !size)
+		return;
+
+	cpuid_ecx_edx();
+
+	memset(key, 0, size);
+	while (bfill > 0) {
+		unsigned int copy_len = 0;
+		entropy = get_random_long(entropy, params, sys_table);
+		copy_len = (bfill < sizeof(entropy)) ? bfill : sizeof(entropy);
+		memcpy((void *)(key + size - bfill), &entropy, copy_len);
+		bfill -= copy_len;
+	}
+}
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 70acd7e..c8e2237 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -439,7 +439,7 @@ asmlinkage __visible void *decompress_kernel(void *rmode, memptr heap,
 	return output;
 }
 
-#if CONFIG_RANDOMIZE_BASE
+#if CONFIG_HIBERNATE_VERIFICATION || CONFIG_RANDOMIZE_BASE
 #define I8254_PORT_CONTROL	0x43
 #define I8254_PORT_COUNTER0	0x40
 #define I8254_CMD_READBACK	0xC0
@@ -489,4 +489,4 @@ unsigned long get_random_boot(struct boot_params *boot_params)
 
 	return hash;
 }
-#endif /* CONFIG_RANDOMIZE_BASE */
+#endif /* CONFIG_HIBERNATE_VERIFICATION || CONFIG_RANDOMIZE_BASE */
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 60e4893..3508a6e 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -53,7 +53,7 @@ int cmdline_find_option(const char *option, char *buffer, int bufsize);
 int cmdline_find_option_bool(const char *option);
 #endif
 
-#if CONFIG_RANDOMIZE_BASE
+#if CONFIG_HIBERNATE_VERIFICATION || CONFIG_RANDOMIZE_BASE
 extern u16 read_i8254(void);
 extern unsigned long get_random_boot(struct boot_params *boot_params);
 #endif
-- 
2.1.4

[PATCH v2 05/16] x86/efi: Get entropy through EFI random number generator protocol

[Lee, Chun-Yi]

To grab random numbers through EFI protocol as one of the entropies
source of swsusp key, this patch adds the logic for accessing EFI RNG
(random number generator) protocol that's introduced since UEFI 2.4.

Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 arch/x86/boot/compressed/efi_random.c | 209 ++++++++++++++++++++++++++++++++++
 include/linux/efi.h                   |  13 +++
 2 files changed, 222 insertions(+)

diff --git a/arch/x86/boot/compressed/efi_random.c b/arch/x86/boot/compressed/efi_random.c
index a69352e..1d29e28 100644
--- a/arch/x86/boot/compressed/efi_random.c
+++ b/arch/x86/boot/compressed/efi_random.c
@@ -1,7 +1,209 @@
 #include "misc.h"
 
 #include <linux/efi.h>
+#include <linux/stringify.h>
 #include <asm/archrandom.h>
+#include <asm/efi.h>
+
+#define EFI_STATUS_STR(_status) \
+case EFI_##_status: \
+	return "EFI_" __stringify(_status);
+
+static char *efi_status_to_str(efi_status_t status)
+{
+	switch (status) {
+	EFI_STATUS_STR(SUCCESS)
+	EFI_STATUS_STR(INVALID_PARAMETER)
+	EFI_STATUS_STR(OUT_OF_RESOURCES)
+	EFI_STATUS_STR(DEVICE_ERROR)
+	EFI_STATUS_STR(WRITE_PROTECTED)
+	EFI_STATUS_STR(SECURITY_VIOLATION)
+	EFI_STATUS_STR(NOT_FOUND)
+	}
+
+	return "";
+}
+
+static efi_status_t efi_locate_rng(efi_system_table_t *sys_table,
+				   void ***rng_handle)
+{
+	efi_guid_t rng_proto = EFI_RNG_PROTOCOL_GUID;
+	unsigned long size = 0;
+	efi_status_t status;
+
+	*rng_handle = NULL;
+	status = efi_call_early(locate_handle,
+				EFI_LOCATE_BY_PROTOCOL,
+				&rng_proto, NULL, &size, *rng_handle);
+
+	if (status == EFI_BUFFER_TOO_SMALL) {
+		status = efi_call_early(allocate_pool,
+					EFI_LOADER_DATA,
+					size, (void **)rng_handle);
+
+		if (status != EFI_SUCCESS) {
+			efi_printk(sys_table, "Failed to alloc mem for rng_handle\n");
+			return status;
+		}
+
+		status = efi_call_early(locate_handle,
+					EFI_LOCATE_BY_PROTOCOL, &rng_proto,
+					NULL, &size, *rng_handle);
+	}
+
+	if (status != EFI_SUCCESS) {
+		efi_printk(sys_table, "Failed to locate EFI_RNG_PROTOCOL\n");
+		efi_call_early(free_pool, *rng_handle);
+	}
+
+	return status;
+}
+
+static bool efi_rng_supported32(efi_system_table_t *sys_table, void **rng_handle)
+{
+	const struct efi_config *efi_early = __efi_early();
+	efi_rng_protocol_32 *rng = NULL;
+	efi_guid_t rng_proto = EFI_RNG_PROTOCOL_GUID;
+	u32 *handles = (u32 *)(unsigned long)rng_handle;
+	unsigned long size = 0;
+	void **algorithmlist = NULL;
+	efi_status_t status;
+
+	status = efi_call_early(handle_protocol, handles[0],
+				&rng_proto, (void **)&rng);
+	if (status != EFI_SUCCESS)
+		efi_printk(sys_table, "Failed to get EFI_RNG_PROTOCOL handles\n");
+
+	if (status == EFI_SUCCESS && rng) {
+		status = efi_early->call((unsigned long)rng->get_info, rng,
+					&size, algorithmlist);
+		return (status == EFI_BUFFER_TOO_SMALL);
+	}
+
+	return false;
+}
+
+static bool efi_rng_supported64(efi_system_table_t *sys_table, void **rng_handle)
+{
+	const struct efi_config *efi_early = __efi_early();
+	efi_rng_protocol_64 *rng = NULL;
+	efi_guid_t rng_proto = EFI_RNG_PROTOCOL_GUID;
+	u64 *handles = (u64 *)(unsigned long)rng_handle;
+	unsigned long size = 0;
+	void **algorithmlist = NULL;
+	efi_status_t status;
+
+	status = efi_call_early(handle_protocol, handles[0],
+				&rng_proto, (void **)&rng);
+	if (status != EFI_SUCCESS)
+		efi_printk(sys_table, "Failed to get EFI_RNG_PROTOCOL handles\n");
+
+	if (status == EFI_SUCCESS && rng) {
+		status = efi_early->call((unsigned long)rng->get_info, rng,
+					&size, algorithmlist);
+		return (status == EFI_BUFFER_TOO_SMALL);
+	}
+
+	return false;
+}
+
+static bool efi_rng_supported(efi_system_table_t *sys_table)
+{
+	const struct efi_config *efi_early = __efi_early();
+	bool supported;
+	efi_status_t status;
+	void **rng_handle = NULL;
+
+	status = efi_locate_rng(sys_table, &rng_handle);
+	if (status != EFI_SUCCESS)
+		return false;
+
+	if (efi_early->is64)
+		supported = efi_rng_supported64(sys_table, rng_handle);
+	else
+		supported = efi_rng_supported32(sys_table, rng_handle);
+
+	efi_call_early(free_pool, rng_handle);
+
+	return supported;
+}
+
+static unsigned long efi_get_rng32(efi_system_table_t *sys_table,
+				   void **rng_handle)
+{
+	const struct efi_config *efi_early = __efi_early();
+	efi_rng_protocol_32 *rng = NULL;
+	efi_guid_t rng_proto = EFI_RNG_PROTOCOL_GUID;
+	u32 *handles = (u32 *)(unsigned long)rng_handle;
+	efi_status_t status;
+	unsigned long rng_number = 0;
+
+	status = efi_call_early(handle_protocol, handles[0],
+				&rng_proto, (void **)&rng);
+	if (status != EFI_SUCCESS)
+		efi_printk(sys_table, "Failed to get EFI_RNG_PROTOCOL handles\n");
+
+	if (status == EFI_SUCCESS && rng) {
+		status = efi_early->call((unsigned long)rng->get_rng, rng, NULL,
+					sizeof(rng_number), &rng_number);
+		if (status != EFI_SUCCESS) {
+			efi_printk(sys_table, "Failed to get RNG value: ");
+			efi_printk(sys_table, efi_status_to_str(status));
+			efi_printk(sys_table, "\n");
+		}
+	}
+
+	return rng_number;
+}
+
+static unsigned long efi_get_rng64(efi_system_table_t *sys_table,
+				   void **rng_handle)
+{
+	const struct efi_config *efi_early = __efi_early();
+	efi_rng_protocol_64 *rng = NULL;
+	efi_guid_t rng_proto = EFI_RNG_PROTOCOL_GUID;
+	u64 *handles = (u64 *)(unsigned long)rng_handle;
+	efi_status_t status;
+	unsigned long rng_number;
+
+	status = efi_call_early(handle_protocol, handles[0],
+				&rng_proto, (void **)&rng);
+	if (status != EFI_SUCCESS)
+		efi_printk(sys_table, "Failed to get EFI_RNG_PROTOCOL handles\n");
+
+	if (status == EFI_SUCCESS && rng) {
+		status = efi_early->call((unsigned long)rng->get_rng, rng, NULL,
+					sizeof(rng_number), &rng_number);
+		if (status != EFI_SUCCESS) {
+			efi_printk(sys_table, "Failed to get RNG value: ");
+			efi_printk(sys_table, efi_status_to_str(status));
+			efi_printk(sys_table, "\n");
+		}
+	}
+
+	return rng_number;
+}
+
+static unsigned long efi_get_rng(efi_system_table_t *sys_table)
+{
+	const struct efi_config *efi_early = __efi_early();
+	unsigned long random = 0;
+	efi_status_t status;
+	void **rng_handle = NULL;
+
+	status = efi_locate_rng(sys_table, &rng_handle);
+	if (status != EFI_SUCCESS)
+		return 0;
+
+	if (efi_early->is64)
+		random = efi_get_rng64(sys_table, rng_handle);
+	else
+		random = efi_get_rng32(sys_table, rng_handle);
+
+	efi_call_early(free_pool, rng_handle);
+
+	return random;
+}
 
 #define EDX_TSC		(1 << 4)
 #define ECX_RDRAND	(1 << 30)
@@ -46,6 +248,13 @@ static unsigned long get_random_long(unsigned long entropy,
 		use_i8254 = false;
 	}
 
+	if (efi_rng_supported(sys_table)) {
+		raw = efi_get_rng(sys_table);
+		if (raw)
+			random ^= raw;
+		use_i8254 = false;
+	}
+
 	if (use_i8254)
 		random ^= read_i8254();
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 85ef051..8914d60 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -427,6 +427,16 @@ typedef struct {
 #define EFI_PCI_IO_ATTRIBUTE_VGA_PALETTE_IO_16 0x20000
 #define EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 0x40000
 
+typedef struct {
+	u32 get_info;
+	u32 get_rng;
+} efi_rng_protocol_32;
+
+typedef struct {
+	u64 get_info;
+	u64 get_rng;
+} efi_rng_protocol_64;
+
 /*
  * Types and defines for EFI ResetSystem
  */
@@ -595,6 +605,9 @@ void efi_native_runtime_setup(void);
 #define DEVICE_TREE_GUID \
     EFI_GUID(  0xb1b621d5, 0xf19c, 0x41a5, 0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0 )
 
+#define EFI_RNG_PROTOCOL_GUID \
+    EFI_GUID(  0x3152bca5, 0xeade, 0x433d, 0x86, 0x2e, 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44 )
+
 typedef struct {
 	efi_guid_t guid;
 	u64 table;
-- 
2.1.4

[PATCH v2 06/16] x86/efi: Generating random HMAC key for siging hibernate image

[Lee, Chun-Yi]

This patch adds codes in EFI stub for generating and storing the
HMAC key in EFI boot service variable for signing hibernate image.

Per rcf2104, the length of HMAC-SHA1 hash result is 20 bytes, and
it recommended the length of key the same with hash rsult, means
also 20 bytes. Using longer key would not significantly increase
the function strength. Due to the nvram space is limited in some
UEFI machines, so using the minimal recommended length 20 bytes
key that will stored in boot service variable.

The HMAC key stored in EFI boot service variable, GUID is
HIBERNATIONKey-fe141863-c070-478e-b8a3-878a5dc9ef21.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 arch/x86/boot/compressed/eboot.c | 60 ++++++++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/suspend.h   |  9 ++++++
 include/linux/suspend.h          |  3 ++
 3 files changed, 72 insertions(+)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 0ffb6db..463aa9b 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -12,6 +12,7 @@
 #include <asm/efi.h>
 #include <asm/setup.h>
 #include <asm/desc.h>
+#include <asm/suspend.h>
 
 #include "../string.h"
 #include "eboot.h"
@@ -1383,6 +1384,63 @@ free_mem_map:
 	return status;
 }
 
+#ifdef CONFIG_HIBERNATE_VERIFICATION
+#define HIBERNATION_KEY \
+	((efi_char16_t [15]) { 'H', 'I', 'B', 'E', 'R', 'N', 'A', 'T', 'I', 'O', 'N', 'K', 'e', 'y', 0 })
+#define HIBERNATION_KEY_ATTRIBUTE	(EFI_VARIABLE_NON_VOLATILE | \
+					EFI_VARIABLE_BOOTSERVICE_ACCESS)
+
+static void setup_hibernation_keys(struct boot_params *params)
+{
+	unsigned long key_size;
+	unsigned long attributes;
+	struct hibernation_keys *keys;
+	efi_status_t status;
+
+	/* Allocate setup_data to carry keys */
+	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+				sizeof(struct hibernation_keys), &keys);
+	if (status != EFI_SUCCESS) {
+		efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n");
+		return;
+	}
+
+	memset(keys, 0, sizeof(struct hibernation_keys));
+
+	status = efi_call_early(get_variable, HIBERNATION_KEY,
+				&EFI_HIBERNATION_GUID, &attributes,
+				&key_size, keys->hibernation_key);
+	if (status == EFI_SUCCESS && attributes != HIBERNATION_KEY_ATTRIBUTE) {
+		efi_printk(sys_table, "A hibernation key is not boot service variable\n");
+		memset(keys->hibernation_key, 0, HIBERNATION_DIGEST_SIZE);
+		status = efi_call_early(set_variable, HIBERNATION_KEY,
+					&EFI_HIBERNATION_GUID, attributes, 0,
+					NULL);
+		if (status == EFI_SUCCESS) {
+			efi_printk(sys_table, "Cleaned existing hibernation key\n");
+			status = EFI_NOT_FOUND;
+		}
+	}
+
+	if (status != EFI_SUCCESS) {
+		efi_printk(sys_table, "Failed to get existing hibernation key\n");
+
+		efi_get_random_key(sys_table, params, keys->hibernation_key,
+				   HIBERNATION_DIGEST_SIZE);
+
+		status = efi_call_early(set_variable, HIBERNATION_KEY,
+					&EFI_HIBERNATION_GUID,
+					HIBERNATION_KEY_ATTRIBUTE,
+					HIBERNATION_DIGEST_SIZE,
+					keys->hibernation_key);
+		if (status != EFI_SUCCESS)
+			efi_printk(sys_table, "Failed to set hibernation key\n");
+	}
+}
+#else
+static void setup_hibernation_keys(struct boot_params *params) {}
+#endif
+
 /*
  * On success we return a pointer to a boot_params structure, and NULL
  * on failure.
@@ -1420,6 +1478,8 @@ struct boot_params *efi_main(struct efi_config *c,
 
 	setup_efi_pci(boot_params);
 
+	setup_hibernation_keys(boot_params);
+
 	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
 				sizeof(*gdt), (void **)&gdt);
 	if (status != EFI_SUCCESS) {
diff --git a/arch/x86/include/asm/suspend.h b/arch/x86/include/asm/suspend.h
index 2fab6c2..ab463c4 100644
--- a/arch/x86/include/asm/suspend.h
+++ b/arch/x86/include/asm/suspend.h
@@ -3,3 +3,12 @@
 #else
 # include <asm/suspend_64.h>
 #endif
+
+#ifdef CONFIG_HIBERNATE_VERIFICATION
+#include <linux/suspend.h>
+
+struct hibernation_keys {
+	unsigned long hkey_status;
+	u8 hibernation_key[HIBERNATION_DIGEST_SIZE];
+};
+#endif
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 56c6de9..aa88b3b 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -328,6 +328,9 @@ struct platform_hibernation_ops {
 
 #ifdef CONFIG_HIBERNATION
 
+#define EFI_HIBERNATION_GUID \
+	EFI_GUID(0xfe141863, 0xc070, 0x478e, 0xb8, 0xa3, 0x87, 0x8a, 0x5d, 0xc9, 0xef, 0x21)
+
 /* HMAC Algorithm of Hibernate Signature */
 #define HIBERNATION_HMAC	"hmac(sha1)"
 #define HIBERNATION_DIGEST_SIZE	20
-- 
2.1.4

[PATCH v2 07/16] efi: Make efi_status_to_err() public

[Lee, Chun-Yi]

Moved the function of transferring EFI status to kernel error for
later used by EFI stub.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/firmware/efi/vars.c | 33 ---------------------------------
 include/linux/efi.h         | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 70a0fb1..f5ede94 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -237,39 +237,6 @@ check_var_size(u32 attributes, unsigned long size)
 	return fops->query_variable_store(attributes, size);
 }
 
-static int efi_status_to_err(efi_status_t status)
-{
-	int err;
-
-	switch (status) {
-	case EFI_SUCCESS:
-		err = 0;
-		break;
-	case EFI_INVALID_PARAMETER:
-		err = -EINVAL;
-		break;
-	case EFI_OUT_OF_RESOURCES:
-		err = -ENOSPC;
-		break;
-	case EFI_DEVICE_ERROR:
-		err = -EIO;
-		break;
-	case EFI_WRITE_PROTECTED:
-		err = -EROFS;
-		break;
-	case EFI_SECURITY_VIOLATION:
-		err = -EACCES;
-		break;
-	case EFI_NOT_FOUND:
-		err = -ENOENT;
-		break;
-	default:
-		err = -EINVAL;
-	}
-
-	return err;
-}
-
 static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor,
 				struct list_head *head)
 {
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8914d60..44a127b 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -874,6 +874,39 @@ efi_guid_to_str(efi_guid_t *guid, char *out)
         return out;
 }
 
+static inline int efi_status_to_err(efi_status_t status)
+{
+	int err;
+
+	switch (status) {
+	case EFI_SUCCESS:
+		err = 0;
+		break;
+	case EFI_INVALID_PARAMETER:
+		err = -EINVAL;
+		break;
+	case EFI_OUT_OF_RESOURCES:
+		err = -ENOSPC;
+		break;
+	case EFI_DEVICE_ERROR:
+		err = -EIO;
+		break;
+	case EFI_WRITE_PROTECTED:
+		err = -EROFS;
+		break;
+	case EFI_SECURITY_VIOLATION:
+		err = -EACCES;
+		break;
+	case EFI_NOT_FOUND:
+		err = -ENOENT;
+		break;
+	default:
+		err = -EINVAL;
+	}
+
+	return err;
+}
+
 extern void efi_init (void);
 extern void *efi_get_pal_addr (void);
 extern void efi_map_pal_code (void);
-- 
2.1.4

[PATCH v2 08/16] x86/efi: Carrying hibernation key by setup data

[Lee, Chun-Yi]

For forwarding hibernation key from EFI stub to boot kernel, this patch
allocates setup data for carrying hibernation key, size and the status
of efi operating.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 arch/x86/boot/compressed/eboot.c      | 26 +++++++++++++++++++++++---
 arch/x86/include/uapi/asm/bootparam.h |  1 +
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 463aa9b..c838d09 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1394,18 +1394,22 @@ static void setup_hibernation_keys(struct boot_params *params)
 {
 	unsigned long key_size;
 	unsigned long attributes;
+	struct setup_data *setup_data, *hibernation_setup_data;
 	struct hibernation_keys *keys;
+	unsigned long size = 0;
 	efi_status_t status;
 
 	/* Allocate setup_data to carry keys */
+	size = sizeof(struct setup_data) + sizeof(struct hibernation_keys);
 	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
-				sizeof(struct hibernation_keys), &keys);
+				size, &hibernation_setup_data);
 	if (status != EFI_SUCCESS) {
 		efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n");
 		return;
 	}
 
-	memset(keys, 0, sizeof(struct hibernation_keys));
+	memset(hibernation_setup_data, 0, size);
+	keys = (struct hibernation_keys *) hibernation_setup_data->data;
 
 	status = efi_call_early(get_variable, HIBERNATION_KEY,
 				&EFI_HIBERNATION_GUID, &attributes,
@@ -1419,7 +1423,8 @@ static void setup_hibernation_keys(struct boot_params *params)
 		if (status == EFI_SUCCESS) {
 			efi_printk(sys_table, "Cleaned existing hibernation key\n");
 			status = EFI_NOT_FOUND;
-		}
+		} else
+			goto clean_fail;
 	}
 
 	if (status != EFI_SUCCESS) {
@@ -1436,6 +1441,21 @@ static void setup_hibernation_keys(struct boot_params *params)
 		if (status != EFI_SUCCESS)
 			efi_printk(sys_table, "Failed to set hibernation key\n");
 	}
+
+clean_fail:
+	hibernation_setup_data->type = SETUP_HIBERNATION_KEYS;
+	hibernation_setup_data->len = sizeof(struct hibernation_keys);
+	hibernation_setup_data->next = 0;
+	keys->hkey_status = efi_status_to_err(status);
+
+	setup_data = (struct setup_data *)params->hdr.setup_data;
+	while (setup_data && setup_data->next)
+		setup_data = (struct setup_data *)setup_data->next;
+
+	if (setup_data)
+		setup_data->next = (unsigned long)hibernation_setup_data;
+	else
+		params->hdr.setup_data = (unsigned long)hibernation_setup_data;
 }
 #else
 static void setup_hibernation_keys(struct boot_params *params) {}
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index ab456dc..8c88bc2 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -7,6 +7,7 @@
 #define SETUP_DTB			2
 #define SETUP_PCI			3
 #define SETUP_EFI			4
+#define SETUP_HIBERNATION_KEYS		5
 
 /* ram_size flags */
 #define RAMDISK_IMAGE_START_MASK	0x07FF
-- 
2.1.4

[PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints

[Lee, Chun-Yi]

Add handler to parse the setup data that carrying hibernation key, it
reserves hibernation key by memblock then copies key to a allocated page
in later initcall stage.

And for erasing footprints, the codes in this patch remove setup
data that carried hibernation key, and clean the memory space that
reserved by memblock.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 arch/x86/include/asm/suspend.h  |  4 +++
 arch/x86/kernel/setup.c         | 21 ++++++++++-
 arch/x86/power/Makefile         |  1 +
 arch/x86/power/hibernate_keys.c | 78 +++++++++++++++++++++++++++++++++++++++++
 kernel/power/power.h            |  5 +++
 5 files changed, 108 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/power/hibernate_keys.c

diff --git a/arch/x86/include/asm/suspend.h b/arch/x86/include/asm/suspend.h
index ab463c4..bb3652a 100644
--- a/arch/x86/include/asm/suspend.h
+++ b/arch/x86/include/asm/suspend.h
@@ -7,8 +7,12 @@
 #ifdef CONFIG_HIBERNATE_VERIFICATION
 #include <linux/suspend.h>
 
+extern void parse_hibernation_keys(u64 phys_addr, u32 data_len);
+
 struct hibernation_keys {
 	unsigned long hkey_status;
 	u8 hibernation_key[HIBERNATION_DIGEST_SIZE];
 };
+#else
+static inline void parse_hibernation_keys(u64 phys_addr, u32 data_len) {}
 #endif
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 80f874b..b345359 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -112,6 +112,8 @@
 #include <asm/alternative.h>
 #include <asm/prom.h>
 
+#include <asm/suspend.h>
+
 /*
  * max_low_pfn_mapped: highest direct mapped pfn under 4GB
  * max_pfn_mapped:     highest direct mapped pfn over 4GB
@@ -425,10 +427,22 @@ static void __init reserve_initrd(void)
 }
 #endif /* CONFIG_BLK_DEV_INITRD */
 
+static void __init remove_setup_data(u64 pa_prev, u64 pa_next)
+{
+	struct setup_data *data;
+
+	if (pa_prev) {
+		data = early_memremap(pa_prev, sizeof(*data));
+		data->next = pa_next;
+		early_iounmap(data, sizeof(*data));
+	} else
+		boot_params.hdr.setup_data = pa_next;
+}
+
 static void __init parse_setup_data(void)
 {
 	struct setup_data *data;
-	u64 pa_data, pa_next;
+	u64 pa_data, pa_next, pa_prev = 0;
 
 	pa_data = boot_params.hdr.setup_data;
 	while (pa_data) {
@@ -450,9 +464,14 @@ static void __init parse_setup_data(void)
 		case SETUP_EFI:
 			parse_efi_setup(pa_data, data_len);
 			break;
+		case SETUP_HIBERNATION_KEYS:
+			parse_hibernation_keys(pa_data, data_len);
+			remove_setup_data(pa_prev, pa_next);
+			break;
 		default:
 			break;
 		}
+		pa_prev = pa_data;
 		pa_data = pa_next;
 	}
 }
diff --git a/arch/x86/power/Makefile b/arch/x86/power/Makefile
index a6a198c..ef8d550 100644
--- a/arch/x86/power/Makefile
+++ b/arch/x86/power/Makefile
@@ -5,3 +5,4 @@ CFLAGS_cpu.o	:= $(nostackp)
 
 obj-$(CONFIG_PM_SLEEP)		+= cpu.o
 obj-$(CONFIG_HIBERNATION)	+= hibernate_$(BITS).o hibernate_asm_$(BITS).o
+obj-$(CONFIG_HIBERNATE_VERIFICATION)	+= hibernate_keys.o
diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c
new file mode 100644
index 0000000..357dc0e
--- /dev/null
+++ b/arch/x86/power/hibernate_keys.c
@@ -0,0 +1,78 @@
+/* Hibernation keys handler
+ *
+ * Copyright (C) 2015 Lee, Chun-Yi <jlee@suse.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#include <linux/bootmem.h>
+#include <linux/memblock.h>
+#include <linux/suspend.h>
+#include <asm/suspend.h>
+
+/* physical address of hibernation keys from boot params */
+static u64 keys_phys_addr;
+
+/* A page used to keep hibernation keys */
+static struct hibernation_keys *hibernation_keys;
+
+void __init parse_hibernation_keys(u64 phys_addr, u32 data_len)
+{
+	struct setup_data *hibernation_setup_data;
+
+	/* Reserve keys memory, will copy and erase in init_hibernation_keys() */
+	keys_phys_addr = phys_addr + sizeof(struct setup_data);
+	memblock_reserve(keys_phys_addr, sizeof(struct hibernation_keys));
+
+	/* clear hibernation_data */
+	hibernation_setup_data = early_memremap(phys_addr, data_len);
+	if (!hibernation_setup_data)
+		return;
+
+	memset(hibernation_setup_data, 0, sizeof(struct setup_data));
+	early_memunmap(hibernation_setup_data, data_len);
+}
+
+int get_hibernation_key(u8 **hkey)
+{
+	if (!hibernation_keys)
+		return -ENODEV;
+
+	if (!hibernation_keys->hkey_status)
+		*hkey = hibernation_keys->hibernation_key;
+
+	return hibernation_keys->hkey_status;
+}
+
+static int __init init_hibernation_keys(void)
+{
+	struct hibernation_keys *keys;
+	int ret = 0;
+
+	if (!keys_phys_addr)
+		return -ENODEV;
+
+	keys = early_memremap(keys_phys_addr, sizeof(struct hibernation_keys));
+
+	/* Copy hibernation keys to a allocated page */
+	hibernation_keys = (struct hibernation_keys *)get_zeroed_page(GFP_KERNEL);
+	if (hibernation_keys) {
+		*hibernation_keys = *keys;
+	} else {
+		pr_err("PM: Allocate hibernation keys page failed\n");
+		ret = -ENOMEM;
+	}
+
+	/* Erase keys data no matter copy success or failed */
+	memset(keys, 0, sizeof(struct hibernation_keys));
+	early_memunmap(keys, sizeof(struct hibernation_keys));
+	memblock_free(keys_phys_addr, sizeof(struct hibernation_keys));
+	keys_phys_addr = 0;
+
+	return ret;
+}
+
+late_initcall(init_hibernation_keys);
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 6ea5c78..7d8f310 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -16,6 +16,11 @@ struct swsusp_info {
 	u8                      signature[HIBERNATION_DIGEST_SIZE];
 } __aligned(PAGE_SIZE);
 
+#ifdef CONFIG_HIBERNATE_VERIFICATION
+/* arch/x86/power/hibernate_keys.c */
+extern int get_hibernation_key(u8 **hkey);
+#endif
+
 /* kernel/power/snapshot.c */
 extern void __init hibernate_reserved_size_init(void);
 extern void __init hibernate_image_size_init(void);
-- 
2.1.4

[PATCH v2 10/16] PM / hibernate: Generate and verify signature of hibernate snapshot

[Lee, Chun-Yi]

This is the heart of generating and verifying the signature of
snapshot image of hibernate.

When creating hibernation image, HMAC-SHA1 calculates hash result
of all data pages that are copied to image. The signature is stored
in the header of snapshot, and verified by resuming code when it's
writing snapshot image to memory space.

When the signature verification failed, the hibernate code will stop
to recover system to image kernel and system will boots as normal.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 kernel/power/power.h    |   5 +
 kernel/power/snapshot.c | 254 +++++++++++++++++++++++++++++++++++++++++++++---
 kernel/power/swap.c     |   4 +
 kernel/power/user.c     |   4 +
 4 files changed, 252 insertions(+), 15 deletions(-)

diff --git a/kernel/power/power.h b/kernel/power/power.h
index 7d8f310..ccc1e72 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -143,6 +143,11 @@ extern int snapshot_read_next(struct snapshot_handle *handle);
 extern int snapshot_write_next(struct snapshot_handle *handle);
 extern void snapshot_write_finalize(struct snapshot_handle *handle);
 extern int snapshot_image_loaded(struct snapshot_handle *handle);
+#ifdef CONFIG_HIBERNATE_VERIFICATION
+extern int snapshot_image_verify(void);
+#else
+static inline int snapshot_image_verify(void) { return 0; }
+#endif
 
 /* If unset, the snapshot device cannot be open. */
 extern atomic_t snapshot_device_available;
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 5235dd4..b8c7e2e 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -36,6 +36,8 @@
 #include <asm/tlbflush.h>
 #include <asm/io.h>
 
+#include <crypto/hash.h>
+
 #include "power.h"
 
 static int swsusp_page_is_free(struct page *);
@@ -1265,7 +1267,214 @@ static inline void copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
 }
 #endif /* CONFIG_HIGHMEM */
 
-static void
+/* Total number of image pages */
+static unsigned int nr_copy_pages;
+
+/*
+ * Signature of snapshot
+ */
+static u8 signature[HIBERNATION_DIGEST_SIZE];
+
+/* Buffer point array for collecting address of page buffers */
+void **h_buf;
+
+#ifdef CONFIG_HIBERNATE_VERIFICATION
+static int
+__copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
+{
+	unsigned long pfn, dst_pfn;
+	struct page *d_page;
+	void *hash_buffer = NULL;
+	struct crypto_shash *tfm = NULL;
+	struct shash_desc *desc = NULL;
+	u8 *key = NULL, *digest = NULL;
+	size_t digest_size, desc_size;
+	int key_err = 0, ret = 0;
+
+	key_err = get_hibernation_key(&key);
+	if (key_err)
+		goto copy_pages;
+
+	tfm = crypto_alloc_shash(HIBERNATION_HMAC, 0, 0);
+	if (IS_ERR(tfm)) {
+		pr_err("PM: Allocate HMAC failed: %ld\n", PTR_ERR(tfm));
+		return PTR_ERR(tfm);
+	}
+
+	ret = crypto_shash_setkey(tfm, key, HIBERNATION_DIGEST_SIZE);
+	if (ret) {
+		pr_err("PM: Set HMAC key failed\n");
+		goto error_setkey;
+	}
+
+	desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
+	digest_size = crypto_shash_digestsize(tfm);
+	digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
+	if (!digest) {
+		pr_err("PM: Allocate digest failed\n");
+		ret = -ENOMEM;
+		goto error_digest;
+	}
+
+	desc = (void *) digest + digest_size;
+	desc->tfm = tfm;
+	desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+	ret = crypto_shash_init(desc);
+	if (ret < 0)
+		goto error_shash;
+
+copy_pages:
+	memory_bm_position_reset(orig_bm);
+	memory_bm_position_reset(copy_bm);
+	for (;;) {
+		pfn = memory_bm_next_pfn(orig_bm);
+		if (unlikely(pfn == BM_END_OF_MAP))
+			break;
+		dst_pfn = memory_bm_next_pfn(copy_bm);
+		copy_data_page(dst_pfn, pfn);
+
+		/* Generate digest */
+		d_page = pfn_to_page(dst_pfn);
+		if (PageHighMem(d_page)) {
+			void *kaddr = kmap_atomic(d_page);
+
+			copy_page(buffer, kaddr);
+			kunmap_atomic(kaddr);
+			hash_buffer = buffer;
+		} else {
+			hash_buffer = page_address(d_page);
+		}
+
+		if (key_err)
+			continue;
+
+		ret = crypto_shash_update(desc, hash_buffer, PAGE_SIZE);
+		if (ret)
+			goto error_shash;
+	}
+
+	if (key_err)
+		goto error_key;
+
+	ret = crypto_shash_final(desc, digest);
+	if (ret)
+		goto error_shash;
+
+	memset(signature, 0, HIBERNATION_DIGEST_SIZE);
+	memcpy(signature, digest, HIBERNATION_DIGEST_SIZE);
+
+	kfree(digest);
+	crypto_free_shash(tfm);
+
+	return 0;
+
+error_shash:
+	kfree(digest);
+error_setkey:
+error_digest:
+	crypto_free_shash(tfm);
+error_key:
+	return ret;
+}
+
+int snapshot_image_verify(void)
+{
+	struct crypto_shash *tfm;
+	struct shash_desc *desc;
+	u8 *key, *digest;
+	size_t digest_size, desc_size;
+	int ret, i;
+
+	if (!h_buf)
+		return 0;
+
+	ret = get_hibernation_key(&key);
+	if (ret)
+		goto forward_ret;
+
+	tfm = crypto_alloc_shash(HIBERNATION_HMAC, 0, 0);
+	if (IS_ERR(tfm)) {
+		pr_err("PM: Allocate HMAC failed: %ld\n", PTR_ERR(tfm));
+		return PTR_ERR(tfm);
+	}
+
+	ret = crypto_shash_setkey(tfm, key, HIBERNATION_DIGEST_SIZE);
+	if (ret) {
+		pr_err("PM: Set HMAC key failed\n");
+		goto error_setkey;
+	}
+
+	desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
+	digest_size = crypto_shash_digestsize(tfm);
+	digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
+	if (!digest) {
+		pr_err("PM: Allocate digest failed\n");
+		ret = -ENOMEM;
+		goto error_digest;
+	}
+	desc = (void *) digest + digest_size;
+	desc->tfm = tfm;
+	desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+
+	ret = crypto_shash_init(desc);
+	if (ret < 0)
+		goto error_shash;
+
+	for (i = 0; i < nr_copy_pages; i++) {
+		ret = crypto_shash_update(desc, *(h_buf + i), PAGE_SIZE);
+		if (ret)
+			goto error_shash;
+	}
+
+	ret = crypto_shash_final(desc, digest);
+	if (ret)
+		goto error_shash;
+
+	pr_debug("PM: Signature %*phN\n", HIBERNATION_DIGEST_SIZE, signature);
+	pr_debug("PM: Digest %*phN\n", (int) digest_size, digest);
+	if (memcmp(signature, digest, HIBERNATION_DIGEST_SIZE))
+		ret = -EKEYREJECTED;
+
+error_shash:
+	kfree(h_buf);
+	kfree(digest);
+error_setkey:
+error_digest:
+	crypto_free_shash(tfm);
+forward_ret:
+	if (ret)
+		pr_warn("PM: Signature verifying failed: %d\n", ret);
+	return ret;
+}
+
+static void alloc_h_buf(void)
+{
+	h_buf = kmalloc(sizeof(void *) * nr_copy_pages, GFP_KERNEL);
+	if (!h_buf)
+		pr_err("PM: Allocate buffer point array failed\n");
+}
+#else
+static int
+__copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
+{
+	unsigned long pfn;
+
+	memory_bm_position_reset(orig_bm);
+	memory_bm_position_reset(copy_bm);
+	for (;;) {
+		pfn = memory_bm_next_pfn(orig_bm);
+		if (unlikely(pfn == BM_END_OF_MAP))
+			break;
+		copy_data_page(memory_bm_next_pfn(copy_bm), pfn);
+	}
+
+	return 0;
+}
+
+static inline void alloc_h_buf(void) {}
+#endif /* CONFIG_HIBERNATE_VERIFICATION */
+
+static int
 copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
 {
 	struct zone *zone;
@@ -1280,18 +1489,10 @@ copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
 			if (page_is_saveable(zone, pfn))
 				memory_bm_set_bit(orig_bm, pfn);
 	}
-	memory_bm_position_reset(orig_bm);
-	memory_bm_position_reset(copy_bm);
-	for(;;) {
-		pfn = memory_bm_next_pfn(orig_bm);
-		if (unlikely(pfn == BM_END_OF_MAP))
-			break;
-		copy_data_page(memory_bm_next_pfn(copy_bm), pfn);
-	}
+
+	return __copy_data_pages(copy_bm, orig_bm);
 }
 
-/* Total number of image pages */
-static unsigned int nr_copy_pages;
 /* Number of pages needed for saving the original pfns of the image pages */
 static unsigned int nr_meta_pages;
 /*
@@ -1837,6 +2038,7 @@ swsusp_alloc(struct memory_bitmap *orig_bm, struct memory_bitmap *copy_bm,
 asmlinkage __visible int swsusp_save(void)
 {
 	unsigned int nr_pages, nr_highmem;
+	int ret;
 
 	printk(KERN_INFO "PM: Creating hibernation image:\n");
 
@@ -1859,7 +2061,11 @@ asmlinkage __visible int swsusp_save(void)
 	 * Kill them.
 	 */
 	drain_local_pages(NULL);
-	copy_data_pages(&copy_bm, &orig_bm);
+	ret = copy_data_pages(&copy_bm, &orig_bm);
+	if (ret) {
+		pr_err("PM: Copy data pages failed\n");
+		return ret;
+	}
 
 	/*
 	 * End of critical section. From now on, we can write to memory,
@@ -1914,6 +2120,7 @@ static int init_header(struct swsusp_info *info)
 	info->pages = snapshot_get_image_size();
 	info->size = info->pages;
 	info->size <<= PAGE_SHIFT;
+	memcpy(info->signature, signature, HIBERNATION_DIGEST_SIZE);
 	return init_header_complete(info);
 }
 
@@ -2076,6 +2283,8 @@ load_header(struct swsusp_info *info)
 	if (!error) {
 		nr_copy_pages = info->image_pages;
 		nr_meta_pages = info->pages - info->image_pages - 1;
+		memset(signature, 0, HIBERNATION_DIGEST_SIZE);
+		memcpy(signature, info->signature, HIBERNATION_DIGEST_SIZE);
 	}
 	return error;
 }
@@ -2414,7 +2623,8 @@ prepare_image(struct memory_bitmap *new_bm, struct memory_bitmap *bm)
  *	set for its caller to write to.
  */
 
-static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
+static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca,
+		unsigned long *_pfn)
 {
 	struct pbe *pbe;
 	struct page *page;
@@ -2423,6 +2633,9 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
 	if (pfn == BM_END_OF_MAP)
 		return ERR_PTR(-EFAULT);
 
+	if (_pfn)
+		*_pfn = pfn;
+
 	page = pfn_to_page(pfn);
 	if (PageHighMem(page))
 		return get_highmem_page_buffer(page, ca);
@@ -2469,6 +2682,7 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
 int snapshot_write_next(struct snapshot_handle *handle)
 {
 	static struct chain_allocator ca;
+	unsigned long pfn;
 	int error = 0;
 
 	/* Check if we have already loaded the entire image */
@@ -2491,6 +2705,12 @@ int snapshot_write_next(struct snapshot_handle *handle)
 		if (error)
 			return error;
 
+		/* Allocate buffer point array for generating
+		 * digest to compare with signature.
+		 * h_buf will freed in snapshot_image_verify().
+		 */
+		alloc_h_buf();
+
 		error = memory_bm_create(&copy_bm, GFP_ATOMIC, PG_ANY);
 		if (error)
 			return error;
@@ -2513,20 +2733,24 @@ int snapshot_write_next(struct snapshot_handle *handle)
 			chain_init(&ca, GFP_ATOMIC, PG_SAFE);
 			memory_bm_position_reset(&orig_bm);
 			restore_pblist = NULL;
-			handle->buffer = get_buffer(&orig_bm, &ca);
+			handle->buffer = get_buffer(&orig_bm, &ca, &pfn);
 			handle->sync_read = 0;
 			if (IS_ERR(handle->buffer))
 				return PTR_ERR(handle->buffer);
+			if (h_buf)
+				*h_buf = handle->buffer;
 		}
 	} else {
 		copy_last_highmem_page();
 		/* Restore page key for data page (s390 only). */
 		page_key_write(handle->buffer);
-		handle->buffer = get_buffer(&orig_bm, &ca);
+		handle->buffer = get_buffer(&orig_bm, &ca, &pfn);
 		if (IS_ERR(handle->buffer))
 			return PTR_ERR(handle->buffer);
 		if (handle->buffer != buffer)
 			handle->sync_read = 0;
+		if (h_buf)
+			*(h_buf + (handle->cur - nr_meta_pages - 1)) = handle->buffer;
 	}
 	handle->cur++;
 	return PAGE_SIZE;
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 2f30ca9..ff2b36f 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -1085,6 +1085,8 @@ static int load_image(struct swap_map_handle *handle,
 		snapshot_write_finalize(snapshot);
 		if (!snapshot_image_loaded(snapshot))
 			ret = -ENODATA;
+		if (!ret)
+			ret = snapshot_image_verify();
 	}
 	swsusp_show_speed(start, stop, nr_to_read, "Read");
 	return ret;
@@ -1440,6 +1442,8 @@ out_finish:
 				}
 			}
 		}
+		if (!ret)
+			ret = snapshot_image_verify();
 	}
 	swsusp_show_speed(start, stop, nr_to_read, "Read");
 out_clean:
diff --git a/kernel/power/user.c b/kernel/power/user.c
index 526e891..9b891d5 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -268,6 +268,10 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 			error = -EPERM;
 			break;
 		}
+		if (snapshot_image_verify()) {
+			error = -EPERM;
+			break;
+		}
 		error = hibernation_restore(data->platform_support);
 		break;
 
-- 
2.1.4

[PATCH v2 11/16] PM / hibernate: Avoid including hibernation key to hibernate image

[Lee, Chun-Yi]

The HMAC key should only resides in kernel memory space but not leak
to outside. To avoid including hibernation key in hibernate snapshot
image, this patch adds the checking block in the code for asking saveable
pages to make sure the key page should not marked as saveable.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 arch/x86/power/hibernate_keys.c | 15 +++++++++++++++
 kernel/power/power.h            |  3 +++
 kernel/power/snapshot.c         |  6 ++++++
 3 files changed, 24 insertions(+)

diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c
index 357dc0e..f44823e 100644
--- a/arch/x86/power/hibernate_keys.c
+++ b/arch/x86/power/hibernate_keys.c
@@ -47,6 +47,21 @@ int get_hibernation_key(u8 **hkey)
 	return hibernation_keys->hkey_status;
 }
 
+
+bool swsusp_page_is_keys(struct page *page)
+{
+	bool ret = false;
+
+	if (!hibernation_keys || hibernation_keys->hkey_status)
+		return ret;
+
+	ret = (page_to_pfn(page) == page_to_pfn(virt_to_page(hibernation_keys)));
+	if (ret)
+		pr_info("PM: Avoid snapshot the page of hibernation key.\n");
+
+	return ret;
+}
+
 static int __init init_hibernation_keys(void)
 {
 	struct hibernation_keys *keys;
diff --git a/kernel/power/power.h b/kernel/power/power.h
index ccc1e72..6d1d406 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -19,6 +19,9 @@ struct swsusp_info {
 #ifdef CONFIG_HIBERNATE_VERIFICATION
 /* arch/x86/power/hibernate_keys.c */
 extern int get_hibernation_key(u8 **hkey);
+extern bool swsusp_page_is_keys(struct page *page);
+#else
+static inline bool swsusp_page_is_keys(struct page *page) { return false; }
 #endif
 
 /* kernel/power/snapshot.c */
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index b8c7e2e..5522028 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1093,6 +1093,9 @@ static struct page *saveable_highmem_page(struct zone *zone, unsigned long pfn)
 
 	BUG_ON(!PageHighMem(page));
 
+	if (swsusp_page_is_keys(page))
+		return NULL;
+
 	if (swsusp_page_is_forbidden(page) ||  swsusp_page_is_free(page) ||
 	    PageReserved(page))
 		return NULL;
@@ -1155,6 +1158,9 @@ static struct page *saveable_page(struct zone *zone, unsigned long pfn)
 
 	BUG_ON(PageHighMem(page));
 
+	if (swsusp_page_is_keys(page))
+		return NULL;
+
 	if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
 		return NULL;
 
-- 
2.1.4

[PATCH v2 12/16] PM / hibernate: Forward signature verifying result and key to image kernel

[Lee, Chun-Yi]

Due to the memory space of boot kernel will be overwritten after restoring
snapshot image and switching to image kernel. There have some informations
should be forwarded from boot kernel to image kernel: the result of
signature verifying, flag of enforce verifying signature and hibernation key.
That because those informations did not include in hibernate image or
produced when restoring.

The significant reason is image kernel needs hibernation key to sign image for
next hibernate cycle, otherwise the signature generating will be failed in
next cycle. In additional, it's also useful to expose the verification
result in dmesg after recovery.

The codes in hibernate key handler allocates an empty page as the buffer
of forward information that will be included in snapshot iamge, then keeps
page frame number in image header. When restoring snapshot image to memory
space of boot kernel, snapshot codes will asking key handler to fill forward
informations to buffer page. Then restoring hibernation key data to key page,
and cleaning this page buffer for next cycle.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 arch/x86/power/hibernate_keys.c | 63 +++++++++++++++++++++++++++++++++++++++++
 kernel/power/hibernate.c        |  1 +
 kernel/power/power.h            |  6 ++++
 kernel/power/snapshot.c         | 27 +++++++++++++++++-
 kernel/power/user.c             |  1 +
 5 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c
index f44823e..5e92101 100644
--- a/arch/x86/power/hibernate_keys.c
+++ b/arch/x86/power/hibernate_keys.c
@@ -19,6 +19,15 @@ static u64 keys_phys_addr;
 /* A page used to keep hibernation keys */
 static struct hibernation_keys *hibernation_keys;
 
+/* Forward information and keys from boot kernel to image kernel */
+struct forward_info {
+	bool            sig_enforce;
+	int             sig_verify_ret;
+	struct hibernation_keys hibernation_keys;
+};
+
+static struct forward_info *forward_buff;
+
 void __init parse_hibernation_keys(u64 phys_addr, u32 data_len)
 {
 	struct setup_data *hibernation_setup_data;
@@ -62,6 +71,55 @@ bool swsusp_page_is_keys(struct page *page)
 	return ret;
 }
 
+unsigned long get_forward_buff_pfn(void)
+{
+	if (!forward_buff)
+		return 0;
+
+	return page_to_pfn(virt_to_page(forward_buff));
+}
+
+void fill_forward_info(void *forward_buff_page, int verify_ret)
+{
+	struct forward_info *info;
+
+	if (!forward_buff_page)
+		return;
+
+	memset(forward_buff_page, 0, PAGE_SIZE);
+	info = (struct forward_info *)forward_buff_page;
+	info->sig_verify_ret = verify_ret;
+
+	if (hibernation_keys && !hibernation_keys->hkey_status) {
+		info->hibernation_keys = *hibernation_keys;
+		memset(hibernation_keys, 0, PAGE_SIZE);
+	} else
+		pr_info("PM: Fill hibernation keys failed\n");
+
+	pr_info("PM: Filled sign information to forward buffer\n");
+}
+
+void restore_sig_forward_info(void)
+{
+	if (!forward_buff) {
+		pr_warn("PM: Did not allocate forward buffer\n");
+		return;
+	}
+
+	if (forward_buff->sig_verify_ret)
+		pr_warn("PM: Signature verifying failed: %d\n",
+			forward_buff->sig_verify_ret);
+
+	if (hibernation_keys) {
+		memset(hibernation_keys, 0, PAGE_SIZE);
+		*hibernation_keys = forward_buff->hibernation_keys;
+		pr_info("PM: Restored hibernation keys\n");
+	}
+
+	/* clean forward information buffer for next round */
+	memset(forward_buff, 0, PAGE_SIZE);
+}
+
 static int __init init_hibernation_keys(void)
 {
 	struct hibernation_keys *keys;
@@ -76,6 +134,11 @@ static int __init init_hibernation_keys(void)
 	hibernation_keys = (struct hibernation_keys *)get_zeroed_page(GFP_KERNEL);
 	if (hibernation_keys) {
 		*hibernation_keys = *keys;
+		forward_buff = (struct forward_info *)get_zeroed_page(GFP_KERNEL);
+		if (!forward_buff) {
+			pr_err("PM: Allocate forward buffer failed\n");
+			ret = -ENOMEM;
+		}
 	} else {
 		pr_err("PM: Allocate hibernation keys page failed\n");
 		ret = -ENOMEM;
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 690f78f..640ca8a 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -702,6 +702,7 @@ int hibernate(void)
 		pm_restore_gfp_mask();
 	} else {
 		pr_debug("PM: Image restored successfully.\n");
+		restore_sig_forward_info();
 	}
 
  Free_bitmaps:
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 6d1d406..1ea3513 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -13,6 +13,7 @@ struct swsusp_info {
 	unsigned long		image_pages;
 	unsigned long		pages;
 	unsigned long		size;
+	unsigned long		forward_buff_pfn;
 	u8                      signature[HIBERNATION_DIGEST_SIZE];
 } __aligned(PAGE_SIZE);
 
@@ -20,8 +21,13 @@ struct swsusp_info {
 /* arch/x86/power/hibernate_keys.c */
 extern int get_hibernation_key(u8 **hkey);
 extern bool swsusp_page_is_keys(struct page *page);
+extern unsigned long get_forward_buff_pfn(void);
+extern void fill_forward_info(void *forward_buff_page, int verify_ret);
+extern void restore_sig_forward_info(void);
 #else
 static inline bool swsusp_page_is_keys(struct page *page) { return false; }
+static inline unsigned long get_forward_buff_pfn(void) { return 0; }
+static inline void restore_sig_forward_info(void) {}
 #endif
 
 /* kernel/power/snapshot.c */
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 5522028..3629249 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1281,6 +1281,14 @@ static unsigned int nr_copy_pages;
  */
 static u8 signature[HIBERNATION_DIGEST_SIZE];
 
+/*
+ * Keep the pfn of forwarding information buffer from resume target.
+ * Writing hibernation keys to this buffer in snapshot image before restoring.
+ */
+unsigned long forward_buff_pfn;
+
+void *forward_buff;
+
 /* Buffer point array for collecting address of page buffers */
 void **h_buf;
 
@@ -1383,13 +1391,24 @@ error_key:
 	return ret;
 }
 
+static void snapshot_fill_sig_forward_info(int verify_ret)
+{
+	if (!forward_buff_pfn || !forward_buff) {
+		pr_err("PM: Did not find forward buffer\n");
+		return;
+	}
+
+	/* Fill hibernation keys to snapshot in memory for next round */
+	fill_forward_info(forward_buff, verify_ret);
+}
+
 int snapshot_image_verify(void)
 {
 	struct crypto_shash *tfm;
 	struct shash_desc *desc;
 	u8 *key, *digest;
 	size_t digest_size, desc_size;
-	int ret, i;
+	int i, ret = 0;
 
 	if (!h_buf)
 		return 0;
@@ -1450,6 +1469,7 @@ error_digest:
 forward_ret:
 	if (ret)
 		pr_warn("PM: Signature verifying failed: %d\n", ret);
+	snapshot_fill_sig_forward_info(ret);
 	return ret;
 }
 
@@ -2126,6 +2146,7 @@ static int init_header(struct swsusp_info *info)
 	info->pages = snapshot_get_image_size();
 	info->size = info->pages;
 	info->size <<= PAGE_SHIFT;
+	info->forward_buff_pfn = get_forward_buff_pfn();
 	memcpy(info->signature, signature, HIBERNATION_DIGEST_SIZE);
 	return init_header_complete(info);
 }
@@ -2289,6 +2310,7 @@ load_header(struct swsusp_info *info)
 	if (!error) {
 		nr_copy_pages = info->image_pages;
 		nr_meta_pages = info->pages - info->image_pages - 1;
+		forward_buff_pfn = info->forward_buff_pfn;
 		memset(signature, 0, HIBERNATION_DIGEST_SIZE);
 		memcpy(signature, info->signature, HIBERNATION_DIGEST_SIZE);
 	}
@@ -2757,6 +2779,9 @@ int snapshot_write_next(struct snapshot_handle *handle)
 			handle->sync_read = 0;
 		if (h_buf)
 			*(h_buf + (handle->cur - nr_meta_pages - 1)) = handle->buffer;
+		/* Keep the buffer of hibernation keys in snapshot */
+		if (forward_buff_pfn && pfn == forward_buff_pfn)
+			forward_buff = handle->buffer;
 	}
 	handle->cur++;
 	return PAGE_SIZE;
diff --git a/kernel/power/user.c b/kernel/power/user.c
index 9b891d5..ffd327a 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -241,6 +241,7 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 		if (!data->frozen || data->ready)
 			break;
 		pm_restore_gfp_mask();
+		restore_sig_forward_info();
 		free_basic_memory_bitmaps();
 		data->free_bitmaps = false;
 		thaw_processes();
-- 
2.1.4

[PATCH v2 13/16] PM / hibernate: Add configuration to enforce signature verification

[Lee, Chun-Yi]

Like kernel module signature checking, there's both a config option and
a boot parameter which control whether we accept or fail with unsigned
hibernate image and image that are signed with an unknown key.

If hibernate signing is enabled, the kernel will be tainted if a snapshot
image is restored that is unsigned or has a signature for which we don't
have the key. When the enforce flag is enabled, then the hibernate
restoring process will be failed and boot as normal.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 Documentation/kernel-parameters.txt |  5 +++++
 arch/x86/power/hibernate_keys.c     | 19 +++++++++++++++++--
 include/linux/kernel.h              |  1 +
 include/linux/suspend.h             |  3 +++
 kernel/panic.c                      |  2 ++
 kernel/power/Kconfig                |  8 ++++++++
 kernel/power/hibernate.c            |  7 +++++++
 kernel/power/snapshot.c             |  6 +++++-
 8 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 1d6f045..86a6916 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3318,6 +3318,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		noresume	Don't check if there's a hibernation image
 				present during boot.
 		nocompress	Don't compress/decompress hibernation images.
+		sigenforce	When CONFIG_HIBERNATE_VERIFICATION is set, this
+				menas that snapshot image without (valid)
+				signature will fail to restore. Note that if
+				HIBERNATE_VERIFICATION_FORCE is set, that is
+				always true, so this option does nothing.
 		no		Disable hibernation and resume.
 
 	retain_initrd	[RAM] Keep initrd memory after extraction
diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c
index 5e92101..51e808a 100644
--- a/arch/x86/power/hibernate_keys.c
+++ b/arch/x86/power/hibernate_keys.c
@@ -89,6 +89,7 @@ void fill_forward_info(void *forward_buff_page, int verify_ret)
 	memset(forward_buff_page, 0, PAGE_SIZE);
 	info = (struct forward_info *)forward_buff_page;
 	info->sig_verify_ret = verify_ret;
+	info->sig_enforce = sigenforce;
 
 	if (hibernation_keys && !hibernation_keys->hkey_status) {
 		info->hibernation_keys = *hibernation_keys;
@@ -106,10 +107,24 @@ void restore_sig_forward_info(void)
 		return;
 	}
 
-	if (forward_buff->sig_verify_ret)
-		pr_warn("PM: Signature verifying failed: %d\n",
+	sigenforce = forward_buff->sig_enforce;
+	if (sigenforce)
+		pr_info("PM: Enforce hibernate signature verifying\n");
+
+	if (forward_buff->sig_verify_ret) {
+		pr_warn("PM: Hibernate signature verifying failed: %d\n",
 			forward_buff->sig_verify_ret);
 
+		/* taint kernel */
+		if (!sigenforce) {
+			pr_warn("PM: System restored from unsafe snapshot - "
+				"tainting kernel\n");
+			add_taint(TAINT_UNSAFE_HIBERNATE, LOCKDEP_STILL_OK);
+			pr_info("%s\n", print_tainted());
+		}
+	} else
+		pr_info("PM: Signature verifying pass\n");
+
 	if (hibernation_keys) {
 		memset(hibernation_keys, 0, PAGE_SIZE);
 		*hibernation_keys = forward_buff->hibernation_keys;
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 5582410..a620786 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -489,6 +489,7 @@ extern enum system_states {
 #define TAINT_UNSIGNED_MODULE		13
 #define TAINT_SOFTLOCKUP		14
 #define TAINT_LIVEPATCH			15
+#define TAINT_UNSAFE_HIBERNATE		16
 
 extern const char hex_asc[];
 #define hex_asc_lo(x)	hex_asc[((x) & 0x0f)]
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index aa88b3b..d318b72 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -335,6 +335,9 @@ struct platform_hibernation_ops {
 #define HIBERNATION_HMAC	"hmac(sha1)"
 #define HIBERNATION_DIGEST_SIZE	20
 
+/* kernel/power/hibernate.c */
+extern int sigenforce;
+
 /* kernel/power/snapshot.c */
 extern void __register_nosave_region(unsigned long b, unsigned long e, int km);
 static inline void __init register_nosave_region(unsigned long b, unsigned long e)
diff --git a/kernel/panic.c b/kernel/panic.c
index 04e91ff..a53da16 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -228,6 +228,7 @@ static const struct tnt tnts[] = {
 	{ TAINT_UNSIGNED_MODULE,	'E', ' ' },
 	{ TAINT_SOFTLOCKUP,		'L', ' ' },
 	{ TAINT_LIVEPATCH,		'K', ' ' },
+	{ TAINT_UNSAFE_HIBERNATE,	'H', ' ' },
 };
 
 /**
@@ -249,6 +250,7 @@ static const struct tnt tnts[] = {
  *  'E' - Unsigned module has been loaded.
  *  'L' - A soft lockup has previously occurred.
  *  'K' - Kernel has been live patched.
+ *  'H' - System restored from unsafe hibernate snapshot image.
  *
  *	The string is overwritten by the next call to print_tainted().
  */
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 8608b3b..f2a7e21 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -79,6 +79,14 @@ config HIBERNATE_VERIFICATION
 	  relies on UEFI secure boot environment, EFI stub generates HMAC
 	  key for hibernate verification.
 
+config HIBERNATE_VERIFICATION_FORCE
+	bool "Require hibernate snapshot image to be validly signed"
+	depends on HIBERNATE_VERIFICATION
+	help
+	  Reject hibernate resuming from unsigned snapshot image or signed
+	  snapshot image for which we don't have a key. Without this, such
+	  snapshot image will simply taint the kernel when resuming.
+
 config ARCH_SAVE_PAGE_KEYS
 	bool
 
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 640ca8a..2c2cc90 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -43,6 +43,11 @@ static char resume_file[256] = CONFIG_PM_STD_PARTITION;
 dev_t swsusp_resume_device;
 sector_t swsusp_resume_block;
 __visible int in_suspend __nosavedata;
+#ifdef CONFIG_HIBERNATE_VERIFICATION_FORCE
+int sigenforce = 1;
+#else
+int sigenforce;
+#endif
 
 enum {
 	HIBERNATION_INVALID,
@@ -1119,6 +1124,8 @@ static int __init hibernate_setup(char *str)
 		noresume = 1;
 	else if (!strncmp(str, "nocompress", 10))
 		nocompress = 1;
+	else if (!strncmp(str, "sigenforce", 10))
+		sigenforce = 1;
 	else if (!strncmp(str, "no", 2)) {
 		noresume = 1;
 		nohibernate = 1;
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 3629249..486dd73 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1469,7 +1469,11 @@ error_digest:
 forward_ret:
 	if (ret)
 		pr_warn("PM: Signature verifying failed: %d\n", ret);
-	snapshot_fill_sig_forward_info(ret);
+	/* forward check result when verifying pass or not enforce verifying */
+	if (!ret || !sigenforce) {
+		snapshot_fill_sig_forward_info(ret);
+		ret = 0;
+	}
 	return ret;
 }
 
-- 
2.1.4

[PATCH v2 14/16] PM / hibernate: Allow user trigger hibernation key re-generating

[Lee, Chun-Yi]

This patch provides a ioctl for triggering hibernation key re-generating
process. It's allow user call ioctl to raise the flag of key re-generating.
Kernel writes a flag to a efi runtime variable, the GUID is
S4SignKeyRegen-fe141863-c070-478e-b8a3-878a5dc9ef21, then EFI stub will
re-generates hibernation key when queried flag.

To aviod the hibernation key changes in hibernating cycle that causes hiberne
restoring failed, this flag is only available when system runs normal
reboot or shutdown. The hibernate code will clean the flag when it raised
in a hiberante cycle.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 arch/x86/boot/compressed/eboot.c          | 19 ++++++++++++---
 arch/x86/power/hibernate_keys.c           |  2 ++
 drivers/firmware/Makefile                 |  1 +
 drivers/firmware/efi/Kconfig              |  4 ++++
 drivers/firmware/efi/Makefile             |  1 +
 drivers/firmware/efi/efi-hibernate_keys.c | 39 +++++++++++++++++++++++++++++++
 include/linux/suspend.h                   | 16 +++++++++++++
 include/uapi/linux/suspend_ioctls.h       |  3 ++-
 kernel/power/Kconfig                      |  1 +
 kernel/power/hibernate.c                  |  2 ++
 kernel/power/user.c                       | 10 ++++++++
 kernel/reboot.c                           |  3 +++
 12 files changed, 97 insertions(+), 4 deletions(-)
 create mode 100644 drivers/firmware/efi/efi-hibernate_keys.c

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c838d09..3228820 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1394,10 +1394,12 @@ static void setup_hibernation_keys(struct boot_params *params)
 {
 	unsigned long key_size;
 	unsigned long attributes;
+	unsigned long ignore;
 	struct setup_data *setup_data, *hibernation_setup_data;
 	struct hibernation_keys *keys;
+	bool regen_key = false;
 	unsigned long size = 0;
-	efi_status_t status;
+	efi_status_t status, reg_status;
 
 	/* Allocate setup_data to carry keys */
 	size = sizeof(struct setup_data) + sizeof(struct hibernation_keys);
@@ -1427,12 +1429,16 @@ static void setup_hibernation_keys(struct boot_params *params)
 			goto clean_fail;
 	}
 
-	if (status != EFI_SUCCESS) {
-		efi_printk(sys_table, "Failed to get existing hibernation key\n");
+	reg_status = efi_call_early(get_variable, HIBERNATION_KEY_REGEN_FLAG,
+				&EFI_HIBERNATION_GUID, NULL, &ignore, &regen_key);
+	if ((status != EFI_SUCCESS) ||
+	   (reg_status == EFI_SUCCESS && regen_key)) {
+		efi_printk(sys_table, "Regenerating hibernation key\n");
 
 		efi_get_random_key(sys_table, params, keys->hibernation_key,
 				   HIBERNATION_DIGEST_SIZE);
 
+		/* Set new hibernation key to bootservice non-volatile variable */
 		status = efi_call_early(set_variable, HIBERNATION_KEY,
 					&EFI_HIBERNATION_GUID,
 					HIBERNATION_KEY_ATTRIBUTE,
@@ -1440,6 +1446,13 @@ static void setup_hibernation_keys(struct boot_params *params)
 					keys->hibernation_key);
 		if (status != EFI_SUCCESS)
 			efi_printk(sys_table, "Failed to set hibernation key\n");
+
+		efi_call_early(get_variable, HIBERNATION_KEY, &EFI_HIBERNATION_GUID,
+				NULL, &key_size, keys->hibernation_key);
+
+		/* Clean key regenerate flag */
+		efi_call_early(set_variable, HIBERNATION_KEY_REGEN_FLAG,
+				&EFI_HIBERNATION_GUID, 0, 0, NULL);
 	}
 
 clean_fail:
diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c
index 51e808a..4fac9ab 100644
--- a/arch/x86/power/hibernate_keys.c
+++ b/arch/x86/power/hibernate_keys.c
@@ -165,6 +165,8 @@ static int __init init_hibernation_keys(void)
 	memblock_free(keys_phys_addr, sizeof(struct hibernation_keys));
 	keys_phys_addr = 0;
 
+	set_hibernation_key_regen_flag = false;
+
 	return ret;
 }
 
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 4a4b897..4474437 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -19,3 +19,4 @@ obj-y				+= broadcom/
 obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
 obj-$(CONFIG_EFI)		+= efi/
 obj-$(CONFIG_UEFI_CPER)		+= efi/
+obj-$(CONFIG_EFI_HIBERNATION_KEYS)	+= efi/
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 54071c1..bef29da 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -69,3 +69,7 @@ endmenu
 
 config UEFI_CPER
 	bool
+
+config EFI_HIBERNATION_KEYS
+	bool
+	select EFI_VARS
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 6fd3da9..d472c02 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_EFI)			+= efi.o vars.o reboot.o
 obj-$(CONFIG_EFI_VARS)			+= efivars.o
 obj-$(CONFIG_EFI_ESRT)			+= esrt.o
 obj-$(CONFIG_EFI_VARS_PSTORE)		+= efi-pstore.o
+obj-$(CONFIG_EFI_HIBERNATION_KEYS)	+= efi-hibernate_keys.o
 obj-$(CONFIG_UEFI_CPER)			+= cper.o
 obj-$(CONFIG_EFI_RUNTIME_MAP)		+= runtime-map.o
 obj-$(CONFIG_EFI_RUNTIME_WRAPPERS)	+= runtime-wrappers.o
diff --git a/drivers/firmware/efi/efi-hibernate_keys.c b/drivers/firmware/efi/efi-hibernate_keys.c
new file mode 100644
index 0000000..8a50bf1
--- /dev/null
+++ b/drivers/firmware/efi/efi-hibernate_keys.c
@@ -0,0 +1,39 @@
+/* EFI variable handler of hibernation key regen flag
+ *
+ * Copyright (C) 2015 Lee, Chun-Yi <jlee@suse.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#include <linux/efi.h>
+#include <linux/slab.h>
+#include <linux/suspend.h>
+
+void create_hibernation_key_regen_flag(void)
+{
+	struct efivar_entry *entry = NULL;
+	int err = 0;
+
+	if (!set_hibernation_key_regen_flag)
+		return;
+
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return;
+
+	memcpy(entry->var.VariableName,
+		HIBERNATION_KEY_REGEN_FLAG, sizeof(HIBERNATION_KEY_REGEN_FLAG));
+	memcpy(&(entry->var.VendorGuid),
+		&EFI_HIBERNATION_GUID, sizeof(efi_guid_t));
+
+	err = efivar_entry_set(entry, HIBERNATION_KEY_SEED_ATTRIBUTE,
+				sizeof(bool), &set_hibernation_key_regen_flag, NULL);
+	if (err)
+		pr_warn("PM: Set flag of regenerating hibernation key failed: %d\n", err);
+
+	kfree(entry);
+}
+EXPORT_SYMBOL_GPL(create_hibernation_key_regen_flag);
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index d318b72..dcca7ae 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -330,6 +330,11 @@ struct platform_hibernation_ops {
 
 #define EFI_HIBERNATION_GUID \
 	EFI_GUID(0xfe141863, 0xc070, 0x478e, 0xb8, 0xa3, 0x87, 0x8a, 0x5d, 0xc9, 0xef, 0x21)
+#define HIBERNATION_KEY_REGEN_FLAG \
+	((efi_char16_t [20]) { 'H', 'I', 'B', 'E', 'R', 'N', 'A', 'T', 'I', 'O', 'N', 'K', 'e', 'y', 'R', 'e', 'g', 'e', 'n', 0 })
+#define HIBERNATION_KEY_SEED_ATTRIBUTE	(EFI_VARIABLE_NON_VOLATILE | \
+					EFI_VARIABLE_BOOTSERVICE_ACCESS | \
+					EFI_VARIABLE_RUNTIME_ACCESS)
 
 /* HMAC Algorithm of Hibernate Signature */
 #define HIBERNATION_HMAC	"hmac(sha1)"
@@ -338,6 +343,16 @@ struct platform_hibernation_ops {
 /* kernel/power/hibernate.c */
 extern int sigenforce;
 
+/* kernel/power/user.c */
+extern bool set_hibernation_key_regen_flag;
+
+#ifdef CONFIG_HIBERNATE_VERIFICATION
+/* drivers/firmware/efi/efi-hibernate_keys.c */
+extern void create_hibernation_key_regen_flag(void);
+#else
+static inline void create_hibernation_key_regen_flag(void) {}
+#endif
+
 /* kernel/power/snapshot.c */
 extern void __register_nosave_region(unsigned long b, unsigned long e, int km);
 static inline void __init register_nosave_region(unsigned long b, unsigned long e)
@@ -370,6 +385,7 @@ static inline void hibernation_set_ops(const struct platform_hibernation_ops *op
 static inline int hibernate(void) { return -ENOSYS; }
 static inline bool system_entering_hibernation(void) { return false; }
 static inline bool hibernation_available(void) { return false; }
+static inline void create_hibernation_key_regen_flag(void) {}
 #endif /* CONFIG_HIBERNATION */
 
 /* Hibernation and suspend events */
diff --git a/include/uapi/linux/suspend_ioctls.h b/include/uapi/linux/suspend_ioctls.h
index 0b30382..0a08450 100644
--- a/include/uapi/linux/suspend_ioctls.h
+++ b/include/uapi/linux/suspend_ioctls.h
@@ -28,6 +28,7 @@ struct resume_swap_area {
 #define SNAPSHOT_PREF_IMAGE_SIZE	_IO(SNAPSHOT_IOC_MAGIC, 18)
 #define SNAPSHOT_AVAIL_SWAP_SIZE	_IOR(SNAPSHOT_IOC_MAGIC, 19, __kernel_loff_t)
 #define SNAPSHOT_ALLOC_SWAP_PAGE	_IOR(SNAPSHOT_IOC_MAGIC, 20, __kernel_loff_t)
-#define SNAPSHOT_IOC_MAXNR	20
+#define SNAPSHOT_REGENERATE_KEY		_IO(SNAPSHOT_IOC_MAGIC, 21)
+#define SNAPSHOT_IOC_MAXNR	21
 
 #endif /* _LINUX_SUSPEND_IOCTLS_H */
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index f2a7e21..1a03777 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -71,6 +71,7 @@ config HIBERNATE_VERIFICATION
 	depends on HIBERNATION
 	depends on EFI_STUB
 	depends on X86
+	select EFI_HIBERNATION_KEYS
 	select CRYPTO_HMAC
 	select CRYPTO_SHA1
 	help
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 2c2cc90..8bdb4bb 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -653,6 +653,8 @@ int hibernate(void)
 {
 	int error;
 
+	set_hibernation_key_regen_flag = false;
+
 	if (!hibernation_available()) {
 		pr_debug("PM: Hibernation not available.\n");
 		return -EPERM;
diff --git a/kernel/power/user.c b/kernel/power/user.c
index ffd327a..a183abd 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -32,6 +32,9 @@
 
 #define SNAPSHOT_MINOR	231
 
+/* Handler will create HIBERNATIONKeyRegen EFI variable when flag raised */
+bool set_hibernation_key_regen_flag;
+
 static struct snapshot_data {
 	struct snapshot_handle handle;
 	int swap;
@@ -338,6 +341,8 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 			error = -EPERM;
 			break;
 		}
+		/* clean flag to avoid hibernation key regenerated */
+		set_hibernation_key_regen_flag = false;
 		/*
 		 * Tasks are frozen and the notifiers have been called with
 		 * PM_HIBERNATION_PREPARE
@@ -351,6 +356,7 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 		break;
 
 	case SNAPSHOT_POWER_OFF:
+		set_hibernation_key_regen_flag = false;
 		if (data->platform_support)
 			error = hibernation_platform_enter();
 		break;
@@ -386,6 +392,10 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 		}
 		break;
 
+	case SNAPSHOT_REGENERATE_KEY:
+		set_hibernation_key_regen_flag = !!arg;
+		break;
+
 	default:
 		error = -ENOTTY;
 
diff --git a/kernel/reboot.c b/kernel/reboot.c
index d20c85d..f83b88a 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -213,6 +213,7 @@ void migrate_to_reboot_cpu(void)
  */
 void kernel_restart(char *cmd)
 {
+	create_hibernation_key_regen_flag();
 	kernel_restart_prepare(cmd);
 	migrate_to_reboot_cpu();
 	syscore_shutdown();
@@ -240,6 +241,7 @@ static void kernel_shutdown_prepare(enum system_states state)
  */
 void kernel_halt(void)
 {
+	create_hibernation_key_regen_flag();
 	kernel_shutdown_prepare(SYSTEM_HALT);
 	migrate_to_reboot_cpu();
 	syscore_shutdown();
@@ -256,6 +258,7 @@ EXPORT_SYMBOL_GPL(kernel_halt);
  */
 void kernel_power_off(void)
 {
+	create_hibernation_key_regen_flag();
 	kernel_shutdown_prepare(SYSTEM_POWER_OFF);
 	if (pm_power_off_prepare)
 		pm_power_off_prepare();
-- 
2.1.4

[PATCH v2 15/16] PM / hibernate: Bypass verification logic on legacy BIOS

[Lee, Chun-Yi]

Current hibernate signature verification solution relies on EFI stub
and efi boot service variable on x86 architecture. So the verification
logic was bypassed on legacy BIOS through checking EFI_BOOT flag.

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/firmware/efi/efi-hibernate_keys.c | 3 +++
 kernel/power/Kconfig                      | 3 ++-
 kernel/power/snapshot.c                   | 8 ++++++--
 kernel/power/user.c                       | 6 +++++-
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/efi-hibernate_keys.c b/drivers/firmware/efi/efi-hibernate_keys.c
index 8a50bf1..2125302 100644
--- a/drivers/firmware/efi/efi-hibernate_keys.c
+++ b/drivers/firmware/efi/efi-hibernate_keys.c
@@ -17,6 +17,9 @@ void create_hibernation_key_regen_flag(void)
 	struct efivar_entry *entry = NULL;
 	int err = 0;
 
+	if (!efi_enabled(EFI_RUNTIME_SERVICES))
+		return;
+
 	if (!set_hibernation_key_regen_flag)
 		return;
 
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 1a03777..c30598e 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -78,7 +78,8 @@ config HIBERNATE_VERIFICATION
 	  This option provides support for generating and verifying the
 	  signature of memory snapshot image by HMAC-SHA1. Current mechanism
 	  relies on UEFI secure boot environment, EFI stub generates HMAC
-	  key for hibernate verification.
+	  key for hibernate verification. So, the verification logic will be
+	  bypassed on legacy BIOS.
 
 config HIBERNATE_VERIFICATION_FORCE
 	bool "Require hibernate snapshot image to be validly signed"
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 486dd73..22b80b7 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -29,6 +29,7 @@
 #include <linux/slab.h>
 #include <linux/compiler.h>
 #include <linux/ktime.h>
+#include <linux/efi.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -1469,8 +1470,11 @@ error_digest:
 forward_ret:
 	if (ret)
 		pr_warn("PM: Signature verifying failed: %d\n", ret);
-	/* forward check result when verifying pass or not enforce verifying */
-	if (!ret || !sigenforce) {
+	if (ret == -ENODEV && !efi_enabled(EFI_BOOT)) {
+		pr_warn("PM: Bypass verification on non-EFI machine\n");
+		ret = 0;
+	} else if (!ret || !sigenforce) {
+		/* forward check result when verifying pass or not enforce verifying */
 		snapshot_fill_sig_forward_info(ret);
 		ret = 0;
 	}
diff --git a/kernel/power/user.c b/kernel/power/user.c
index a183abd..686d095 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -24,6 +24,7 @@
 #include <linux/console.h>
 #include <linux/cpu.h>
 #include <linux/freezer.h>
+#include <linux/efi.h>
 
 #include <asm/uaccess.h>
 
@@ -393,7 +394,10 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 		break;
 
 	case SNAPSHOT_REGENERATE_KEY:
-		set_hibernation_key_regen_flag = !!arg;
+		if (!efi_enabled(EFI_BOOT))
+			error = -ENODEV;
+		else
+			set_hibernation_key_regen_flag = !!arg;
 		break;
 
 	default:
-- 
2.1.4

[PATCH v2 16/16] PM / hibernate: Document signature verification of hibernate snapshot

[Lee, Chun-Yi]

Reviewed-by: Jiri Kosina <jkosina@suse.com>
Tested-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 Documentation/power/swsusp-signature-verify.txt | 86 +++++++++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/power/swsusp-signature-verify.txt

diff --git a/Documentation/power/swsusp-signature-verify.txt b/Documentation/power/swsusp-signature-verify.txt
new file mode 100644
index 0000000..7ba4a3f
--- /dev/null
+++ b/Documentation/power/swsusp-signature-verify.txt
@@ -0,0 +1,86 @@
+Signature verification of hibernate snapshot
+============================================
+
+1) Introduction
+2) How to enable
+3) How does it work
+4) Trigger key re-generate
+
+1) Introduction
+---------------------
+
+The hibernate function provided by kernel was used to snapshot memory
+to be a image for keeping in storage, then restored in appropriate time.
+There have potential threat from hacking the memory snapshot image.
+Cracker may triggers hibernating process through ioctl to grab snapshot
+image, then restoring modified image back to memory. Another situation
+is booting to other hacked OS to modify the snapshot image in swap
+partition or file, then user may runs malware after image restored to
+memory. In addition, the above weakness cause kernel is not fully trusted
+in EFI secure boot environment.
+
+So, kernel hibernate function needs a mechanism to verify integrity of
+hibernate snapshot image.
+
+The origin idea is from Jiri Kosina: Let EFI bootloader generates key-pair
+in UEFI secure boot environment, then forwarding keys to boot kernel for
+signing/verifying snapshot image.
+
+
+2) How to enable
+-----------------
+
+If the HIBERNATE_VERIFICATION compile option is true, kernel hibernate code
+will generating and verifying the signature of memory snapshot image by
+HMAC-SHA1 algorithm. Current solution relies on EFI stub on x86 architecture,
+so the signature verification logic will be bypassed on legacy BIOS.
+
+When the snapshot image unsigned or signed with an unknown key, the signature
+verification will be failed. The default behavior of verifying failed is
+accept restoring image but tainting kernel with H taint flag.
+
+Like kernel module signature checking, there's both a config option and
+a boot parameter which control whether we accept or stop whole recovery
+process when verification failed. Using HIBERNATE_VERIFICATION_FORCE kernel
+compile option or "sigenforce" kernel parameter to force hibernate recovery
+process stop when verification failed.
+
+
+3) How does it work
+-------------------
+
+For signing hibernate image, kernel need a key for generating signature of
+image. The origin idea is using PKI, the EFI bootloader, shim generates key
+pair and forward to boot kernel for signing/verifying image. In Linux Plumbers
+Conference 2013, we got response from community experts for just using
+symmetric key algorithm to generate signature, that's simpler and no EFI
+bootloader's involving.
+
+Current solution is using HMAC-SHA1 algorithm, it generating HMAC key in EFI
+stub by using RDRAND, RDTSC and EFI RNG protocol to grab random number to be
+the entropy of key. Then the HMAC key stored in efi boot service variable,
+key's security relies on EFI secure boot: When EFI secure boot enabled, only
+trusted efi program allowed accessing boot service variables.
+
+In every EFI stub booting stage, it loads key from variable then forward key
+to boot kernel for waiting to sign snapshot image by user trigger hibernating.
+The HMAC-SHA1 algorithm generates signature then kernel put signature to the
+header with the memory snapshot image. The signature with image is delivered
+to userspace hibernating application or direct stored in swap partition.
+
+When hibernate recovering, kernel will verify the image signature before
+switch whole system to image kernel and image memory space. When verifying
+failed, kernel is tainted or stop recovering and discarding image.
+
+
+4) Trigger key re-generate
+--------------------------
+
+The hibernate signature verifying function allows user to trigger the key
+re-generating process in EFI stub through SNAPSHOT_REGENERATE_KEY ioctl.
+
+User can raise a key-regen flag in kernel through ioctl. When system runs
+normal shutdown or reboot, kernel writes a efi runtime variable as a flag
+then EFI stub will query the flag in next boot cycle. To avoid the hibernation
+key changes in hibernating cycle that causes hibernate restoring failed,
+the regen flag will be clear in a hibernate cycle.
-- 
2.1.4

Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints

[Chen, Yu C]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 893 bytes --]

Hi Chun-yi,
On Tue, 2015-08-11 at 14:16 +0800, Lee, Chun-Yi wrote:

> +/* A page used to keep hibernation keys */
> +static struct hibernation_keys *hibernation_keys;
> +
> +void __init parse_hibernation_keys(u64 phys_addr, u32 data_len)
> +{
> +	struct setup_data *hibernation_setup_data;
> +
> +	/* Reserve keys memory, will copy and erase in init_hibernation_keys() */
> +	keys_phys_addr = phys_addr + sizeof(struct setup_data);
> +	memblock_reserve(keys_phys_addr, sizeof(struct hibernation_keys));
> +
> +	/* clear hibernation_data */
> +	hibernation_setup_data = early_memremap(phys_addr, data_len);
> +	if (!hibernation_setup_data)
> +		return;
> +
should we invoke memblock_free if early_memremap failed?

Best Regards,
Yu


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints

[joeyli]

Hi Yu, 

Thanks for your reviewing.

On Thu, Aug 13, 2015 at 02:45:32AM +0000, Chen, Yu C wrote:
> Hi Chun-yi,
> On Tue, 2015-08-11 at 14:16 +0800, Lee, Chun-Yi wrote:
> 
> > +/* A page used to keep hibernation keys */
> > +static struct hibernation_keys *hibernation_keys;
> > +
> > +void __init parse_hibernation_keys(u64 phys_addr, u32 data_len)
> > +{
> > +	struct setup_data *hibernation_setup_data;
> > +
> > +	/* Reserve keys memory, will copy and erase in init_hibernation_keys() */
> > +	keys_phys_addr = phys_addr + sizeof(struct setup_data);
> > +	memblock_reserve(keys_phys_addr, sizeof(struct hibernation_keys));
> > +
> > +	/* clear hibernation_data */
> > +	hibernation_setup_data = early_memremap(phys_addr, data_len);
> > +	if (!hibernation_setup_data)
> > +		return;
> > +
> should we invoke memblock_free if early_memremap failed?
> 
> Best Regards,
> Yu
> 
> 

Should not free memblock reservation of key data.

Using memblock_reserve is for reserve hibernation key until
init_hibernation_keys() copy key data to arbitrary allocated page.
Using early_memremap is for cleaning the setup_data header that's
used to carry key. That above 2 actions are different purposes.

So, even the action of cleaning setup_data failed, it doesn't
affect to the action for reserve hibernation key by memblock until
copy it.


Thanks a lot!
Joey Lee

Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints

[joeyli]

On Tue, Aug 11, 2015 at 02:16:29PM +0800, Lee, Chun-Yi wrote:
> Add handler to parse the setup data that carrying hibernation key, it
> reserves hibernation key by memblock then copies key to a allocated page
> in later initcall stage.
>
[...snip] 
> diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c
> new file mode 100644
> index 0000000..357dc0e
> --- /dev/null
> +++ b/arch/x86/power/hibernate_keys.c
> @@ -0,0 +1,78 @@
> +/* Hibernation keys handler
> + *
[...snip]
> +static int __init init_hibernation_keys(void)
> +{
> +	struct hibernation_keys *keys;
> +	int ret = 0;
> +
> +	if (!keys_phys_addr)
> +		return -ENODEV;
> +
> +	keys = early_memremap(keys_phys_addr, sizeof(struct hibernation_keys));
> +
> +	/* Copy hibernation keys to a allocated page */
> +	hibernation_keys = (struct hibernation_keys *)get_zeroed_page(GFP_KERNEL);
> +	if (hibernation_keys) {
> +		*hibernation_keys = *keys;
> +	} else {
> +		pr_err("PM: Allocate hibernation keys page failed\n");
> +		ret = -ENOMEM;
> +	}
> +
> +	/* Erase keys data no matter copy success or failed */
> +	memset(keys, 0, sizeof(struct hibernation_keys));
> +	early_memunmap(keys, sizeof(struct hibernation_keys));
> +	memblock_free(keys_phys_addr, sizeof(struct hibernation_keys));
> +	keys_phys_addr = 0;
> +
> +	return ret;
> +}
> +
> +late_initcall(init_hibernation_keys);

Yu's reviewing triggered me rethinking...

I afraid that's too late in late_initcall stage for copying key data that
reserved by memblock. I will try to move this copy action to setup_arch().


Regards
Joey Lee

Re: [PATCH v2 08/16] x86/efi: Carrying hibernation key by setup data

[Pavel Machek]

On Tue 2015-08-11 14:16:28, Lee, Chun-Yi wrote:
> For forwarding hibernation key from EFI stub to boot kernel, this patch
> allocates setup data for carrying hibernation key, size and the status
> of efi operating.
> 
> Reviewed-by: Jiri Kosina <jkosina@suse.com>

Jiri, are you sure you reviewed these? This is not really
english, afaict, and efi/EFI should be spelled consistently.

Could you try reviewing it again? Pointing out 10s of small
bugs is quite boring...

>  	unsigned long key_size;
>  	unsigned long attributes;
> +	struct setup_data *setup_data, *hibernation_setup_data;
>  	struct hibernation_keys *keys;
> +	unsigned long size = 0;
>  	efi_status_t status;
>  
>  	/* Allocate setup_data to carry keys */
> +	size = sizeof(struct setup_data) + sizeof(struct hibernation_keys);
>  	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> -				sizeof(struct hibernation_keys), &keys);
> +				size, &hibernation_setup_data);
>  	if (status != EFI_SUCCESS) {
>  		efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n");
>  		return;
>  	}
>  
> -	memset(keys, 0, sizeof(struct hibernation_keys));
> +	memset(hibernation_setup_data, 0, size);
> +	keys = (struct hibernation_keys *) hibernation_setup_data->data;
>  

any chance to type stuff correctly so that casts are not
neccessary?

> +clean_fail:
> +	hibernation_setup_data->type = SETUP_HIBERNATION_KEYS;
> +	hibernation_setup_data->len = sizeof(struct hibernation_keys);
> +	hibernation_setup_data->next = 0;
> +	keys->hkey_status = efi_status_to_err(status);
> +
> +	setup_data = (struct setup_data *)params->hdr.setup_data;
> +	while (setup_data && setup_data->next)
> +		setup_data = (struct setup_data *)setup_data->next;

way too many casts here.

							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2 08/16] x86/efi: Carrying hibernation key by setup data

[joeyli]

On Sat, Aug 15, 2015 at 07:07:38PM +0200, Pavel Machek wrote:
> On Tue 2015-08-11 14:16:28, Lee, Chun-Yi wrote:
> > For forwarding hibernation key from EFI stub to boot kernel, this patch
> > allocates setup data for carrying hibernation key, size and the status
> > of efi operating.
> > 
> > Reviewed-by: Jiri Kosina <jkosina@suse.com>
> 
> Jiri, are you sure you reviewed these? This is not really
> english, afaict, and efi/EFI should be spelled consistently.
> 
> Could you try reviewing it again? Pointing out 10s of small
> bugs is quite boring...
>

It's my fault, I will find someone help to review my English in all
patch description for next edition.
 
> >  	unsigned long key_size;
> >  	unsigned long attributes;
> > +	struct setup_data *setup_data, *hibernation_setup_data;
> >  	struct hibernation_keys *keys;
> > +	unsigned long size = 0;
> >  	efi_status_t status;
> >  
> >  	/* Allocate setup_data to carry keys */
> > +	size = sizeof(struct setup_data) + sizeof(struct hibernation_keys);
> >  	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> > -				sizeof(struct hibernation_keys), &keys);
> > +				size, &hibernation_setup_data);
> >  	if (status != EFI_SUCCESS) {
> >  		efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n");
> >  		return;
> >  	}
> >  
> > -	memset(keys, 0, sizeof(struct hibernation_keys));
> > +	memset(hibernation_setup_data, 0, size);
> > +	keys = (struct hibernation_keys *) hibernation_setup_data->data;
> >  
> 
> any chance to type stuff correctly so that casts are not
> neccessary?
> 

The data element defined in setup_data struct as u8:
        __u8 data[0];

So I use cast here.

> > +clean_fail:
> > +	hibernation_setup_data->type = SETUP_HIBERNATION_KEYS;
> > +	hibernation_setup_data->len = sizeof(struct hibernation_keys);
> > +	hibernation_setup_data->next = 0;
> > +	keys->hkey_status = efi_status_to_err(status);
> > +
> > +	setup_data = (struct setup_data *)params->hdr.setup_data;
> > +	while (setup_data && setup_data->next)
> > +		setup_data = (struct setup_data *)setup_data->next;
> 
> way too many casts here.
> 
> 							Pavel

I follow setup_efi_pci32() and setup_efi_pci64():

        while (data && data->next)
                data = (struct setup_data *)(unsigned long)data->next;

That's better I also add (unsigned long) cast as setup_efi_pci. Do
you have good suggestion let the above code more graceful?


Thanks a lot!
Joey Lee

Re: [PATCH v2 08/16] x86/efi: Carrying hibernation key by setup data

[Jiri Kosina]

On Sat, 15 Aug 2015, Pavel Machek wrote:

> > For forwarding hibernation key from EFI stub to boot kernel, this patch
> > allocates setup data for carrying hibernation key, size and the status
> > of efi operating.
> > 
> > Reviewed-by: Jiri Kosina <jkosina@suse.com>
> 
> Jiri, are you sure you reviewed these? This is not really
> english, afaict, and efi/EFI should be spelled consistently.

Yes, I did review the code and the fact that it is still in compliance 
with my original idea. I think you can blame me on not reviewing 
changelogs super-rigorously though, so all suggestions for improvements 
are absolutely welcome.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 08/16] x86/efi: Carrying hibernation key by setup data

[Nigel Cunningham]

Hi all.

I've rejoined LKML, so I'll try to help with reviewing PM patches. I'd forgotten how much it is a case of sipping at a fire hydrant!

Regards,

Nigel

On 17/08/15 07:23, Jiri Kosina wrote:
> On Sat, 15 Aug 2015, Pavel Machek wrote:
>
>>> For forwarding hibernation key from EFI stub to boot kernel, this patch
>>> allocates setup data for carrying hibernation key, size and the status
>>> of efi operating.
>>>
>>> Reviewed-by: Jiri Kosina <jkosina@suse.com>
>> Jiri, are you sure you reviewed these? This is not really
>> english, afaict, and efi/EFI should be spelled consistently.
> Yes, I did review the code and the fact that it is still in compliance 
> with my original idea. I think you can blame me on not reviewing 
> changelogs super-rigorously though, so all suggestions for improvements 
> are absolutely welcome.
>
> Thanks,
>

Re: [PATCH v2 02/16] x86/efi: Add get and set variable to EFI services pointer table

[Matt Fleming]

On Tue, 11 Aug, at 02:16:22PM, Lee, Chun-Yi wrote:
> Add get variable and set variable function to EFI services pointer
> table for supporting later functions of hibernate signature
> verification to keep the HMAC key in efi boot service variable.
> EFI boot stub needs get/set_variable functions for accessing key.
> 
> Reviewed-by: Jiri Kosina <jkosina@suse.com>
> Tested-by: Jiri Kosina <jkosina@suse.com>
> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> ---
>  arch/x86/boot/compressed/eboot.c   | 4 ++++
>  arch/x86/boot/compressed/head_32.S | 6 +++---
>  arch/x86/boot/compressed/head_64.S | 8 ++++----
>  arch/x86/include/asm/efi.h         | 2 ++
>  4 files changed, 13 insertions(+), 7 deletions(-)

Reviewed-by: Matt Fleming <matt.fleming@intel.com>

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 04/16] x86/efi: Generating random number in EFI stub

[Matt Fleming]

On Tue, 11 Aug, at 02:16:24PM, Lee, Chun-Yi wrote:
> This patch adds the codes for generating random number array as the
> HMAC key that will used by later EFI stub codes.
> 
> The original codes in efi_random copied from aslr and add the codes
> to accept input entropy and EFI debugging. In later patch will add
> the codes to get random number by EFI protocol. The separate codes
> can avoid impacting aslr function.
 
Is there some way we can share the code between aslr and the EFI boot
stub? People may not review both files when making changes and so bug
fixes to one might not appear in the other.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 05/16] x86/efi: Get entropy through EFI random number generator protocol

[Matt Fleming]

On Tue, 11 Aug, at 02:16:25PM, Lee, Chun-Yi wrote:
> To grab random numbers through EFI protocol as one of the entropies
> source of swsusp key, this patch adds the logic for accessing EFI RNG
> (random number generator) protocol that's introduced since UEFI 2.4.
> 
> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> ---
>  arch/x86/boot/compressed/efi_random.c | 209 ++++++++++++++++++++++++++++++++++
>  include/linux/efi.h                   |  13 +++
>  2 files changed, 222 insertions(+)

[...]

Most of this looks good.

> +static unsigned long efi_get_rng(efi_system_table_t *sys_table)
> +{
> +	const struct efi_config *efi_early = __efi_early();
> +	unsigned long random = 0;
> +	efi_status_t status;
> +	void **rng_handle = NULL;
> +
> +	status = efi_locate_rng(sys_table, &rng_handle);
> +	if (status != EFI_SUCCESS)
> +		return 0;
> +
> +	if (efi_early->is64)
> +		random = efi_get_rng64(sys_table, rng_handle);
> +	else
> +		random = efi_get_rng32(sys_table, rng_handle);
> +
> +	efi_call_early(free_pool, rng_handle);
> +
> +	return random;
> +}
  
I think this function is named particularly poorly in the UEFI spec
(GetRNG), can we maybe make this efi_get_rng_number(),
efi_get_rng_value() or efi_get_rng_long() to match the existing
naming?

> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 85ef051..8914d60 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -427,6 +427,16 @@ typedef struct {
>  #define EFI_PCI_IO_ATTRIBUTE_VGA_PALETTE_IO_16 0x20000
>  #define EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 0x40000
>  
> +typedef struct {
> +	u32 get_info;
> +	u32 get_rng;
> +} efi_rng_protocol_32;
> +
> +typedef struct {
> +	u64 get_info;
> +	u64 get_rng;
> +} efi_rng_protocol_64;
> +

We've been kinda slack with the naming conventions in efi.h but these
should really both be 'efi_rng_protocol_*_t'.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 07/16] efi: Make efi_status_to_err() public

[Matt Fleming]

On Tue, 11 Aug, at 02:16:27PM, Lee, Chun-Yi wrote:
> Moved the function of transferring EFI status to kernel error for
> later used by EFI stub.
 
Might I suggest:

 "Move the function for converting EFI status to kernel error values
  from drivers/firmware/efi/ to include/linux/efi.h for use by the x86
  EFI stub in an upcoming patch."

?

> Reviewed-by: Jiri Kosina <jkosina@suse.com>
> Tested-by: Jiri Kosina <jkosina@suse.com>
> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> ---
>  drivers/firmware/efi/vars.c | 33 ---------------------------------
>  include/linux/efi.h         | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 33 deletions(-)

The patch content is fine,

Reviewed-by: Matt Fleming <matt.fleming@intel.com>

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 05/16] x86/efi: Get entropy through EFI random number generator protocol

[Matt Fleming]

On Tue, 11 Aug, at 02:16:25PM, Lee, Chun-Yi wrote:
> +
> +static unsigned long efi_get_rng64(efi_system_table_t *sys_table,
> +				   void **rng_handle)
> +{
> +	const struct efi_config *efi_early = __efi_early();
> +	efi_rng_protocol_64 *rng = NULL;
> +	efi_guid_t rng_proto = EFI_RNG_PROTOCOL_GUID;
> +	u64 *handles = (u64 *)(unsigned long)rng_handle;
> +	efi_status_t status;
> +	unsigned long rng_number;
> +
> +	status = efi_call_early(handle_protocol, handles[0],
> +				&rng_proto, (void **)&rng);
> +	if (status != EFI_SUCCESS)
> +		efi_printk(sys_table, "Failed to get EFI_RNG_PROTOCOL handles\n");
> +
> +	if (status == EFI_SUCCESS && rng) {
> +		status = efi_early->call((unsigned long)rng->get_rng, rng, NULL,
> +					sizeof(rng_number), &rng_number);

Actually, one thing just occurred to me - you're not passing an
RNGAlgorithm value and are relying upon the firmware's default
implementation.

I don't think that's a safe bet, the default could be anything and
might vary across implementations.

Can we do a little better here and pick a "preferred" algorithm
instead of the default?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 06/16] x86/efi: Generating random HMAC key for siging hibernate image

[Matt Fleming]

On Tue, 11 Aug, at 02:16:26PM, Lee, Chun-Yi wrote:
> This patch adds codes in EFI stub for generating and storing the
> HMAC key in EFI boot service variable for signing hibernate image.
> 
> Per rcf2104, the length of HMAC-SHA1 hash result is 20 bytes, and
> it recommended the length of key the same with hash rsult, means
> also 20 bytes. Using longer key would not significantly increase
> the function strength. Due to the nvram space is limited in some
> UEFI machines, so using the minimal recommended length 20 bytes
> key that will stored in boot service variable.

I'm having a hard time understanding the middle part of this
paragraph, specifically the part of the key and the hash result.
There's a typo in the subject too s/siging/signing/and "Generating"
should be "Generate".

> The HMAC key stored in EFI boot service variable, GUID is
> HIBERNATIONKey-fe141863-c070-478e-b8a3-878a5dc9ef21.
 
I'd really like to see some of the explanation from your cover letter
included in the commit message for this patch, and in particular why
signing hibernate images is a good thing.

Recording that for posterity in the commit message is going to be
helpful when someone looks at this patch in 2 years time and wonders
why RNG support was added to the EFI stub and why they might want to
sign hibernate images.

> Reviewed-by: Jiri Kosina <jkosina@suse.com>
> Tested-by: Jiri Kosina <jkosina@suse.com>
> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> ---
>  arch/x86/boot/compressed/eboot.c | 60 ++++++++++++++++++++++++++++++++++++++++
>  arch/x86/include/asm/suspend.h   |  9 ++++++
>  include/linux/suspend.h          |  3 ++
>  3 files changed, 72 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 0ffb6db..463aa9b 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -12,6 +12,7 @@
>  #include <asm/efi.h>
>  #include <asm/setup.h>
>  #include <asm/desc.h>
> +#include <asm/suspend.h>
>  
>  #include "../string.h"
>  #include "eboot.h"
> @@ -1383,6 +1384,63 @@ free_mem_map:
>  	return status;
>  }
>  
> +#ifdef CONFIG_HIBERNATE_VERIFICATION
> +#define HIBERNATION_KEY \
> +	((efi_char16_t [15]) { 'H', 'I', 'B', 'E', 'R', 'N', 'A', 'T', 'I', 'O', 'N', 'K', 'e', 'y', 0 })
> +#define HIBERNATION_KEY_ATTRIBUTE	(EFI_VARIABLE_NON_VOLATILE | \
> +					EFI_VARIABLE_BOOTSERVICE_ACCESS)
> +
> +static void setup_hibernation_keys(struct boot_params *params)
> +{
> +	unsigned long key_size;
> +	unsigned long attributes;
> +	struct hibernation_keys *keys;
> +	efi_status_t status;
> +
> +	/* Allocate setup_data to carry keys */
> +	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> +				sizeof(struct hibernation_keys), &keys);
> +	if (status != EFI_SUCCESS) {
> +		efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n");
> +		return;
> +	}
> +
> +	memset(keys, 0, sizeof(struct hibernation_keys));
> +
> +	status = efi_call_early(get_variable, HIBERNATION_KEY,
> +				&EFI_HIBERNATION_GUID, &attributes,
> +				&key_size, keys->hibernation_key);

Tiny nit, but could you put a new line here please? This is a large
chunk of code.

> +	if (status == EFI_SUCCESS && attributes != HIBERNATION_KEY_ATTRIBUTE) {
> +		efi_printk(sys_table, "A hibernation key is not boot service variable\n");
> +		memset(keys->hibernation_key, 0, HIBERNATION_DIGEST_SIZE);
> +		status = efi_call_early(set_variable, HIBERNATION_KEY,
> +					&EFI_HIBERNATION_GUID, attributes, 0,
> +					NULL);
> +		if (status == EFI_SUCCESS) {
> +			efi_printk(sys_table, "Cleaned existing hibernation key\n");
> +			status = EFI_NOT_FOUND;
> +		}
> +	}

Hmm.. it's not clear to me that we should be deleting this EFI
variable if the attributes are bogus. It would be safer to just bail.

> +
> +	if (status != EFI_SUCCESS) {
> +		efi_printk(sys_table, "Failed to get existing hibernation key\n");
> +
> +		efi_get_random_key(sys_table, params, keys->hibernation_key,
> +				   HIBERNATION_DIGEST_SIZE);
> +
> +		status = efi_call_early(set_variable, HIBERNATION_KEY,
> +					&EFI_HIBERNATION_GUID,
> +					HIBERNATION_KEY_ATTRIBUTE,
> +					HIBERNATION_DIGEST_SIZE,
> +					keys->hibernation_key);
> +		if (status != EFI_SUCCESS)
> +			efi_printk(sys_table, "Failed to set hibernation key\n");

You're leaking 'keys' here.

> diff --git a/arch/x86/include/asm/suspend.h b/arch/x86/include/asm/suspend.h
> index 2fab6c2..ab463c4 100644
> --- a/arch/x86/include/asm/suspend.h
> +++ b/arch/x86/include/asm/suspend.h
> @@ -3,3 +3,12 @@
>  #else
>  # include <asm/suspend_64.h>
>  #endif
> +
> +#ifdef CONFIG_HIBERNATE_VERIFICATION
> +#include <linux/suspend.h>
> +
> +struct hibernation_keys {
> +	unsigned long hkey_status;
> +	u8 hibernation_key[HIBERNATION_DIGEST_SIZE];
> +};
> +#endif

Have you given any thought to how things are going to work if we
change the hash function in the future, or provide a choice? That
information doesn't appear anywhere in the above struct.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 08/16] x86/efi: Carrying hibernation key by setup data

[Matt Fleming]

On Tue, 11 Aug, at 02:16:28PM, Lee, Chun-Yi wrote:
> For forwarding hibernation key from EFI stub to boot kernel, this patch
> allocates setup data for carrying hibernation key, size and the status
> of efi operating.
 
This could do with some more information, and include that the key is
used to validate hibernate images.

But now that I think about it, is there a reason this patch hasn't
been merged with patch 6? The memory leak I mentioned in patch 6
becomes a non-issue in this one, so it would be good if these two
could be squashed together.
  
> Reviewed-by: Jiri Kosina <jkosina@suse.com>
> Tested-by: Jiri Kosina <jkosina@suse.com>
> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> ---
>  arch/x86/boot/compressed/eboot.c      | 26 +++++++++++++++++++++++---
>  arch/x86/include/uapi/asm/bootparam.h |  1 +
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 463aa9b..c838d09 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -1394,18 +1394,22 @@ static void setup_hibernation_keys(struct boot_params *params)
>  {
>  	unsigned long key_size;
>  	unsigned long attributes;
> +	struct setup_data *setup_data, *hibernation_setup_data;
>  	struct hibernation_keys *keys;
> +	unsigned long size = 0;
>  	efi_status_t status;

One thing to be aware of is that eboot.c has mainly used the
"reverse-christmas-tree" style of variable declarations, with longer
lines first, and shorter ones following. I haven't mentioned it before
because none of your changes seemed to be too different (and it's not
a tree-wide convention), but the above setup_data line goes a bit too
far.

Could you try and keep them ordered, longest line first?

>  
>  	/* Allocate setup_data to carry keys */
> +	size = sizeof(struct setup_data) + sizeof(struct hibernation_keys);
>  	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> -				sizeof(struct hibernation_keys), &keys);
> +				size, &hibernation_setup_data);
>  	if (status != EFI_SUCCESS) {
>  		efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n");
>  		return;
>  	}
>  
> -	memset(keys, 0, sizeof(struct hibernation_keys));
> +	memset(hibernation_setup_data, 0, size);
> +	keys = (struct hibernation_keys *) hibernation_setup_data->data;
>  
>  	status = efi_call_early(get_variable, HIBERNATION_KEY,
>  				&EFI_HIBERNATION_GUID, &attributes,
> @@ -1419,7 +1423,8 @@ static void setup_hibernation_keys(struct boot_params *params)
>  		if (status == EFI_SUCCESS) {
>  			efi_printk(sys_table, "Cleaned existing hibernation key\n");
>  			status = EFI_NOT_FOUND;
> -		}
> +		} else
> +			goto clean_fail;

Please add braces for the 'else' clause. Also, please include a
comment stating that the reason you jump to the label instead of
returning is so that the EFI status error code can be recorded in
hibernation_setup_data.

>  	}
>  
>  	if (status != EFI_SUCCESS) {
> @@ -1436,6 +1441,21 @@ static void setup_hibernation_keys(struct boot_params *params)
>  		if (status != EFI_SUCCESS)
>  			efi_printk(sys_table, "Failed to set hibernation key\n");
>  	}
> +
> +clean_fail:
> +	hibernation_setup_data->type = SETUP_HIBERNATION_KEYS;
> +	hibernation_setup_data->len = sizeof(struct hibernation_keys);
> +	hibernation_setup_data->next = 0;
> +	keys->hkey_status = efi_status_to_err(status);
> +
> +	setup_data = (struct setup_data *)params->hdr.setup_data;
> +	while (setup_data && setup_data->next)
> +		setup_data = (struct setup_data *)setup_data->next;
> +
> +	if (setup_data)
> +		setup_data->next = (unsigned long)hibernation_setup_data;
> +	else
> +		params->hdr.setup_data = (unsigned long)hibernation_setup_data;

This label name is a little confusing because you reach it both when
the EFI boot variable was successfully created and when a bogus EFI
variable failed to be deleted, i.e. it's not always reached because of
a failure.

How about 'setup' or simply 'out' ?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints

[Matt Fleming]

On Tue, 11 Aug, at 02:16:29PM, Lee, Chun-Yi wrote:
> Add handler to parse the setup data that carrying hibernation key, it
> reserves hibernation key by memblock then copies key to a allocated page
> in later initcall stage.
> 
> And for erasing footprints, the codes in this patch remove setup
> data that carried hibernation key, and clean the memory space that
> reserved by memblock.
> 
> Reviewed-by: Jiri Kosina <jkosina@suse.com>
> Tested-by: Jiri Kosina <jkosina@suse.com>
> Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> ---
>  arch/x86/include/asm/suspend.h  |  4 +++
>  arch/x86/kernel/setup.c         | 21 ++++++++++-
>  arch/x86/power/Makefile         |  1 +
>  arch/x86/power/hibernate_keys.c | 78 +++++++++++++++++++++++++++++++++++++++++
>  kernel/power/power.h            |  5 +++
>  5 files changed, 108 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/power/hibernate_keys.c

[...]

> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 80f874b..b345359 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -112,6 +112,8 @@
>  #include <asm/alternative.h>
>  #include <asm/prom.h>
>  
> +#include <asm/suspend.h>
> +
>  /*
>   * max_low_pfn_mapped: highest direct mapped pfn under 4GB
>   * max_pfn_mapped:     highest direct mapped pfn over 4GB
> @@ -425,10 +427,22 @@ static void __init reserve_initrd(void)
>  }
>  #endif /* CONFIG_BLK_DEV_INITRD */
>  
> +static void __init remove_setup_data(u64 pa_prev, u64 pa_next)
> +{
> +	struct setup_data *data;
> +
> +	if (pa_prev) {
> +		data = early_memremap(pa_prev, sizeof(*data));
> +		data->next = pa_next;
> +		early_iounmap(data, sizeof(*data));

This should be early_memunmap for consistency().

> diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c
> new file mode 100644
> index 0000000..357dc0e
> --- /dev/null
> +++ b/arch/x86/power/hibernate_keys.c
> @@ -0,0 +1,78 @@
> +/* Hibernation keys handler
> + *
> + * Copyright (C) 2015 Lee, Chun-Yi <jlee@suse.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +#include <linux/bootmem.h>
> +#include <linux/memblock.h>
> +#include <linux/suspend.h>
> +#include <asm/suspend.h>
> +
> +/* physical address of hibernation keys from boot params */
> +static u64 keys_phys_addr;
> +
> +/* A page used to keep hibernation keys */
> +static struct hibernation_keys *hibernation_keys;
> +
> +void __init parse_hibernation_keys(u64 phys_addr, u32 data_len)
> +{
> +	struct setup_data *hibernation_setup_data;
> +
> +	/* Reserve keys memory, will copy and erase in init_hibernation_keys() */
> +	keys_phys_addr = phys_addr + sizeof(struct setup_data);
> +	memblock_reserve(keys_phys_addr, sizeof(struct hibernation_keys));
> +
> +	/* clear hibernation_data */
> +	hibernation_setup_data = early_memremap(phys_addr, data_len);
> +	if (!hibernation_setup_data)
> +		return;
> +
> +	memset(hibernation_setup_data, 0, sizeof(struct setup_data));

Why is this necessary? You're only clearing the struct setup_data
fields and you unlinked this setup_data entry in remove_setup_data()
anyway.

> +	early_memunmap(hibernation_setup_data, data_len);
> +}
> +
> +int get_hibernation_key(u8 **hkey)
> +{
> +	if (!hibernation_keys)
> +		return -ENODEV;
> +
> +	if (!hibernation_keys->hkey_status)
> +		*hkey = hibernation_keys->hibernation_key;
> +
> +	return hibernation_keys->hkey_status;
> +}

For global functions like this it's usually much preferred to prefix
the name with the subsystem, i.e. hibernation_get_key().

> +static int __init init_hibernation_keys(void)
> +{
> +	struct hibernation_keys *keys;
> +	int ret = 0;
> +
> +	if (!keys_phys_addr)
> +		return -ENODEV;
> +
> +	keys = early_memremap(keys_phys_addr, sizeof(struct hibernation_keys));
> +
> +	/* Copy hibernation keys to a allocated page */
> +	hibernation_keys = (struct hibernation_keys *)get_zeroed_page(GFP_KERNEL);
> +	if (hibernation_keys) {
> +		*hibernation_keys = *keys;
> +	} else {
> +		pr_err("PM: Allocate hibernation keys page failed\n");
> +		ret = -ENOMEM;
> +	}

It seems overkill to allocate an entire page for 28 bytes of data.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 04/16] x86/efi: Generating random number in EFI stub

[joeyli]

Hi Matt, 

Thanks for your reviewing and sorry for my delay.

On Thu, Aug 20, 2015 at 03:12:21PM +0100, Matt Fleming wrote:
> On Tue, 11 Aug, at 02:16:24PM, Lee, Chun-Yi wrote:
> > This patch adds the codes for generating random number array as the
> > HMAC key that will used by later EFI stub codes.
> > 
> > The original codes in efi_random copied from aslr and add the codes
> > to accept input entropy and EFI debugging. In later patch will add
> > the codes to get random number by EFI protocol. The separate codes
> > can avoid impacting aslr function.
>  
> Is there some way we can share the code between aslr and the EFI boot
> stub? People may not review both files when making changes and so bug
> fixes to one might not appear in the other.
> 
> -- 
> Matt Fleming, Intel Open Source Technology Center

The origin design is base on get_random_long() from aslr, so I direct
copy it to efi_random.c then modified it. 
There have some reasons I didn't share the code with aslr:

 - There have some debug_puttstr() for debugging in aslr, but those
   debugging log do not work in EFI stub, and I want put efi_print
   for debugging purpose. I don't want affect the code in aslr, so
   I choice copy get_random_long() to efi_random.c specific for EFI stub.

 - In subsequent patches add EFI random protocol support to
   get_random_longh().

In next version, I will try to extract shared code to misc.c to reuse them
between aslr and efi_random.


Thanks a lot!
Joey Lee

Re: [PATCH v2 05/16] x86/efi: Get entropy through EFI random number generator protocol

[joeyli]

On Thu, Aug 20, 2015 at 03:47:06PM +0100, Matt Fleming wrote:
> On Tue, 11 Aug, at 02:16:25PM, Lee, Chun-Yi wrote:
> > To grab random numbers through EFI protocol as one of the entropies
> > source of swsusp key, this patch adds the logic for accessing EFI RNG
> > (random number generator) protocol that's introduced since UEFI 2.4.
> > 
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > ---
> >  arch/x86/boot/compressed/efi_random.c | 209 ++++++++++++++++++++++++++++++++++
> >  include/linux/efi.h                   |  13 +++
> >  2 files changed, 222 insertions(+)
> 
> [...]
> 
> Most of this looks good.
> 
> > +static unsigned long efi_get_rng(efi_system_table_t *sys_table)
> > +{
> > +	const struct efi_config *efi_early = __efi_early();
> > +	unsigned long random = 0;
> > +	efi_status_t status;
> > +	void **rng_handle = NULL;
> > +
> > +	status = efi_locate_rng(sys_table, &rng_handle);
> > +	if (status != EFI_SUCCESS)
> > +		return 0;
> > +
> > +	if (efi_early->is64)
> > +		random = efi_get_rng64(sys_table, rng_handle);
> > +	else
> > +		random = efi_get_rng32(sys_table, rng_handle);
> > +
> > +	efi_call_early(free_pool, rng_handle);
> > +
> > +	return random;
> > +}
>   
> I think this function is named particularly poorly in the UEFI spec
> (GetRNG), can we maybe make this efi_get_rng_number(),
> efi_get_rng_value() or efi_get_rng_long() to match the existing
> naming?
>

Thanks for your suggestion, I will change the naming of functions.
 
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 85ef051..8914d60 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -427,6 +427,16 @@ typedef struct {
> >  #define EFI_PCI_IO_ATTRIBUTE_VGA_PALETTE_IO_16 0x20000
> >  #define EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 0x40000
> >  
> > +typedef struct {
> > +	u32 get_info;
> > +	u32 get_rng;
> > +} efi_rng_protocol_32;
> > +
> > +typedef struct {
> > +	u64 get_info;
> > +	u64 get_rng;
> > +} efi_rng_protocol_64;
> > +
> 
> We've been kinda slack with the naming conventions in efi.h but these
> should really both be 'efi_rng_protocol_*_t'.
> 

I will also change the name of struct here. 

> -- 
> Matt Fleming, Intel Open Source Technology Center


Thanks a lot!
Joey Lee

Re: [PATCH v2 05/16] x86/efi: Get entropy through EFI random number generator protocol

[joeyli]

On Thu, Aug 20, 2015 at 09:26:20PM +0100, Matt Fleming wrote:
> On Tue, 11 Aug, at 02:16:25PM, Lee, Chun-Yi wrote:
> > +
> > +static unsigned long efi_get_rng64(efi_system_table_t *sys_table,
> > +				   void **rng_handle)
> > +{
> > +	const struct efi_config *efi_early = __efi_early();
> > +	efi_rng_protocol_64 *rng = NULL;
> > +	efi_guid_t rng_proto = EFI_RNG_PROTOCOL_GUID;
> > +	u64 *handles = (u64 *)(unsigned long)rng_handle;
> > +	efi_status_t status;
> > +	unsigned long rng_number;
> > +
> > +	status = efi_call_early(handle_protocol, handles[0],
> > +				&rng_proto, (void **)&rng);
> > +	if (status != EFI_SUCCESS)
> > +		efi_printk(sys_table, "Failed to get EFI_RNG_PROTOCOL handles\n");
> > +
> > +	if (status == EFI_SUCCESS && rng) {
> > +		status = efi_early->call((unsigned long)rng->get_rng, rng, NULL,
> > +					sizeof(rng_number), &rng_number);
> 
> Actually, one thing just occurred to me - you're not passing an
> RNGAlgorithm value and are relying upon the firmware's default
> implementation.
> 
> I don't think that's a safe bet, the default could be anything and
> might vary across implementations.
> 

I didn't set specific RNGAlgorithm because different BIOS may
set different algorithm as default, it's also a kind of random situation
to provide uncertainty.

On the other hand, if the specific RNGAlgorithm doesn't support by BIOS
then EFI stub still need use BIOS's _default_ algorithm to get random
value.

> Can we do a little better here and pick a "preferred" algorithm
> instead of the default?
> 
> -- 
> Matt Fleming, Intel Open Source Technology Center

Per EDK2 implementation, EFI_RNG_ALGORITHM_SP800_90_CTR_256 is the default
algorithm that provided by driver, and EFI_RNG_ALGORITHM_RAW is the second
algorithm supported by EDK2. BIOS vendor need to write driver to support
others.

Maybe using EFI_RNG_ALGORITHM_SP800_90_CTR_256 as the default RNGAlgorithm
in efi_random can cover the most widely UEFI implementation, but when BIOS
do not support EFI_RNG_ALGORITHM_SP800_90_CTR_256 then kernel still need
use BIOS's _default_ setting.

I hope your suggestion.


Thanks a lot!
Joey Lee

Re: [PATCH v2 06/16] x86/efi: Generating random HMAC key for siging hibernate image

[joeyli]

On Thu, Aug 20, 2015 at 09:40:44PM +0100, Matt Fleming wrote:
> On Tue, 11 Aug, at 02:16:26PM, Lee, Chun-Yi wrote:
> > This patch adds codes in EFI stub for generating and storing the
> > HMAC key in EFI boot service variable for signing hibernate image.
> > 
> > Per rcf2104, the length of HMAC-SHA1 hash result is 20 bytes, and
> > it recommended the length of key the same with hash rsult, means
> > also 20 bytes. Using longer key would not significantly increase
> > the function strength. Due to the nvram space is limited in some
> > UEFI machines, so using the minimal recommended length 20 bytes
> > key that will stored in boot service variable.
> 
> I'm having a hard time understanding the middle part of this
> paragraph, specifically the part of the key and the hash result.
> There's a typo in the subject too s/siging/signing/and "Generating"
> should be "Generate".
>

In description, I want to explain why the length of key is 20 bytes.
I will try to explain more clear, if it still confuses reviewer then
I will remove the description.
 
> > The HMAC key stored in EFI boot service variable, GUID is
> > HIBERNATIONKey-fe141863-c070-478e-b8a3-878a5dc9ef21.
>  
> I'd really like to see some of the explanation from your cover letter
> included in the commit message for this patch, and in particular why
> signing hibernate images is a good thing.
> 
> Recording that for posterity in the commit message is going to be
> helpful when someone looks at this patch in 2 years time and wonders
> why RNG support was added to the EFI stub and why they might want to
> sign hibernate images.
> 

Thanks for suggestion, I will move some description from cover letter
to here and add comment for history and RNG support.

> > Reviewed-by: Jiri Kosina <jkosina@suse.com>
> > Tested-by: Jiri Kosina <jkosina@suse.com>
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > ---
> >  arch/x86/boot/compressed/eboot.c | 60 ++++++++++++++++++++++++++++++++++++++++
> >  arch/x86/include/asm/suspend.h   |  9 ++++++
> >  include/linux/suspend.h          |  3 ++
> >  3 files changed, 72 insertions(+)
> > 
> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index 0ffb6db..463aa9b 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -12,6 +12,7 @@
> >  #include <asm/efi.h>
> >  #include <asm/setup.h>
> >  #include <asm/desc.h>
> > +#include <asm/suspend.h>
> >  
> >  #include "../string.h"
> >  #include "eboot.h"
> > @@ -1383,6 +1384,63 @@ free_mem_map:
> >  	return status;
> >  }
> >  
> > +#ifdef CONFIG_HIBERNATE_VERIFICATION
> > +#define HIBERNATION_KEY \
> > +	((efi_char16_t [15]) { 'H', 'I', 'B', 'E', 'R', 'N', 'A', 'T', 'I', 'O', 'N', 'K', 'e', 'y', 0 })
> > +#define HIBERNATION_KEY_ATTRIBUTE	(EFI_VARIABLE_NON_VOLATILE | \
> > +					EFI_VARIABLE_BOOTSERVICE_ACCESS)
> > +
> > +static void setup_hibernation_keys(struct boot_params *params)
> > +{
> > +	unsigned long key_size;
> > +	unsigned long attributes;
> > +	struct hibernation_keys *keys;
> > +	efi_status_t status;
> > +
> > +	/* Allocate setup_data to carry keys */
> > +	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> > +				sizeof(struct hibernation_keys), &keys);
> > +	if (status != EFI_SUCCESS) {
> > +		efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n");
> > +		return;
> > +	}
> > +
> > +	memset(keys, 0, sizeof(struct hibernation_keys));
> > +
> > +	status = efi_call_early(get_variable, HIBERNATION_KEY,
> > +				&EFI_HIBERNATION_GUID, &attributes,
> > +				&key_size, keys->hibernation_key);
> 
> Tiny nit, but could you put a new line here please? This is a large
> chunk of code.
>

OK, I will add new line here.
 
> > +	if (status == EFI_SUCCESS && attributes != HIBERNATION_KEY_ATTRIBUTE) {
> > +		efi_printk(sys_table, "A hibernation key is not boot service variable\n");
> > +		memset(keys->hibernation_key, 0, HIBERNATION_DIGEST_SIZE);
> > +		status = efi_call_early(set_variable, HIBERNATION_KEY,
> > +					&EFI_HIBERNATION_GUID, attributes, 0,
> > +					NULL);
> > +		if (status == EFI_SUCCESS) {
> > +			efi_printk(sys_table, "Cleaned existing hibernation key\n");
> > +			status = EFI_NOT_FOUND;
> > +		}
> > +	}
> 
> Hmm.. it's not clear to me that we should be deleting this EFI
> variable if the attributes are bogus. It would be safer to just bail.
> 

The purpose of checking attribute of hibernation key variable is 
in case someone created a key variable on runtime environment _before_
this kernel create boot service variable. That causes EFI stub may load
a key that from non-secure environment.

That's why delete non-boot service variable and create new one here.

> > +
> > +	if (status != EFI_SUCCESS) {
> > +		efi_printk(sys_table, "Failed to get existing hibernation key\n");
> > +
> > +		efi_get_random_key(sys_table, params, keys->hibernation_key,
> > +				   HIBERNATION_DIGEST_SIZE);
> > +
> > +		status = efi_call_early(set_variable, HIBERNATION_KEY,
> > +					&EFI_HIBERNATION_GUID,
> > +					HIBERNATION_KEY_ATTRIBUTE,
> > +					HIBERNATION_DIGEST_SIZE,
> > +					keys->hibernation_key);
> > +		if (status != EFI_SUCCESS)
> > +			efi_printk(sys_table, "Failed to set hibernation key\n");
> 
> You're leaking 'keys' here.
>

Oh, you are right, I will free keys here.
 
> > diff --git a/arch/x86/include/asm/suspend.h b/arch/x86/include/asm/suspend.h
> > index 2fab6c2..ab463c4 100644
> > --- a/arch/x86/include/asm/suspend.h
> > +++ b/arch/x86/include/asm/suspend.h
> > @@ -3,3 +3,12 @@
> >  #else
> >  # include <asm/suspend_64.h>
> >  #endif
> > +
> > +#ifdef CONFIG_HIBERNATE_VERIFICATION
> > +#include <linux/suspend.h>
> > +
> > +struct hibernation_keys {
> > +	unsigned long hkey_status;
> > +	u8 hibernation_key[HIBERNATION_DIGEST_SIZE];
> > +};
> > +#endif
> 
> Have you given any thought to how things are going to work if we
> change the hash function in the future, or provide a choice? That
> information doesn't appear anywhere in the above struct.
> 

Do you mean the hash function of signing hibernation image changed, so the
hibernation key also need to re-generate for new length?

I will add a field in struct to forward the length of hibernation key variable.
In the future kernel can check the length to handle the change.

> -- 
> Matt Fleming, Intel Open Source Technology Center

Thanks a lot!
Joey Lee

Re: [PATCH v2 07/16] efi: Make efi_status_to_err() public

[joeyli]

On Thu, Aug 20, 2015 at 04:07:06PM +0100, Matt Fleming wrote:
> On Tue, 11 Aug, at 02:16:27PM, Lee, Chun-Yi wrote:
> > Moved the function of transferring EFI status to kernel error for
> > later used by EFI stub.
>  
> Might I suggest:
> 
>  "Move the function for converting EFI status to kernel error values
>   from drivers/firmware/efi/ to include/linux/efi.h for use by the x86
>   EFI stub in an upcoming patch."
> 
> ?
> 

Thanks for your suggestion, I will follow it.

> > Reviewed-by: Jiri Kosina <jkosina@suse.com>
> > Tested-by: Jiri Kosina <jkosina@suse.com>
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > ---
> >  drivers/firmware/efi/vars.c | 33 ---------------------------------
> >  include/linux/efi.h         | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 33 insertions(+), 33 deletions(-)
> 
> The patch content is fine,
> 
> Reviewed-by: Matt Fleming <matt.fleming@intel.com>
> 

Thanks

> -- 
> Matt Fleming, Intel Open Source Technology Center

Joey Lee

Re: [PATCH v2 08/16] x86/efi: Carrying hibernation key by setup data

[joeyli]

On Fri, Aug 21, 2015 at 01:40:26PM +0100, Matt Fleming wrote:
> On Tue, 11 Aug, at 02:16:28PM, Lee, Chun-Yi wrote:
> > For forwarding hibernation key from EFI stub to boot kernel, this patch
> > allocates setup data for carrying hibernation key, size and the status
> > of efi operating.
>  
> This could do with some more information, and include that the key is
> used to validate hibernate images.
> 
> But now that I think about it, is there a reason this patch hasn't
> been merged with patch 6? The memory leak I mentioned in patch 6
> becomes a non-issue in this one, so it would be good if these two
> could be squashed together.
>

OK, I will merge this patch with patch 6.

Actually the sequence of patches are from the order of my developing.
And, the purpose of code in this patch a bit different with patch 6,
so I didn't merge them together.
   
> > Reviewed-by: Jiri Kosina <jkosina@suse.com>
> > Tested-by: Jiri Kosina <jkosina@suse.com>
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > ---
> >  arch/x86/boot/compressed/eboot.c      | 26 +++++++++++++++++++++++---
> >  arch/x86/include/uapi/asm/bootparam.h |  1 +
> >  2 files changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index 463aa9b..c838d09 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -1394,18 +1394,22 @@ static void setup_hibernation_keys(struct boot_params *params)
> >  {
> >  	unsigned long key_size;
> >  	unsigned long attributes;
> > +	struct setup_data *setup_data, *hibernation_setup_data;
> >  	struct hibernation_keys *keys;
> > +	unsigned long size = 0;
> >  	efi_status_t status;
> 
> One thing to be aware of is that eboot.c has mainly used the
> "reverse-christmas-tree" style of variable declarations, with longer
> lines first, and shorter ones following. I haven't mentioned it before
> because none of your changes seemed to be too different (and it's not
> a tree-wide convention), but the above setup_data line goes a bit too
> far.
> 
> Could you try and keep them ordered, longest line first?
> 

Sure, sorry for I didn't aware that before.

> >  
> >  	/* Allocate setup_data to carry keys */
> > +	size = sizeof(struct setup_data) + sizeof(struct hibernation_keys);
> >  	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> > -				sizeof(struct hibernation_keys), &keys);
> > +				size, &hibernation_setup_data);
> >  	if (status != EFI_SUCCESS) {
> >  		efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n");
> >  		return;
> >  	}
> >  
> > -	memset(keys, 0, sizeof(struct hibernation_keys));
> > +	memset(hibernation_setup_data, 0, size);
> > +	keys = (struct hibernation_keys *) hibernation_setup_data->data;
> >  
> >  	status = efi_call_early(get_variable, HIBERNATION_KEY,
> >  				&EFI_HIBERNATION_GUID, &attributes,
> > @@ -1419,7 +1423,8 @@ static void setup_hibernation_keys(struct boot_params *params)
> >  		if (status == EFI_SUCCESS) {
> >  			efi_printk(sys_table, "Cleaned existing hibernation key\n");
> >  			status = EFI_NOT_FOUND;
> > -		}
> > +		} else
> > +			goto clean_fail;
> 
> Please add braces for the 'else' clause. Also, please include a
> comment stating that the reason you jump to the label instead of
> returning is so that the EFI status error code can be recorded in
> hibernation_setup_data.
>

Thanks for suggestions, I will modify it.
 
> >  	}
> >  
> >  	if (status != EFI_SUCCESS) {
> > @@ -1436,6 +1441,21 @@ static void setup_hibernation_keys(struct boot_params *params)
> >  		if (status != EFI_SUCCESS)
> >  			efi_printk(sys_table, "Failed to set hibernation key\n");
> >  	}
> > +
> > +clean_fail:
> > +	hibernation_setup_data->type = SETUP_HIBERNATION_KEYS;
> > +	hibernation_setup_data->len = sizeof(struct hibernation_keys);
> > +	hibernation_setup_data->next = 0;
> > +	keys->hkey_status = efi_status_to_err(status);
> > +
> > +	setup_data = (struct setup_data *)params->hdr.setup_data;
> > +	while (setup_data && setup_data->next)
> > +		setup_data = (struct setup_data *)setup_data->next;
> > +
> > +	if (setup_data)
> > +		setup_data->next = (unsigned long)hibernation_setup_data;
> > +	else
> > +		params->hdr.setup_data = (unsigned long)hibernation_setup_data;
> 
> This label name is a little confusing because you reach it both when
> the EFI boot variable was successfully created and when a bogus EFI
> variable failed to be deleted, i.e. it's not always reached because of
> a failure.
> 
> How about 'setup' or simply 'out' ?
>

I will change the label to 'setup' that match with setting setup_data.
 
> -- 
> Matt Fleming, Intel Open Source Technology Center


Thanks a lot!
Joey Lee

Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints

[joeyli]

On Fri, Aug 21, 2015 at 02:27:53PM +0100, Matt Fleming wrote:
> On Tue, 11 Aug, at 02:16:29PM, Lee, Chun-Yi wrote:
> > Add handler to parse the setup data that carrying hibernation key, it
> > reserves hibernation key by memblock then copies key to a allocated page
> > in later initcall stage.
> > 
> > And for erasing footprints, the codes in this patch remove setup
> > data that carried hibernation key, and clean the memory space that
> > reserved by memblock.
> > 
> > Reviewed-by: Jiri Kosina <jkosina@suse.com>
> > Tested-by: Jiri Kosina <jkosina@suse.com>
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > ---
> >  arch/x86/include/asm/suspend.h  |  4 +++
> >  arch/x86/kernel/setup.c         | 21 ++++++++++-
> >  arch/x86/power/Makefile         |  1 +
> >  arch/x86/power/hibernate_keys.c | 78 +++++++++++++++++++++++++++++++++++++++++
> >  kernel/power/power.h            |  5 +++
> >  5 files changed, 108 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/x86/power/hibernate_keys.c
> 
> [...]
> 
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index 80f874b..b345359 100644
> > --- a/arch/x86/kernel/setup.c
> > +++ b/arch/x86/kernel/setup.c
> > @@ -112,6 +112,8 @@
> >  #include <asm/alternative.h>
> >  #include <asm/prom.h>
> >  
> > +#include <asm/suspend.h>
> > +
> >  /*
> >   * max_low_pfn_mapped: highest direct mapped pfn under 4GB
> >   * max_pfn_mapped:     highest direct mapped pfn over 4GB
> > @@ -425,10 +427,22 @@ static void __init reserve_initrd(void)
> >  }
> >  #endif /* CONFIG_BLK_DEV_INITRD */
> >  
> > +static void __init remove_setup_data(u64 pa_prev, u64 pa_next)
> > +{
> > +	struct setup_data *data;
> > +
> > +	if (pa_prev) {
> > +		data = early_memremap(pa_prev, sizeof(*data));
> > +		data->next = pa_next;
> > +		early_iounmap(data, sizeof(*data));
> 
> This should be early_memunmap for consistency().
>

I will use early_memunmap, thanks!
 
> > diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c
> > new file mode 100644
> > index 0000000..357dc0e
> > --- /dev/null
> > +++ b/arch/x86/power/hibernate_keys.c
> > @@ -0,0 +1,78 @@
> > +/* Hibernation keys handler
> > + *
> > + * Copyright (C) 2015 Lee, Chun-Yi <jlee@suse.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public Licence
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the Licence, or (at your option) any later version.
> > + */
> > +
> > +#include <linux/bootmem.h>
> > +#include <linux/memblock.h>
> > +#include <linux/suspend.h>
> > +#include <asm/suspend.h>
> > +
> > +/* physical address of hibernation keys from boot params */
> > +static u64 keys_phys_addr;
> > +
> > +/* A page used to keep hibernation keys */
> > +static struct hibernation_keys *hibernation_keys;
> > +
> > +void __init parse_hibernation_keys(u64 phys_addr, u32 data_len)
> > +{
> > +	struct setup_data *hibernation_setup_data;
> > +
> > +	/* Reserve keys memory, will copy and erase in init_hibernation_keys() */
> > +	keys_phys_addr = phys_addr + sizeof(struct setup_data);
> > +	memblock_reserve(keys_phys_addr, sizeof(struct hibernation_keys));
> > +
> > +	/* clear hibernation_data */
> > +	hibernation_setup_data = early_memremap(phys_addr, data_len);
> > +	if (!hibernation_setup_data)
> > +		return;
> > +
> > +	memset(hibernation_setup_data, 0, sizeof(struct setup_data));
> 
> Why is this necessary? You're only clearing the struct setup_data
> fields and you unlinked this setup_data entry in remove_setup_data()
> anyway.
>

I want clean the setup_data fields that point to key data
before remove it from setup_data list.
 
> > +	early_memunmap(hibernation_setup_data, data_len);
> > +}
> > +
> > +int get_hibernation_key(u8 **hkey)
> > +{
> > +	if (!hibernation_keys)
> > +		return -ENODEV;
> > +
> > +	if (!hibernation_keys->hkey_status)
> > +		*hkey = hibernation_keys->hibernation_key;
> > +
> > +	return hibernation_keys->hkey_status;
> > +}
> 
> For global functions like this it's usually much preferred to prefix
> the name with the subsystem, i.e. hibernation_get_key().
>

I see, I will change the function name.
 
> > +static int __init init_hibernation_keys(void)
> > +{
> > +	struct hibernation_keys *keys;
> > +	int ret = 0;
> > +
> > +	if (!keys_phys_addr)
> > +		return -ENODEV;
> > +
> > +	keys = early_memremap(keys_phys_addr, sizeof(struct hibernation_keys));
> > +
> > +	/* Copy hibernation keys to a allocated page */
> > +	hibernation_keys = (struct hibernation_keys *)get_zeroed_page(GFP_KERNEL);
> > +	if (hibernation_keys) {
> > +		*hibernation_keys = *keys;
> > +	} else {
> > +		pr_err("PM: Allocate hibernation keys page failed\n");
> > +		ret = -ENOMEM;
> > +	}
> 
> It seems overkill to allocate an entire page for 28 bytes of data.
> 

Here to allocate an entire page because the basic unit is 'page' when
hibernation code checking saveable page that should included/excluded in
snapshot image.
Allocating an page to hibernation key can clearer to check the page should
excluded in snapshot image.

> -- 
> Matt Fleming, Intel Open Source Technology Center


Thanks a lot!
Joey Lee

Re: [PATCH v2 06/16] x86/efi: Generating random HMAC key for siging hibernate image

[Matt Fleming]

On Thu, 27 Aug, at 05:04:52PM, joeyli wrote:
> 
> The purpose of checking attribute of hibernation key variable is 
> in case someone created a key variable on runtime environment _before_
> this kernel create boot service variable. That causes EFI stub may load
> a key that from non-secure environment.
> 
> That's why delete non-boot service variable and create new one here.
 
I think bailing is more appropriate in that case, not deleting the
variable. The environment is not what we expected it to be, so we
shouldn't tamper with it.

But I don't feel super strongly about this point. I just wanted to
raise the question of whether it actually makes sense to delete the
variable that we obviously didn't create or whether it makes more
sense to refuse to verify hibernation images.

> > > diff --git a/arch/x86/include/asm/suspend.h b/arch/x86/include/asm/suspend.h
> > > index 2fab6c2..ab463c4 100644
> > > --- a/arch/x86/include/asm/suspend.h
> > > +++ b/arch/x86/include/asm/suspend.h
> > > @@ -3,3 +3,12 @@
> > >  #else
> > >  # include <asm/suspend_64.h>
> > >  #endif
> > > +
> > > +#ifdef CONFIG_HIBERNATE_VERIFICATION
> > > +#include <linux/suspend.h>
> > > +
> > > +struct hibernation_keys {
> > > +	unsigned long hkey_status;
> > > +	u8 hibernation_key[HIBERNATION_DIGEST_SIZE];
> > > +};
> > > +#endif
> > 
> > Have you given any thought to how things are going to work if we
> > change the hash function in the future, or provide a choice? That
> > information doesn't appear anywhere in the above struct.
> > 
> 
> Do you mean the hash function of signing hibernation image changed, so the
> hibernation key also need to re-generate for new length?
 
Yeah, that kind of thing.

> I will add a field in struct to forward the length of hibernation key variable.
> In the future kernel can check the length to handle the change.

Would it also make sense to explicitly record the hash function used
as well as the length?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints

[Matt Fleming]

On Thu, 27 Aug, at 06:21:44PM, joeyli wrote:
> On Fri, Aug 21, 2015 at 02:27:53PM +0100, Matt Fleming wrote:
> > On Tue, 11 Aug, at 02:16:29PM, Lee, Chun-Yi wrote:
> > > +static int __init init_hibernation_keys(void)
> > > +{
> > > +	struct hibernation_keys *keys;
> > > +	int ret = 0;
> > > +
> > > +	if (!keys_phys_addr)
> > > +		return -ENODEV;
> > > +
> > > +	keys = early_memremap(keys_phys_addr, sizeof(struct hibernation_keys));
> > > +
> > > +	/* Copy hibernation keys to a allocated page */
> > > +	hibernation_keys = (struct hibernation_keys *)get_zeroed_page(GFP_KERNEL);
> > > +	if (hibernation_keys) {
> > > +		*hibernation_keys = *keys;
> > > +	} else {
> > > +		pr_err("PM: Allocate hibernation keys page failed\n");
> > > +		ret = -ENOMEM;
> > > +	}
> > 
> > It seems overkill to allocate an entire page for 28 bytes of data.
> > 
> 
> Here to allocate an entire page because the basic unit is 'page' when
> hibernation code checking saveable page that should included/excluded in
> snapshot image.
> Allocating an page to hibernation key can clearer to check the page should
> excluded in snapshot image.

Do other pieces of code do something similar, i.e. allocate an entire
page because the hibernation code deals with pages? If so, it might be
possible to lump all these pieces of data together into a single
'auxiliary' page.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 06/16] x86/efi: Generating random HMAC key for siging hibernate image

[joeyli]

On Wed, Sep 09, 2015 at 01:15:45PM +0100, Matt Fleming wrote:
> On Thu, 27 Aug, at 05:04:52PM, joeyli wrote:
> > 
> > The purpose of checking attribute of hibernation key variable is 
> > in case someone created a key variable on runtime environment _before_
> > this kernel create boot service variable. That causes EFI stub may load
> > a key that from non-secure environment.
> > 
> > That's why delete non-boot service variable and create new one here.
>  
> I think bailing is more appropriate in that case, not deleting the
> variable. The environment is not what we expected it to be, so we
> shouldn't tamper with it.
> 
> But I don't feel super strongly about this point. I just wanted to
> raise the question of whether it actually makes sense to delete the
> variable that we obviously didn't create or whether it makes more
> sense to refuse to verify hibernation images.
>

Thanks for you point out. But, I want keep this logic to avoid someone
create a runtime variable with the same name-SSID to confuse the key
generator in EFI stub.
 
> > > > diff --git a/arch/x86/include/asm/suspend.h b/arch/x86/include/asm/suspend.h
> > > > index 2fab6c2..ab463c4 100644
> > > > --- a/arch/x86/include/asm/suspend.h
> > > > +++ b/arch/x86/include/asm/suspend.h
> > > > @@ -3,3 +3,12 @@
> > > >  #else
> > > >  # include <asm/suspend_64.h>
> > > >  #endif
> > > > +
> > > > +#ifdef CONFIG_HIBERNATE_VERIFICATION
> > > > +#include <linux/suspend.h>
> > > > +
> > > > +struct hibernation_keys {
> > > > +	unsigned long hkey_status;
> > > > +	u8 hibernation_key[HIBERNATION_DIGEST_SIZE];
> > > > +};
> > > > +#endif
> > > 
> > > Have you given any thought to how things are going to work if we
> > > change the hash function in the future, or provide a choice? That
> > > information doesn't appear anywhere in the above struct.
> > > 
> > 
> > Do you mean the hash function of signing hibernation image changed, so the
> > hibernation key also need to re-generate for new length?
>  
> Yeah, that kind of thing.
> 
> > I will add a field in struct to forward the length of hibernation key variable.
> > In the future kernel can check the length to handle the change.
> 
> Would it also make sense to explicitly record the hash function used
> as well as the length?
> 
> -- 
> Matt Fleming, Intel Open Source Technology Center

The job of key generator in EFI stub is to generate appropriate length of key
for usage by later activities. It doesn't need to know how does kernel use
which kind of hash algorithm, so I think do not need record the hash function.

But, kernel should knows the exactly length of old key to decide if it needs to
use flag to trigger key regeneration process in EFI stub to create new key for
new length.

Only the other hand, the hash algorithm of hibernation was setted in kernel
compiler time even user can change  it, so I think to forward the key length
from querying efi variable is enough, don't need write the hash algorithm to
EFI boot variable.


Thanks a lot!
Joey Lee

Re: [PATCH v2 09/16] PM / hibernate: Reserve hibernation key and erase footprints

[joeyli]

On Wed, Sep 09, 2015 at 01:24:08PM +0100, Matt Fleming wrote:
> On Thu, 27 Aug, at 06:21:44PM, joeyli wrote:
> > On Fri, Aug 21, 2015 at 02:27:53PM +0100, Matt Fleming wrote:
> > > On Tue, 11 Aug, at 02:16:29PM, Lee, Chun-Yi wrote:
> > > > +static int __init init_hibernation_keys(void)
> > > > +{
> > > > +	struct hibernation_keys *keys;
> > > > +	int ret = 0;
> > > > +
> > > > +	if (!keys_phys_addr)
> > > > +		return -ENODEV;
> > > > +
> > > > +	keys = early_memremap(keys_phys_addr, sizeof(struct hibernation_keys));
> > > > +
> > > > +	/* Copy hibernation keys to a allocated page */
> > > > +	hibernation_keys = (struct hibernation_keys *)get_zeroed_page(GFP_KERNEL);
> > > > +	if (hibernation_keys) {
> > > > +		*hibernation_keys = *keys;
> > > > +	} else {
> > > > +		pr_err("PM: Allocate hibernation keys page failed\n");
> > > > +		ret = -ENOMEM;
> > > > +	}
> > > 
> > > It seems overkill to allocate an entire page for 28 bytes of data.
> > > 
> > 
> > Here to allocate an entire page because the basic unit is 'page' when
> > hibernation code checking saveable page that should included/excluded in
> > snapshot image.
> > Allocating an page to hibernation key can clearer to check the page should
> > excluded in snapshot image.
> 
> Do other pieces of code do something similar, i.e. allocate an entire
> page because the hibernation code deals with pages? If so, it might be
> possible to lump all these pieces of data together into a single
> 'auxiliary' page.
> 
> -- 
> Matt Fleming, Intel Open Source Technology Center

Thanks for your suggestions, I will lump all these data to into a single page
with the key. I want keep all the contents in the page only for hibernation's
usage but not to share with other subsystem in kernel, that's more safe to avoid
the key be accessed by with other code.


Thanks a lot!
Joey Lee

[PATCH v2 00/16] Signature verification of hibernate snapshot

[Lee, Chun-Yi]

Hi experts,

This patchset is the implementation of signature verification of hibernate
snapshot image. The origin idea is from Jiri Kosina: Let EFI bootloader
generate key-pair in UEFI secure boot environment, then forward it to kernel
for sign/verify hibernate image.

The first patchset for this function was sent in Sep. 2013, the implementation
is base on PKI. This new patchset is base on HMAC-SHA1.

The hibernate function provided by kernel was used to snapshot memory
to be a image for keeping in storage, then restored in appropriate time.
There have potential threat from hacking the memory snapshot image.
Cracker may triggers hibernating process through ioctl to grab snapshot
image, then restoring modified image back to memory. Another situation
is booting to other hacked OS to modify the snapshot image in swap
partition or file, then user may runs malware after image restored to
memory. In addition, the above weakness cause kernel is not fully trusted
in EFI secure boot environment.

So, kernel hibernate function needs a mechanism to verify integrity of
hibernate snapshot image.

For signing hibernate image, kernel need a key for generating signature of
image. The origin idea is using PKI, the EFI bootloader, shim generates key
pair and forward to boot kernel for signing/verifying image. In Linux Plumbers
Conference 2013, we got response from community experts for just using
symmetric key algorithm to generate signature, that's simpler and no EFI
bootloader's involving.

Current solution is using HMAC-SHA1 algorithm, it generating HMAC key in EFI
stub, the HMAC key stored in efi boot service variable, When hibernate
recovering, kernel will verify the image signature before switch whole system
to image kernel and image memory space. When verifying failed, kernel is
tainted or stop recovering and discarding image.

Set HIBERNATE_VERIFICATION compile option to true for enabling hibernate
verification. The default behavior of verifying failed is accept restoring
image but tainting kernel with H taint flag. Using HIBERNATE_VERIFICATION_FORCE
kernel compile option or "sigenforce" kernel parameter to force hibernate
recovery process stop when verification failed. It allows user to trigger the
key re-generating process in EFI stub through SNAPSHOT_REGENERATE_KEY ioctl.

v2:
 - Replaced all SWSUSP naming with HIBERNATION.
 - Moved swsusp_info structure definition only for CONFIG_HIBERNATION.
 - Fixed typo in patch subject and description.
 - Changed name of i8254() to read_i8254()
 - Removed all efi_printk log in efi_random.c
 - Changed the size of key array to be unsigned in efi_random.c
 - Avoid calling cpuid many times.
 - Add line breaks to error log in efi_random.c
 - Removed free_handle label in efi_random.c
 - Moved efi_status_to_str() to efi_random.c, and reduce code duplication.
 - Set rng_handle = NULL in efi_locate_rng()
 - Changed u32 random to bool in efi_rng_supported()
 - Using EFI status codes explicitly.
 - Modified Copyright declaration.
 - Moved set_hibernation_key_regen_flag to user.c


Lee, Chun-Yi (16):
  PM / hibernate: define HMAC algorithm and digest size of hibernation
  x86/efi: Add get and set variable to EFI services pointer table
  x86/boot: Public getting random boot function
  x86/efi: Generating random number in EFI stub
  x86/efi: Get entropy through EFI random number generator protocol
  x86/efi: Generating random HMAC key for siging hibernate image
  efi: Make efi_status_to_err() public
  x86/efi: Carrying hibernation key by setup data
  PM / hibernate: Reserve hibernation key and erase footprints
  PM / hibernate: Generate and verify signature of hibernate snapshot
  PM / hibernate: Avoid including hibernation key to hibernate image
  PM / hibernate: Forward signature verifying result and key to image
    kernel
  PM / hibernate: Add configuration to enforce signature verification
  PM / hibernate: Allow user trigger hibernation key re-generating
  PM / hibernate: Bypass verification logic on legacy BIOS
  PM / hibernate: Document signature verification of hibernate snapshot

 Documentation/kernel-parameters.txt             |   5 +
 Documentation/power/swsusp-signature-verify.txt |  86 +++++++
 arch/x86/boot/compressed/Makefile               |   1 +
 arch/x86/boot/compressed/aslr.c                 |  57 +----
 arch/x86/boot/compressed/eboot.c                |  97 ++++++++
 arch/x86/boot/compressed/efi_random.c           | 289 +++++++++++++++++++++++
 arch/x86/boot/compressed/head_32.S              |   6 +-
 arch/x86/boot/compressed/head_64.S              |   8 +-
 arch/x86/boot/compressed/misc.c                 |  55 +++++
 arch/x86/boot/compressed/misc.h                 |   4 +
 arch/x86/include/asm/efi.h                      |   2 +
 arch/x86/include/asm/suspend.h                  |  13 ++
 arch/x86/include/uapi/asm/bootparam.h           |   1 +
 arch/x86/kernel/setup.c                         |  21 +-
 arch/x86/power/Makefile                         |   1 +
 arch/x86/power/hibernate_keys.c                 | 173 ++++++++++++++
 drivers/firmware/Makefile                       |   1 +
 drivers/firmware/efi/Kconfig                    |   4 +
 drivers/firmware/efi/Makefile                   |   1 +
 drivers/firmware/efi/efi-hibernate_keys.c       |  42 ++++
 drivers/firmware/efi/vars.c                     |  33 ---
 include/linux/efi.h                             |  46 ++++
 include/linux/kernel.h                          |   1 +
 include/linux/suspend.h                         |  27 +++
 include/uapi/linux/suspend_ioctls.h             |   3 +-
 kernel/panic.c                                  |   2 +
 kernel/power/Kconfig                            |  23 ++
 kernel/power/hibernate.c                        |  10 +
 kernel/power/power.h                            |  22 +-
 kernel/power/snapshot.c                         | 293 ++++++++++++++++++++++--
 kernel/power/swap.c                             |   4 +
 kernel/power/user.c                             |  19 ++
 kernel/reboot.c                                 |   3 +
 33 files changed, 1240 insertions(+), 113 deletions(-)
 create mode 100644 Documentation/power/swsusp-signature-verify.txt
 create mode 100644 arch/x86/boot/compressed/efi_random.c
 create mode 100644 arch/x86/power/hibernate_keys.c
 create mode 100644 drivers/firmware/efi/efi-hibernate_keys.c

-- 
2.1.4