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.
Lee, Chun-Yi (16):
PM / hibernate: define HMAC algorithm and digest size of swsusp
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: Public the function of transferring EFI status to kernel error
x86/efi: Carrying swsusp key by setup data
PM / hibernate: Reserve swsusp key and earse footprints
PM / hibernate: Generate and verify signature of hibernate snapshot
PM / hibernate: Avoid including swsusp 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 swsusp 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 | 55 +----
arch/x86/boot/compressed/eboot.c | 94 ++++++++
arch/x86/boot/compressed/efi_random.c | 281 +++++++++++++++++++++++
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 | 46 ++++
drivers/firmware/efi/vars.c | 33 ---
include/linux/efi.h | 79 +++++++
include/linux/kernel.h | 1 +
include/linux/suspend.h | 26 +++
include/uapi/linux/suspend_ioctls.h | 3 +-
kernel/panic.c | 2 +
kernel/power/Kconfig | 23 ++
kernel/power/hibernate.c | 10 +
kernel/power/power.h | 20 ++
kernel/power/snapshot.c | 293 ++++++++++++++++++++++--
kernel/power/swap.c | 4 +
kernel/power/user.c | 16 ++
kernel/reboot.c | 3 +
33 files changed, 1260 insertions(+), 111 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
--
1.8.4.5
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, add HIBERNATE_VERIFICATION kernel config.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
include/linux/suspend.h | 5 +++++
kernel/power/Kconfig | 13 +++++++++++++
kernel/power/power.h | 1 +
3 files changed, 19 insertions(+)
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 5efe743..6cd2a48 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 SWSUSP_HMAC "hmac(sha1)"
+#define SWSUSP_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..f65fcf7 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -12,6 +12,7 @@ struct swsusp_info {
unsigned long image_pages;
unsigned long pages;
unsigned long size;
+ u8 signature[SWSUSP_DIGEST_SIZE];
} __aligned(PAGE_SIZE);
#ifdef CONFIG_HIBERNATION
--
1.8.4.5
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 veriable.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
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;
--
1.8.4.5
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.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
arch/x86/boot/compressed/aslr.c | 55 +----------------------------------------
arch/x86/boot/compressed/misc.c | 55 +++++++++++++++++++++++++++++++++++++++++
arch/x86/boot/compressed/misc.h | 4 +++
3 files changed, 60 insertions(+), 54 deletions(-)
diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
index d7b1f65..bd6550a 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");
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index a107b93..d929506 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 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..e10908c 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 i8254(void);
+extern unsigned long get_random_boot(struct boot_params *boot_params);
+#endif
#if CONFIG_RANDOMIZE_BASE
/* aslr.c */
--
1.8.4.5
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.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
arch/x86/boot/compressed/Makefile | 1 +
arch/x86/boot/compressed/efi_random.c | 88 +++++++++++++++++++++++++++++++++++
arch/x86/boot/compressed/misc.c | 4 +-
arch/x86/boot/compressed/misc.h | 2 +-
4 files changed, 92 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..bdb2d46
--- /dev/null
+++ b/arch/x86/boot/compressed/efi_random.c
@@ -0,0 +1,88 @@
+#include "misc.h"
+
+#include <linux/efi.h>
+#include <asm/archrandom.h>
+
+#define X86_FEATURE_EDX_TSC (1 << 4)
+#define X86_FEATURE_ECX_RDRAND (1 << 30)
+
+static bool rdrand_feature(void)
+{
+ return (cpuid_ecx(0x1) & X86_FEATURE_ECX_RDRAND);
+}
+
+static bool rdtsc_feature(void)
+{
+ return (cpuid_edx(0x1) & X86_FEATURE_EDX_TSC);
+}
+
+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;
+
+ efi_printk(sys_table, " EFI random");
+
+ if (entropy)
+ random = entropy;
+ else
+ random = get_random_boot(boot_params);
+
+ if (rdrand_feature()) {
+ efi_printk(sys_table, " RDRAND");
+ if (rdrand_long(&raw)) {
+ random ^= raw;
+ use_i8254 = false;
+ }
+ }
+
+ if (rdtsc_feature()) {
+ efi_printk(sys_table, " RDTSC");
+ rdtscll(raw);
+
+ random ^= raw;
+ use_i8254 = false;
+ }
+
+ if (use_i8254) {
+ efi_printk(sys_table, " i8254");
+ random ^= i8254();
+ }
+
+ /* Circular multiply for better bit diffusion */
+ asm("mul %3"
+ : "=a" (random), "=d" (raw)
+ : "a" (random), "rm" (mix_const));
+ random += raw;
+
+ efi_printk(sys_table, "...\n");
+
+ return random;
+}
+
+void efi_get_random_key(efi_system_table_t *sys_table,
+ struct boot_params *params, u8 key[], int size)
+{
+ unsigned long entropy = 0;
+ int i, bfill = size;
+
+ if (key == NULL || !size)
+ return;
+
+ memset(key, 0, size);
+ while (bfill > 0) {
+ entropy = get_random_long(entropy, params, sys_table);
+ if (bfill >= sizeof(entropy))
+ memcpy((void *)(key + size - bfill), &entropy, sizeof(entropy));
+ else
+ memcpy((void *)(key + size - bfill), &entropy, bfill);
+ bfill -= sizeof(entropy);
+ }
+}
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index d929506..2c3c997 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 e10908c..4be2780 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 i8254(void);
extern unsigned long get_random_boot(struct boot_params *boot_params);
#endif
--
1.8.4.5
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 <[email protected]>
---
arch/x86/boot/compressed/efi_random.c | 193 ++++++++++++++++++++++++++++++++++
include/linux/efi.h | 46 ++++++++
2 files changed, 239 insertions(+)
diff --git a/arch/x86/boot/compressed/efi_random.c b/arch/x86/boot/compressed/efi_random.c
index bdb2d46..1f5c63d 100644
--- a/arch/x86/boot/compressed/efi_random.c
+++ b/arch/x86/boot/compressed/efi_random.c
@@ -2,6 +2,191 @@
#include <linux/efi.h>
#include <asm/archrandom.h>
+#include <asm/efi.h>
+
+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;
+
+ 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");
+ 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");
+ goto free_handle;
+ }
+
+ return EFI_SUCCESS;
+
+free_handle:
+ 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");
+
+ 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");
+
+ 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();
+ u32 random = 0;
+ 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)
+ random = efi_rng_supported64(sys_table, rng_handle);
+ else
+ random = efi_rng_supported32(sys_table, rng_handle);
+
+ efi_call_early(free_pool, rng_handle);
+
+ return random;
+
+}
+
+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");
+
+ 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));
+ }
+ }
+
+ 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");
+
+ 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));
+ }
+ }
+
+ 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 X86_FEATURE_EDX_TSC (1 << 4)
#define X86_FEATURE_ECX_RDRAND (1 << 30)
@@ -51,6 +236,14 @@ static unsigned long get_random_long(unsigned long entropy,
use_i8254 = false;
}
+ if (efi_rng_supported(sys_table)) {
+ efi_printk(sys_table, " EFI_RNG");
+ raw = efi_get_rng(sys_table);
+ if (raw)
+ random ^= raw;
+ use_i8254 = false;
+ }
+
if (use_i8254) {
efi_printk(sys_table, " i8254");
random ^= i8254();
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 85ef051..a628f83 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;
@@ -861,6 +874,39 @@ efi_guid_to_str(efi_guid_t *guid, char *out)
return out;
}
+static inline char *efi_status_to_str(efi_status_t status)
+{
+ char *str;
+
+ switch (status) {
+ case EFI_SUCCESS:
+ str = "EFI_SUCCESS";
+ break;
+ case EFI_INVALID_PARAMETER:
+ str = "EFI_INVALID_PARAMETER";
+ break;
+ case EFI_OUT_OF_RESOURCES:
+ str = "EFI_OUT_OF_RESOURCES";
+ break;
+ case EFI_DEVICE_ERROR:
+ str = "EFI_DEVICE_ERROR";
+ break;
+ case EFI_WRITE_PROTECTED:
+ str = "EFI_WRITE_PROTECTED";
+ break;
+ case EFI_SECURITY_VIOLATION:
+ str = "EFI_SECURITY_VIOLATION";
+ break;
+ case EFI_NOT_FOUND:
+ str = "EFI_NOT_FOUND";
+ break;
+ default:
+ str = "";
+ }
+
+ return str;
+}
+
extern void efi_init (void);
extern void *efi_get_pal_addr (void);
extern void efi_map_pal_code (void);
--
1.8.4.5
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.
For generating a messy number as a 20 bytes key, the codes in EFI
stub gets u32 random number five time and every random number is
rolling that last u32 random number as entropy.
The HMAC key stored in EFI boot service variable, the GUID is
S4SignKey-fe141863-c070-478e-b8a3-878a5dc9ef21.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
arch/x86/boot/compressed/eboot.c | 57 ++++++++++++++++++++++++++++++++++++++++
arch/x86/include/asm/suspend.h | 9 +++++++
include/linux/suspend.h | 3 +++
3 files changed, 69 insertions(+)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 0ffb6db..97b356f 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,60 @@ free_mem_map:
return status;
}
+#ifdef CONFIG_HIBERNATE_VERIFICATION
+#define SWSUSP_KEY \
+ ((efi_char16_t [10]) { 'S', 'W', 'S', 'U', 'S', 'P', 'K', 'e', 'y', 0 })
+#define SWSUSP_KEY_ATTRIBUTE (EFI_VARIABLE_NON_VOLATILE | \
+ EFI_VARIABLE_BOOTSERVICE_ACCESS)
+
+static void setup_swsusp_keys(struct boot_params *params)
+{
+ unsigned long key_size, attributes;
+ struct swsusp_keys *swsusp_keys;
+ efi_status_t status;
+
+ /* Allocate setup_data to carry keys */
+ status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+ sizeof(struct swsusp_keys), &swsusp_keys);
+ if (status != EFI_SUCCESS) {
+ efi_printk(sys_table, "Failed to alloc mem for swsusp_keys\n");
+ return;
+ }
+
+ memset(swsusp_keys, 0, sizeof(struct swsusp_keys));
+
+ status = efi_call_early(get_variable, SWSUSP_KEY, &EFI_SWSUSP_GUID,
+ &attributes, &key_size, swsusp_keys->swsusp_key);
+ if (status == EFI_SUCCESS && attributes != SWSUSP_KEY_ATTRIBUTE) {
+ efi_printk(sys_table, "A swsusp key is not boot service variable\n");
+ memset(swsusp_keys->swsusp_key, 0, SWSUSP_DIGEST_SIZE);
+ status = efi_call_early(set_variable, SWSUSP_KEY,
+ &EFI_SWSUSP_GUID, attributes, 0, NULL);
+ if (status == EFI_SUCCESS) {
+ efi_printk(sys_table, "Cleaned existing swsusp key\n");
+ status = EFI_NOT_FOUND;
+ }
+ }
+
+ if (status != EFI_SUCCESS) {
+ efi_printk(sys_table, "Failed to get existing swsusp key\n");
+
+ efi_get_random_key(sys_table, params, swsusp_keys->swsusp_key,
+ SWSUSP_DIGEST_SIZE);
+
+ status = efi_call_early(set_variable, SWSUSP_KEY,
+ &EFI_SWSUSP_GUID,
+ SWSUSP_KEY_ATTRIBUTE,
+ SWSUSP_DIGEST_SIZE,
+ swsusp_keys->swsusp_key);
+ if (status != EFI_SUCCESS)
+ efi_printk(sys_table, "Failed to set swsusp key\n");
+ }
+}
+#else
+static void setup_swsusp_keys(struct boot_params *params) {}
+#endif
+
/*
* On success we return a pointer to a boot_params structure, and NULL
* on failure.
@@ -1420,6 +1475,8 @@ struct boot_params *efi_main(struct efi_config *c,
setup_efi_pci(boot_params);
+ setup_swsusp_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..b0c3f68 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 swsusp_keys {
+ unsigned long skey_status;
+ u8 swsusp_key[SWSUSP_DIGEST_SIZE];
+};
+#endif
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 6cd2a48..1ec7d11 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -328,6 +328,9 @@ struct platform_hibernation_ops {
#ifdef CONFIG_HIBERNATION
+#define EFI_SWSUSP_GUID \
+ EFI_GUID(0xfe141863, 0xc070, 0x478e, 0xb8, 0xa3, 0x87, 0x8a, 0x5d, 0xc9, 0xef, 0x21)
+
/* HMAC Algorithm of Hibernate Signature */
#define SWSUSP_HMAC "hmac(sha1)"
#define SWSUSP_DIGEST_SIZE 20
--
1.8.4.5
Moved the function of transferring EFI status to kernel error for
later used by EFI stub.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
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 a628f83..9239f32 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -907,6 +907,39 @@ static inline char *efi_status_to_str(efi_status_t status)
return str;
}
+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);
--
1.8.4.5
For forwarding swsusp key from EFI stub to boot kernel, this patch
allocates setup data for carrying swsusp key, size and the status
of efi operating.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
arch/x86/boot/compressed/eboot.c | 29 +++++++++++++++++++++++++----
arch/x86/include/uapi/asm/bootparam.h | 1 +
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 97b356f..5e1476e 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1392,19 +1392,23 @@ free_mem_map:
static void setup_swsusp_keys(struct boot_params *params)
{
- unsigned long key_size, attributes;
+ struct setup_data *setup_data, *swsusp_setup_data;
struct swsusp_keys *swsusp_keys;
+ int size = 0;
+ unsigned long key_size, attributes;
efi_status_t status;
/* Allocate setup_data to carry keys */
+ size = sizeof(struct setup_data) + sizeof(struct swsusp_keys);
status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
- sizeof(struct swsusp_keys), &swsusp_keys);
+ size, &swsusp_setup_data);
if (status != EFI_SUCCESS) {
efi_printk(sys_table, "Failed to alloc mem for swsusp_keys\n");
return;
}
- memset(swsusp_keys, 0, sizeof(struct swsusp_keys));
+ memset(swsusp_setup_data, 0, size);
+ swsusp_keys = (struct swsusp_keys *)swsusp_setup_data->data;
status = efi_call_early(get_variable, SWSUSP_KEY, &EFI_SWSUSP_GUID,
&attributes, &key_size, swsusp_keys->swsusp_key);
@@ -1413,7 +1417,9 @@ static void setup_swsusp_keys(struct boot_params *params)
memset(swsusp_keys->swsusp_key, 0, SWSUSP_DIGEST_SIZE);
status = efi_call_early(set_variable, SWSUSP_KEY,
&EFI_SWSUSP_GUID, attributes, 0, NULL);
- if (status == EFI_SUCCESS) {
+ if (status)
+ goto clean_fail;
+ else {
efi_printk(sys_table, "Cleaned existing swsusp key\n");
status = EFI_NOT_FOUND;
}
@@ -1433,6 +1439,21 @@ static void setup_swsusp_keys(struct boot_params *params)
if (status != EFI_SUCCESS)
efi_printk(sys_table, "Failed to set swsusp key\n");
}
+
+clean_fail:
+ swsusp_setup_data->type = SETUP_SWSUSP_KEYS;
+ swsusp_setup_data->len = sizeof(struct swsusp_keys);
+ swsusp_setup_data->next = 0;
+ swsusp_keys->skey_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)swsusp_setup_data;
+ else
+ params->hdr.setup_data = (unsigned long)swsusp_setup_data;
}
#else
static void setup_swsusp_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..12e0a203 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_SWSUSP_KEYS 5
/* ram_size flags */
#define RAMDISK_IMAGE_START_MASK 0x07FF
--
1.8.4.5
Add handler to parse the setup data that carrying swsusp key, it
reserves swsusp key by memblock then copies key to a allocated page
in later initcall stage.
And for earsing footbprints, the codes in this patch remove setup
data that carried swsusp key, and clean the memory space that
reserved by memblock.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
arch/x86/include/asm/suspend.h | 4 +++
arch/x86/kernel/setup.c | 21 ++++++++++-
arch/x86/power/Makefile | 1 +
arch/x86/power/hibernate_keys.c | 79 +++++++++++++++++++++++++++++++++++++++++
kernel/power/power.h | 5 +++
5 files changed, 109 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 b0c3f68..bec87e3 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_swsusp_keys(u64 phys_addr, u32 data_len);
+
struct swsusp_keys {
unsigned long skey_status;
u8 swsusp_key[SWSUSP_DIGEST_SIZE];
};
+#else
+static inline void parse_swsusp_keys(u64 phys_addr, u32 data_len) {}
#endif
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 80f874b..5412be0 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_SWSUSP_KEYS:
+ parse_swsusp_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..4a68b86
--- /dev/null
+++ b/arch/x86/power/hibernate_keys.c
@@ -0,0 +1,79 @@
+/* Swsusp keys handler
+ *
+ * Copyright (C) 2015 SUSE Linux Products GmbH. All rights reserved.
+ * Written by Chun-Yi Lee ([email protected])
+ *
+ * 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 swsusp keys from boot params */
+static u64 keys_phys_addr;
+
+/* A page used to keep swsusp keys */
+static struct swsusp_keys *swsusp_keys;
+
+void __init parse_swsusp_keys(u64 phys_addr, u32 data_len)
+{
+ struct setup_data *swsusp_setup_data;
+
+ /* Reserve keys memory, will copy and earse in init_hibernate_keys() */
+ keys_phys_addr = phys_addr + sizeof(struct setup_data);
+ memblock_reserve(keys_phys_addr, sizeof(struct swsusp_keys));
+
+ /* clear setup_data */
+ swsusp_setup_data = early_memremap(phys_addr, data_len);
+ if (!swsusp_setup_data)
+ return;
+
+ memset(swsusp_setup_data, 0, sizeof(struct setup_data));
+ early_memunmap(swsusp_setup_data, data_len);
+}
+
+int get_swsusp_key(u8 **skey)
+{
+ if (!swsusp_keys)
+ return -ENODEV;
+
+ if (!swsusp_keys->skey_status)
+ *skey = swsusp_keys->swsusp_key;
+
+ return swsusp_keys->skey_status;
+}
+
+static int __init init_hibernate_keys(void)
+{
+ struct swsusp_keys *keys;
+ int ret = 0;
+
+ if (!keys_phys_addr)
+ return -ENODEV;
+
+ keys = early_memremap(keys_phys_addr, sizeof(struct swsusp_keys));
+
+ /* Copy swsusp keys to a allocated page */
+ swsusp_keys = (struct swsusp_keys *)get_zeroed_page(GFP_KERNEL);
+ if (swsusp_keys) {
+ *swsusp_keys = *keys;
+ } else {
+ pr_err("PM: Allocate swsusp keys page failed\n");
+ ret = -ENOMEM;
+ }
+
+ /* Earse keys data no matter copy success or failed */
+ memset(keys, 0, sizeof(struct swsusp_keys));
+ early_memunmap(keys, sizeof(struct swsusp_keys));
+ memblock_free(keys_phys_addr, sizeof(struct swsusp_keys));
+ keys_phys_addr = 0;
+
+ return ret;
+}
+
+late_initcall(init_hibernate_keys);
diff --git a/kernel/power/power.h b/kernel/power/power.h
index f65fcf7..b8020e9 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -16,6 +16,11 @@ struct swsusp_info {
} __aligned(PAGE_SIZE);
#ifdef CONFIG_HIBERNATION
+#ifdef CONFIG_HIBERNATE_VERIFICATION
+/* arch/x86/power/hibernate_keys.c */
+extern int get_swsusp_key(u8 **skey);
+#endif
+
/* kernel/power/snapshot.c */
extern void __init hibernate_reserved_size_init(void);
extern void __init hibernate_image_size_init(void);
--
1.8.4.5
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.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
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 b8020e9..25c541e 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..af60731 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[SWSUSP_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_swsusp_key(&key);
+ if (key_err)
+ goto copy_pages;
+
+ tfm = crypto_alloc_shash(SWSUSP_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, SWSUSP_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, SWSUSP_DIGEST_SIZE);
+ memcpy(signature, digest, SWSUSP_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_swsusp_key(&key);
+ if (ret)
+ goto forward_ret;
+
+ tfm = crypto_alloc_shash(SWSUSP_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, SWSUSP_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", SWSUSP_DIGEST_SIZE, signature);
+ pr_debug("PM: Digest %*phN\n", (int) digest_size, digest);
+ if (memcmp(signature, digest, SWSUSP_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(©_bm, &orig_bm);
+ ret = copy_data_pages(©_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, SWSUSP_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, SWSUSP_DIGEST_SIZE);
+ memcpy(signature, info->signature, SWSUSP_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(©_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;
--
1.8.4.5
The HMAC key should only resides in kernel memory space but not leak
to outside. To avoid including swsusp 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.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
arch/x86/power/hibernate_keys.c | 14 ++++++++++++++
kernel/power/power.h | 3 +++
kernel/power/snapshot.c | 6 ++++++
3 files changed, 23 insertions(+)
diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c
index 4a68b86..775c6d8 100644
--- a/arch/x86/power/hibernate_keys.c
+++ b/arch/x86/power/hibernate_keys.c
@@ -48,6 +48,20 @@ int get_swsusp_key(u8 **skey)
return swsusp_keys->skey_status;
}
+bool swsusp_page_is_keys(struct page *page)
+{
+ bool ret = false;
+
+ if (!swsusp_keys || swsusp_keys->skey_status)
+ return ret;
+
+ ret = (page_to_pfn(page) == page_to_pfn(virt_to_page(swsusp_keys)));
+ if (ret)
+ pr_info("PM: Avoid snapshot the page of swsusp key.\n");
+
+ return ret;
+}
+
static int __init init_hibernate_keys(void)
{
struct swsusp_keys *keys;
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 25c541e..a09b21d 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_swsusp_key(u8 **skey);
+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 af60731..c2bce90 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;
--
1.8.4.5
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 swsusp key.
That because those informations did not include in hibernate image or
produced when restoring.
The significant reason is image kernel needs swsusp 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 swsusp key data to key page,
and cleaning this page buffer for next cycle.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
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 775c6d8..9c3d2fe 100644
--- a/arch/x86/power/hibernate_keys.c
+++ b/arch/x86/power/hibernate_keys.c
@@ -20,6 +20,15 @@ static u64 keys_phys_addr;
/* A page used to keep swsusp keys */
static struct swsusp_keys *swsusp_keys;
+/* Forward information and keys from boot kernel to image kernel */
+struct forward_info {
+ bool sig_enforce;
+ int sig_verify_ret;
+ struct swsusp_keys swsusp_keys;
+};
+
+static struct forward_info *forward_buff;
+
void __init parse_swsusp_keys(u64 phys_addr, u32 data_len)
{
struct setup_data *swsusp_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 (swsusp_keys && !swsusp_keys->skey_status) {
+ info->swsusp_keys = *swsusp_keys;
+ memset(swsusp_keys, 0, PAGE_SIZE);
+ } else
+ pr_info("PM: Fill swsusp 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 (swsusp_keys) {
+ memset(swsusp_keys, 0, PAGE_SIZE);
+ *swsusp_keys = forward_buff->swsusp_keys;
+ pr_info("PM: Restored swsusp keys\n");
+ }
+
+ /* clean forward information buffer for next round */
+ memset(forward_buff, 0, PAGE_SIZE);
+}
+
static int __init init_hibernate_keys(void)
{
struct swsusp_keys *keys;
@@ -76,6 +134,11 @@ static int __init init_hibernate_keys(void)
swsusp_keys = (struct swsusp_keys *)get_zeroed_page(GFP_KERNEL);
if (swsusp_keys) {
*swsusp_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 swsusp 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 a09b21d..5ee0074 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -12,6 +12,7 @@ struct swsusp_info {
unsigned long image_pages;
unsigned long pages;
unsigned long size;
+ unsigned long forward_buff_pfn;
u8 signature[SWSUSP_DIGEST_SIZE];
} __aligned(PAGE_SIZE);
@@ -20,8 +21,13 @@ struct swsusp_info {
/* arch/x86/power/hibernate_keys.c */
extern int get_swsusp_key(u8 **skey);
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 c2bce90..a19ac11 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1281,6 +1281,14 @@ static unsigned int nr_copy_pages;
*/
static u8 signature[SWSUSP_DIGEST_SIZE];
+/*
+ * Keep the pfn of forwarding information buffer from resume target.
+ * Writing swsusp 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 swsusp 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, SWSUSP_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, SWSUSP_DIGEST_SIZE);
memcpy(signature, info->signature, SWSUSP_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 swsusp 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();
--
1.8.4.5
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.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
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 9c3d2fe..9a0f3b3 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 (swsusp_keys && !swsusp_keys->skey_status) {
info->swsusp_keys = *swsusp_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 (swsusp_keys) {
memset(swsusp_keys, 0, PAGE_SIZE);
*swsusp_keys = forward_buff->swsusp_keys;
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 5f0be58..0add6e6 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -488,6 +488,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 1ec7d11..fc3dde0 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -335,6 +335,9 @@ struct platform_hibernation_ops {
#define SWSUSP_HMAC "hmac(sha1)"
#define SWSUSP_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 a19ac11..3eda715 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;
}
--
1.8.4.5
This patch provides a ioctl for triggering swsusp 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 swsusp key when queried flag.
To aviod the swsusp 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.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
arch/x86/boot/compressed/eboot.c | 20 +++++++++++---
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 | 43 +++++++++++++++++++++++++++++++
include/linux/suspend.h | 15 +++++++++++
include/uapi/linux/suspend_ioctls.h | 3 ++-
kernel/power/Kconfig | 1 +
kernel/power/hibernate.c | 2 ++
kernel/power/user.c | 7 +++++
kernel/reboot.c | 3 +++
12 files changed, 97 insertions(+), 5 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 5e1476e..b959f83 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1394,9 +1394,10 @@ static void setup_swsusp_keys(struct boot_params *params)
{
struct setup_data *setup_data, *swsusp_setup_data;
struct swsusp_keys *swsusp_keys;
+ bool regen_key = false;
int size = 0;
- unsigned long key_size, attributes;
- efi_status_t status;
+ unsigned long ignore, key_size, attributes;
+ efi_status_t status, reg_status;
/* Allocate setup_data to carry keys */
size = sizeof(struct setup_data) + sizeof(struct swsusp_keys);
@@ -1425,12 +1426,16 @@ static void setup_swsusp_keys(struct boot_params *params)
}
}
- if (status != EFI_SUCCESS) {
- efi_printk(sys_table, "Failed to get existing swsusp key\n");
+ reg_status = efi_call_early(get_variable, SWSUSP_KEY_REGEN_FLAG,
+ &EFI_SWSUSP_GUID, NULL, &ignore, ®en_key);
+ if ((status != EFI_SUCCESS) ||
+ (reg_status == EFI_SUCCESS && regen_key)) {
+ efi_printk(sys_table, "Regenerating swsusp key\n");
efi_get_random_key(sys_table, params, swsusp_keys->swsusp_key,
SWSUSP_DIGEST_SIZE);
+ /* Set new swsusp key to bootservice non-volatile variable */
status = efi_call_early(set_variable, SWSUSP_KEY,
&EFI_SWSUSP_GUID,
SWSUSP_KEY_ATTRIBUTE,
@@ -1438,6 +1443,13 @@ static void setup_swsusp_keys(struct boot_params *params)
swsusp_keys->swsusp_key);
if (status != EFI_SUCCESS)
efi_printk(sys_table, "Failed to set swsusp key\n");
+
+ efi_call_early(get_variable, SWSUSP_KEY, &EFI_SWSUSP_GUID,
+ NULL, &key_size, swsusp_keys->swsusp_key);
+
+ /* Clean key regenerate flag */
+ efi_call_early(set_variable, SWSUSP_KEY_REGEN_FLAG,
+ &EFI_SWSUSP_GUID, 0, 0, NULL);
}
clean_fail:
diff --git a/arch/x86/power/hibernate_keys.c b/arch/x86/power/hibernate_keys.c
index 9a0f3b3..76811d4 100644
--- a/arch/x86/power/hibernate_keys.c
+++ b/arch/x86/power/hibernate_keys.c
@@ -165,6 +165,8 @@ static int __init init_hibernate_keys(void)
memblock_free(keys_phys_addr, sizeof(struct swsusp_keys));
keys_phys_addr = 0;
+ set_swsusp_key_regen_flag = false;
+
return ret;
}
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 4a4b897..51b0c38 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_SWSUSP_KEYS) += efi/
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 54071c1..de3df38 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -69,3 +69,7 @@ endmenu
config UEFI_CPER
bool
+
+config EFI_SWSUSP_KEYS
+ bool
+ select EFI_VARS
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 6fd3da9..98d9fbc 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_SWSUSP_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..90ae912
--- /dev/null
+++ b/drivers/firmware/efi/efi-hibernate_keys.c
@@ -0,0 +1,43 @@
+/* EFI variable handler of swsusp key regen flag
+ *
+ * Copyright (C) 2015 SUSE Linux Products GmbH. All rights reserved.
+ * Written by Chun-Yi Lee ([email protected])
+ *
+ * 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>
+
+/* Set this flag will creating SWSUSPKeyRegen EFI variable */
+bool set_swsusp_key_regen_flag;
+
+void create_swsusp_key_regen_flag(void)
+{
+ struct efivar_entry *entry = NULL;
+ int err = 0;
+
+ if (!set_swsusp_key_regen_flag)
+ return;
+
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ return;
+
+ memcpy(entry->var.VariableName,
+ SWSUSP_KEY_REGEN_FLAG, sizeof(SWSUSP_KEY_REGEN_FLAG));
+ memcpy(&(entry->var.VendorGuid),
+ &EFI_SWSUSP_GUID, sizeof(efi_guid_t));
+
+ err = efivar_entry_set(entry, SWSUSP_KEY_SEED_ATTRIBUTE,
+ sizeof(bool), &set_swsusp_key_regen_flag, NULL);
+ if (err)
+ pr_warn("PM: Set flag of regenerating swsusp key failed: %d\n", err);
+
+ kfree(entry);
+}
+EXPORT_SYMBOL_GPL(create_swsusp_key_regen_flag);
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index fc3dde0..db8958c 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -330,6 +330,11 @@ struct platform_hibernation_ops {
#define EFI_SWSUSP_GUID \
EFI_GUID(0xfe141863, 0xc070, 0x478e, 0xb8, 0xa3, 0x87, 0x8a, 0x5d, 0xc9, 0xef, 0x21)
+#define SWSUSP_KEY_REGEN_FLAG \
+ ((efi_char16_t [15]) { 'S', 'W', 'S', 'U', 'S', 'P', 'K', 'e', 'y', 'R', 'e', 'g', 'e', 'n', 0 })
+#define SWSUSP_KEY_SEED_ATTRIBUTE (EFI_VARIABLE_NON_VOLATILE | \
+ EFI_VARIABLE_BOOTSERVICE_ACCESS | \
+ EFI_VARIABLE_RUNTIME_ACCESS)
/* HMAC Algorithm of Hibernate Signature */
#define SWSUSP_HMAC "hmac(sha1)"
@@ -338,6 +343,16 @@ struct platform_hibernation_ops {
/* kernel/power/hibernate.c */
extern int sigenforce;
+/* drivers/firmware/efi/efi-hibernate_keys.c */
+extern bool set_swsusp_key_regen_flag;
+
+#ifdef CONFIG_HIBERNATE_VERIFICATION
+/* drivers/firmware/efi/efi-hibernate_keys.c */
+extern void create_swsusp_key_regen_flag(void);
+#else
+static inline void create_swsusp_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)
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..7a64bda 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_SWSUSP_KEYS
select CRYPTO_HMAC
select CRYPTO_SHA1
help
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 2c2cc90..314f268 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -653,6 +653,8 @@ int hibernate(void)
{
int error;
+ set_swsusp_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..8bcb051 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -338,6 +338,8 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
error = -EPERM;
break;
}
+ /* clean flag to avoid swsusp key regenerated */
+ set_swsusp_key_regen_flag = false;
/*
* Tasks are frozen and the notifiers have been called with
* PM_HIBERNATION_PREPARE
@@ -351,6 +353,7 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
break;
case SNAPSHOT_POWER_OFF:
+ set_swsusp_key_regen_flag = false;
if (data->platform_support)
error = hibernation_platform_enter();
break;
@@ -386,6 +389,10 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
}
break;
+ case SNAPSHOT_REGENERATE_KEY:
+ set_swsusp_key_regen_flag = !!arg;
+ break;
+
default:
error = -ENOTTY;
diff --git a/kernel/reboot.c b/kernel/reboot.c
index d20c85d..88e51e3 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_swsusp_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_swsusp_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_swsusp_key_regen_flag();
kernel_shutdown_prepare(SYSTEM_POWER_OFF);
if (pm_power_off_prepare)
pm_power_off_prepare();
--
1.8.4.5
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.
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
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 90ae912..f33bf70 100644
--- a/drivers/firmware/efi/efi-hibernate_keys.c
+++ b/drivers/firmware/efi/efi-hibernate_keys.c
@@ -21,6 +21,9 @@ void create_swsusp_key_regen_flag(void)
struct efivar_entry *entry = NULL;
int err = 0;
+ if (!efi_enabled(EFI_RUNTIME_SERVICES))
+ return;
+
if (!set_swsusp_key_regen_flag)
return;
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 7a64bda..5b04ab9 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 3eda715..0d64c3a 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 8bcb051..d7407ef 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>
@@ -390,7 +391,10 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
break;
case SNAPSHOT_REGENERATE_KEY:
- set_swsusp_key_regen_flag = !!arg;
+ if (!efi_enabled(EFI_BOOT))
+ error = -ENODEV;
+ else
+ set_swsusp_key_regen_flag = !!arg;
break;
default:
--
1.8.4.5
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
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..492e047
--- /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 swsusp
+key changes in hibernating cycle that causes hibernate restoring failed,
+the regen flag will be clear in a hibernate cycle.
--
1.8.4.5
On Thu, 16 Jul 2015, Lee, Chun-Yi wrote:
> 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.
I've finally managed to fully go through the series and test it on my
secure boot testing setup.
Reviewed-by: Jiri Kosina <[email protected]>
Tested-by: Jiri Kosina <[email protected]>
for the whole series. Thanks,
--
Jiri Kosina
SUSE Labs
On Friday, July 24, 2015 07:08:18 PM Jiri Kosina wrote:
> On Thu, 16 Jul 2015, Lee, Chun-Yi wrote:
>
> > 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.
>
> I've finally managed to fully go through the series and test it on my
> secure boot testing setup.
>
> Reviewed-by: Jiri Kosina <[email protected]>
> Tested-by: Jiri Kosina <[email protected]>
>
> for the whole series. Thanks,
Thanks Jiri, much appreciated!
I'd like to hear from Matt, though, and it looks like Ingo han't been CCed ...
Thanks,
Rafael
Hi Jiri,
On Fri, Jul 24, 2015 at 07:08:18PM +0200, Jiri Kosina wrote:
> On Thu, 16 Jul 2015, Lee, Chun-Yi wrote:
>
> > 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.
>
> I've finally managed to fully go through the series and test it on my
> secure boot testing setup.
>
> Reviewed-by: Jiri Kosina <[email protected]>
> Tested-by: Jiri Kosina <[email protected]>
>
> for the whole series. Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
Thanks for your review and test.
Joey Lee
Hi!
> 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.
>
> Signed-off-by: Lee, Chun-Yi <[email protected]>
> +#define X86_FEATURE_EDX_TSC (1 << 4)
> +#define X86_FEATURE_ECX_RDRAND (1 << 30)
Can you pull it from existing includes somewhere?
> +static bool rdrand_feature(void)
> +{
> + return (cpuid_ecx(0x1) & X86_FEATURE_ECX_RDRAND);
> +}
> +
> +static bool rdtsc_feature(void)
> +{
> + return (cpuid_edx(0x1) & X86_FEATURE_EDX_TSC);
> +}
Are these helpers neccessary?
> + if (rdrand_feature()) {
> + efi_printk(sys_table, " RDRAND");
> + if (rdrand_long(&raw)) {
> + random ^= raw;
> + use_i8254 = false;
> + }
> + }
> +
> + if (rdtsc_feature()) {
> + efi_printk(sys_table, " RDTSC");
> + rdtscll(raw);
> +
> + random ^= raw;
> + use_i8254 = false;
> + }
You'll do two (expensive) cpuids calls here.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu 2015-07-16 22:25:15, Lee, Chun-Yi wrote:
> 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, add HIBERNATE_VERIFICATION kernel config.
>
> Signed-off-by: Lee, Chun-Yi <[email protected]>
> ---
> include/linux/suspend.h | 5 +++++
> kernel/power/Kconfig | 13 +++++++++++++
> kernel/power/power.h | 1 +
> 3 files changed, 19 insertions(+)
>
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 5efe743..6cd2a48 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 SWSUSP_HMAC "hmac(sha1)"
> +#define SWSUSP_DIGEST_SIZE 20
I'd replace SWSUSP with HIBERNATION here, and pretty much everywhere.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri, 2015-07-24 at 22:08 +0200, Rafael J. Wysocki wrote:
> On Friday, July 24, 2015 07:08:18 PM Jiri Kosina wrote:
> > On Thu, 16 Jul 2015, Lee, Chun-Yi wrote:
> >
> > > 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.
> >
> > I've finally managed to fully go through the series and test it on my
> > secure boot testing setup.
> >
> > Reviewed-by: Jiri Kosina <[email protected]>
> > Tested-by: Jiri Kosina <[email protected]>
> >
> > for the whole series. Thanks,
>
> Thanks Jiri, much appreciated!
>
> I'd like to hear from Matt, though, and it looks like Ingo han't been CCed ...
I'm hoping to get time to review this series this week.
Hi!
> int cmdline_find_option_bool(const char *option);
> #endif
>
> +#if CONFIG_RANDOMIZE_BASE
Not ifdef?
> +extern u16 i8254(void);
That's rather poor name for global function...
> #if CONFIG_RANDOMIZE_BASE
Ok, maybe not, but I'm confused.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu 2015-07-16 22:25:19, 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 <[email protected]>
> ---
> arch/x86/boot/compressed/efi_random.c | 193 ++++++++++++++++++++++++++++++++++
> include/linux/efi.h | 46 ++++++++
> 2 files changed, 239 insertions(+)
>
> diff --git a/arch/x86/boot/compressed/efi_random.c b/arch/x86/boot/compressed/efi_random.c
> index bdb2d46..1f5c63d 100644
> --- a/arch/x86/boot/compressed/efi_random.c
> +++ b/arch/x86/boot/compressed/efi_random.c
> @@ -2,6 +2,191 @@
>
> #include <linux/efi.h>
> #include <asm/archrandom.h>
> +#include <asm/efi.h>
> +
> +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;
> +
> + 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");
> + 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 locateEFI_RNG_PROTOCOL");
missing \n?
> + goto free_handle;
You use that label exactly once, no need for goto
> +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;
...> +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;
...
> +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;
...
> +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;
Can you do something to avoid each function having two very similar
versions of these functions?
> + if (status != EFI_SUCCESS) {
> + efi_printk(sys_table, " Failed to get RNG value ");
> + efi_printk(sys_table, efi_status_to_str(status));
Yep. You definitely have \n problems here.
> --- 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 don't typedef structs usually...
Make it union so you can have just one
> +static inline char *efi_status_to_str(efi_status_t status)
> +{
> + char *str;
> +
Are you sure you want this inlined?
> + switch (status) {
> + case EFI_SUCCESS:
> + str = "EFI_SUCCESS";
> + break;
Can you use macros to reduce code duplication here?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> For generating a messy number as a 20 bytes key, the codes in EFI
> stub gets u32 random number five time and every random number is
> rolling that last u32 random number as entropy.
Parse error here.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Typo in patch subject.
> And for earsing footbprints, the codes in this patch remove setup
And two typos here.
> data that carried swsusp key, and clean the memory space that
And don't call it swsusp. Please fix globally.
> +++ b/arch/x86/power/hibernate_keys.c
> @@ -0,0 +1,79 @@
> +/* Swsusp keys handler
> + *
> + * Copyright (C) 2015 SUSE Linux Products GmbH. All rights
Are you sure?
> +static int __init init_hibernate_keys(void)
> +{
> + struct swsusp_keys *keys;
> + int ret = 0;
> +
> + if (!keys_phys_addr)
> + return -ENODEV;
> +
> + keys = early_memremap(keys_phys_addr, sizeof(struct swsusp_keys));
> +
> + /* Copy swsusp keys to a allocated page */
> + swsusp_keys = (struct swsusp_keys *)get_zeroed_page(GFP_KERNEL);
> + if (swsusp_keys) {
> + *swsusp_keys = *keys;
> + } else {
> + pr_err("PM: Allocate swsusp keys page failed\n");
> + ret = -ENOMEM;
> + }
> +
> + /* Earse keys data no matter copy success or failed */
> + memset(keys, 0, sizeof(struct swsusp_keys));
> + early_memunmap(keys, sizeof(struct swsusp_keys));
> + memblock_free(keys_phys_addr, sizeof(struct swsusp_keys));
> + keys_phys_addr = 0;
> +
> + return ret;
> +}
> +
> +late_initcall(init_hibernate_keys);
init_hibernation_keys.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu, 2015-07-16 at 22:25 +0800, 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 veriable.
>
> Signed-off-by: Lee, Chun-Yi <[email protected]>
Maybe mention that we want to be able to read/write variables from
within the EFI boot stub? That's really why you want to add
{get,set}_variable pointers.
s/veriable/variable/
Rest of the patch looks good.
On Thu, 2015-07-16 at 22:25 +0800, 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.
>
> Signed-off-by: Lee, Chun-Yi <[email protected]>
> ---
> arch/x86/boot/compressed/Makefile | 1 +
> arch/x86/boot/compressed/efi_random.c | 88 +++++++++++++++++++++++++++++++++++
> arch/x86/boot/compressed/misc.c | 4 +-
> arch/x86/boot/compressed/misc.h | 2 +-
> 4 files changed, 92 insertions(+), 3 deletions(-)
> create mode 100644 arch/x86/boot/compressed/efi_random.c
[...]
> +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;
> +
> + efi_printk(sys_table, " EFI random");
Probably want to remove these efi_printk()s from the final version ;-)
> + if (entropy)
> + random = entropy;
> + else
> + random = get_random_boot(boot_params);
> +
> + if (rdrand_feature()) {
> + efi_printk(sys_table, " RDRAND");
> + if (rdrand_long(&raw)) {
> + random ^= raw;
> + use_i8254 = false;
> + }
> + }
> +
> + if (rdtsc_feature()) {
> + efi_printk(sys_table, " RDTSC");
> + rdtscll(raw);
> +
> + random ^= raw;
> + use_i8254 = false;
> + }
> +
> + if (use_i8254) {
> + efi_printk(sys_table, " i8254");
> + random ^= i8254();
> + }
> +
> + /* Circular multiply for better bit diffusion */
> + asm("mul %3"
> + : "=a" (random), "=d" (raw)
> + : "a" (random), "rm" (mix_const));
> + random += raw;
> +
> + efi_printk(sys_table, "...\n");
> +
> + return random;
> +}
> +
> +void efi_get_random_key(efi_system_table_t *sys_table,
> + struct boot_params *params, u8 key[], int size)
> +{
I would think that the size of the key array should be unsigned.
On Thu, 2015-07-16 at 22:25 +0800, 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 <[email protected]>
> ---
> arch/x86/boot/compressed/efi_random.c | 193 ++++++++++++++++++++++++++++++++++
> include/linux/efi.h | 46 ++++++++
> 2 files changed, 239 insertions(+)
[...]
> @@ -2,6 +2,191 @@
>
> #include <linux/efi.h>
> #include <asm/archrandom.h>
> +#include <asm/efi.h>
> +
> +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;
> +
> + 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");
> + 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");
> + goto free_handle;
> + }
> +
> + return EFI_SUCCESS;
> +
> +free_handle:
> + efi_call_early(free_pool, *rng_handle);
> +
> + return status;
> +}
I would suggest setting *rng_handle = NULL at the beginning of this
function just because if we ever forget to set it that way in the caller
this free_pool call might do screwy things.
> +static bool efi_rng_supported(efi_system_table_t *sys_table)
> +{
> + const struct efi_config *efi_early = __efi_early();
> + u32 random = 0;
> + 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)
> + random = efi_rng_supported64(sys_table, rng_handle);
> + else
> + random = efi_rng_supported32(sys_table, rng_handle);
> +
> + efi_call_early(free_pool, rng_handle);
> +
> + return random;
Oops, 'random' isn't a bool but it should be.
> @@ -51,6 +236,14 @@ static unsigned long get_random_long(unsigned long entropy,
> use_i8254 = false;
> }
>
> + if (efi_rng_supported(sys_table)) {
> + efi_printk(sys_table, " EFI_RNG");
> + raw = efi_get_rng(sys_table);
> + if (raw)
> + random ^= raw;
> + use_i8254 = false;
> + }
> +
> if (use_i8254) {
> efi_printk(sys_table, " i8254");
> random ^= i8254();
Have you looked at the tradeoff in terms of boot time for building a key
array in 'unsigned long' chunks as opposed to passing the array and size
directly for the RNG protocol?
On Thu, 2015-07-16 at 22:25 +0800, 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.
>
> For generating a messy number as a 20 bytes key, the codes in EFI
> stub gets u32 random number five time and every random number is
> rolling that last u32 random number as entropy.
>
> The HMAC key stored in EFI boot service variable, the GUID is
> S4SignKey-fe141863-c070-478e-b8a3-878a5dc9ef21.
[...]
> @@ -1383,6 +1384,60 @@ free_mem_map:
> return status;
> }
>
> +#ifdef CONFIG_HIBERNATE_VERIFICATION
> +#define SWSUSP_KEY \
> + ((efi_char16_t [10]) { 'S', 'W', 'S', 'U', 'S', 'P', 'K', 'e', 'y', 0 })
> +#define SWSUSP_KEY_ATTRIBUTE (EFI_VARIABLE_NON_VOLATILE | \
> + EFI_VARIABLE_BOOTSERVICE_ACCESS)
You mean "SWSUSPKey" not "S4SignKey" right?
On Thu, 2015-07-16 at 22:25 +0800, Lee, Chun-Yi wrote:
> Moved the function of transferring EFI status to kernel error for
> later used by EFI stub.
>
> Signed-off-by: Lee, Chun-Yi <[email protected]>
> ---
> drivers/firmware/efi/vars.c | 33 ---------------------------------
> include/linux/efi.h | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 33 insertions(+), 33 deletions(-)
The patch contents are fine but the title could do with some work,
"public" isn't a verb. I think "Make efi_status_to_err() public" would
be fine.
On Thu, 2015-07-16 at 22:25 +0800, Lee, Chun-Yi wrote:
> For forwarding swsusp key from EFI stub to boot kernel, this patch
> allocates setup data for carrying swsusp key, size and the status
> of efi operating.
>
> Signed-off-by: Lee, Chun-Yi <[email protected]>
> ---
> arch/x86/boot/compressed/eboot.c | 29 +++++++++++++++++++++++++----
> arch/x86/include/uapi/asm/bootparam.h | 1 +
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 97b356f..5e1476e 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -1392,19 +1392,23 @@ free_mem_map:
>
> static void setup_swsusp_keys(struct boot_params *params)
> {
> - unsigned long key_size, attributes;
> + struct setup_data *setup_data, *swsusp_setup_data;
> struct swsusp_keys *swsusp_keys;
> + int size = 0;
> + unsigned long key_size, attributes;
> efi_status_t status;
Why the local variable shuffling? It'd be better to leave key_size and
attributes alone.
Also, 'size' doesn't look like it should be int. If you're passing it to
allocate_pool it really wants to be 'unsigned long'.
> /* Allocate setup_data to carry keys */
> + size = sizeof(struct setup_data) + sizeof(struct swsusp_keys);
> status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> - sizeof(struct swsusp_keys), &swsusp_keys);
> + size, &swsusp_setup_data);
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table, "Failed to alloc mem for swsusp_keys\n");
> return;
> }
>
> - memset(swsusp_keys, 0, sizeof(struct swsusp_keys));
> + memset(swsusp_setup_data, 0, size);
> + swsusp_keys = (struct swsusp_keys *)swsusp_setup_data->data;
>
> status = efi_call_early(get_variable, SWSUSP_KEY, &EFI_SWSUSP_GUID,
> &attributes, &key_size, swsusp_keys->swsusp_key);
> @@ -1413,7 +1417,9 @@ static void setup_swsusp_keys(struct boot_params *params)
> memset(swsusp_keys->swsusp_key, 0, SWSUSP_DIGEST_SIZE);
> status = efi_call_early(set_variable, SWSUSP_KEY,
> &EFI_SWSUSP_GUID, attributes, 0, NULL);
> - if (status == EFI_SUCCESS) {
> + if (status)
> + goto clean_fail;
Please use the EFI status codes explicitly.
Hi Pavel,
Thanks for your review.
On Tue, Jul 28, 2015 at 02:01:12PM +0200, Pavel Machek wrote:
> Hi!
>
> > 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.
> >
> > Signed-off-by: Lee, Chun-Yi <[email protected]>
>
> > +#define X86_FEATURE_EDX_TSC (1 << 4)
> > +#define X86_FEATURE_ECX_RDRAND (1 << 30)
>
> Can you pull it from existing includes somewhere?
>
I didn't see similar definition in header.
> > +static bool rdrand_feature(void)
> > +{
> > + return (cpuid_ecx(0x1) & X86_FEATURE_ECX_RDRAND);
> > +}
> > +
> > +static bool rdtsc_feature(void)
> > +{
> > + return (cpuid_edx(0x1) & X86_FEATURE_EDX_TSC);
> > +}
>
> Are these helpers neccessary?
I will try to simplify it.
>
> > + if (rdrand_feature()) {
> > + efi_printk(sys_table, " RDRAND");
> > + if (rdrand_long(&raw)) {
> > + random ^= raw;
> > + use_i8254 = false;
> > + }
> > + }
> > +
> > + if (rdtsc_feature()) {
> > + efi_printk(sys_table, " RDTSC");
> > + rdtscll(raw);
> > +
> > + random ^= raw;
> > + use_i8254 = false;
> > + }
>
> You'll do two (expensive) cpuids calls here.
>
I will try to avoid call cpuid many times.
Thanks a lot!
Joey Lee
Hi Matt,
Thanks for your review!
On Thu, Jul 30, 2015 at 04:37:42PM +0100, Matt Fleming wrote:
> On Thu, 2015-07-16 at 22:25 +0800, 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.
> >
> > Signed-off-by: Lee, Chun-Yi <[email protected]>
> > ---
> > arch/x86/boot/compressed/Makefile | 1 +
> > arch/x86/boot/compressed/efi_random.c | 88 +++++++++++++++++++++++++++++++++++
> > arch/x86/boot/compressed/misc.c | 4 +-
> > arch/x86/boot/compressed/misc.h | 2 +-
> > 4 files changed, 92 insertions(+), 3 deletions(-)
> > create mode 100644 arch/x86/boot/compressed/efi_random.c
>
> [...]
>
> > +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;
> > +
> > + efi_printk(sys_table, " EFI random");
>
> Probably want to remove these efi_printk()s from the final version ;-)
>
OK, I will remove those log.
> > + if (entropy)
> > + random = entropy;
> > + else
> > + random = get_random_boot(boot_params);
> > +
> > + if (rdrand_feature()) {
> > + efi_printk(sys_table, " RDRAND");
> > + if (rdrand_long(&raw)) {
> > + random ^= raw;
> > + use_i8254 = false;
> > + }
> > + }
> > +
> > + if (rdtsc_feature()) {
> > + efi_printk(sys_table, " RDTSC");
> > + rdtscll(raw);
> > +
> > + random ^= raw;
> > + use_i8254 = false;
> > + }
> > +
> > + if (use_i8254) {
> > + efi_printk(sys_table, " i8254");
> > + random ^= i8254();
> > + }
> > +
> > + /* Circular multiply for better bit diffusion */
> > + asm("mul %3"
> > + : "=a" (random), "=d" (raw)
> > + : "a" (random), "rm" (mix_const));
> > + random += raw;
> > +
> > + efi_printk(sys_table, "...\n");
> > +
> > + return random;
> > +}
> > +
> > +void efi_get_random_key(efi_system_table_t *sys_table,
> > + struct boot_params *params, u8 key[], int size)
> > +{
>
> I would think that the size of the key array should be unsigned.
>
I will change size to unsigned int.
Thanks
Joey Lee
On Tue, Jul 28, 2015 at 02:28:53PM +0200, Pavel Machek wrote:
> On Thu 2015-07-16 22:25:19, 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 <[email protected]>
> > ---
> > arch/x86/boot/compressed/efi_random.c | 193 ++++++++++++++++++++++++++++++++++
> > include/linux/efi.h | 46 ++++++++
> > 2 files changed, 239 insertions(+)
> >
> > diff --git a/arch/x86/boot/compressed/efi_random.c b/arch/x86/boot/compressed/efi_random.c
> > index bdb2d46..1f5c63d 100644
> > --- a/arch/x86/boot/compressed/efi_random.c
> > +++ b/arch/x86/boot/compressed/efi_random.c
> > @@ -2,6 +2,191 @@
> >
> > #include <linux/efi.h>
> > #include <asm/archrandom.h>
> > +#include <asm/efi.h>
> > +
> > +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;
> > +
> > + 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");
> > + 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 locateEFI_RNG_PROTOCOL");
>
> missing \n?
>
Originally those logs just follow a "EFI random" as a complete line. After
removed "EFI random", I will add "\n" back to those log.
> > + goto free_handle;
>
> You use that label exactly once, no need for goto
>
OK, I will remove free_handle label.
> > +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;
> ...> +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;
> ...
> > +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;
> ...
> > +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;
>
> Can you do something to avoid each function having two very similar
> versions of these functions?
>
They are similar but I want follow the style in eboot.c.
On the other hand, it's earlier to locate problem on 32-bit or 64-bit EFI.
So, I will keep the above codes.
> > + if (status != EFI_SUCCESS) {
> > + efi_printk(sys_table, " Failed to get RNG value ");
> > + efi_printk(sys_table, efi_status_to_str(status));
>
> Yep. You definitely have \n problems here.
Thanks, I will add \n here also.
>
> > --- 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 don't typedef structs usually...
>
> Make it union so you can have just one
>
I want to follow the style as efi_pci_io_protocolxxx in efi.h.
So I will keep it.
> > +static inline char *efi_status_to_str(efi_status_t status)
> > +{
> > + char *str;
> > +
>
> Are you sure you want this inlined?
>
It's inlined because in header file.
Currently it's only used by efi_random.c, I will move it to efi_random.
> > + switch (status) {
> > + case EFI_SUCCESS:
> > + str = "EFI_SUCCESS";
> > + break;
>
> Can you use macros to reduce code duplication here?
> Pavel
I will try to reduce duplicate code.
Thanks
Joey Lee
On Tue, Jul 28, 2015 at 02:01:56PM +0200, Pavel Machek wrote:
> On Thu 2015-07-16 22:25:15, Lee, Chun-Yi wrote:
> > 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, add HIBERNATE_VERIFICATION kernel config.
> >
> > Signed-off-by: Lee, Chun-Yi <[email protected]>
> > ---
> > include/linux/suspend.h | 5 +++++
> > kernel/power/Kconfig | 13 +++++++++++++
> > kernel/power/power.h | 1 +
> > 3 files changed, 19 insertions(+)
> >
> > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > index 5efe743..6cd2a48 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 SWSUSP_HMAC "hmac(sha1)"
> > +#define SWSUSP_DIGEST_SIZE 20
>
> I'd replace SWSUSP with HIBERNATION here, and pretty much everywhere.
>
SWSUSP is shorter than HIBERNATION, and there have some codes in hibernate
are also using swsusp. I still want to use it.
Thanks a lot!
Joey Lee
On Thu, Jul 30, 2015 at 04:19:58PM +0100, Matt Fleming wrote:
> On Thu, 2015-07-16 at 22:25 +0800, 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 veriable.
> >
> > Signed-off-by: Lee, Chun-Yi <[email protected]>
>
> Maybe mention that we want to be able to read/write variables from
> within the EFI boot stub? That's really why you want to add
> {get,set}_variable pointers.
>
I will add more detail for accessing efi boot service
variable is because that will not touched by unauthenticated
codes.
> s/veriable/variable/
>
I will fix the typo.
> Rest of the patch looks good.
>
Thanks a lot!
Joey Lee
On Tue, Jul 28, 2015 at 02:21:33PM +0200, Pavel Machek wrote:
> Hi!
>
> > int cmdline_find_option_bool(const char *option);
> > #endif
> >
> > +#if CONFIG_RANDOMIZE_BASE
>
> Not ifdef?
>
> > +extern u16 i8254(void);
>
> That's rather poor name for global function...
This i8254 function only used by aslr and efi_random.
I will keep this name for do not change codes in aslr for naming.
>
> > #if CONFIG_RANDOMIZE_BASE
>
> Ok, maybe not, but I'm confused.
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Thanks a lot!
Joey Lee
On Tue, Jul 28, 2015 at 02:30:26PM +0200, Pavel Machek wrote:
>
> > For generating a messy number as a 20 bytes key, the codes in EFI
> > stub gets u32 random number five time and every random number is
> > rolling that last u32 random number as entropy.
>
> Parse error here.
>
Sorry for I didn't remove old log for developing version. I will remove it.
Thanks a lot!
Joey Lee
On Fri, 2015-07-31 at 17:58 +0800, joeyli wrote:
> >
> > Can you do something to avoid each function having two very similar
> > versions of these functions?
> >
>
> They are similar but I want follow the style in eboot.c.
> On the other hand, it's earlier to locate problem on 32-bit or 64-bit EFI.
>
> So, I will keep the above codes.
FWIW, I think that's fine. I would happily accept a patch to cleanup the
duplication, but I don't think that needs to be a prerequisite for this
support.
I've no problem with the duplication right now.
> >
> > > --- 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 don't typedef structs usually...
> >
> > Make it union so you can have just one
> >
>
> I want to follow the style as efi_pci_io_protocolxxx in efi.h.
> So I will keep it.
Yeah, consistency is better here than sticking with the general Linux
coding style rules.
> > > + switch (status) {
> > > + case EFI_SUCCESS:
> > > + str = "EFI_SUCCESS";
> > > + break;
> >
> > Can you use macros to reduce code duplication here?
> > Pavel
> I will try to reduce duplicate code.
Take a look at __stringify().
On Fri 2015-07-31 18:08:12, joeyli wrote:
> On Tue, Jul 28, 2015 at 02:01:56PM +0200, Pavel Machek wrote:
> > On Thu 2015-07-16 22:25:15, Lee, Chun-Yi wrote:
> > > 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, add HIBERNATE_VERIFICATION kernel config.
> > >
> > > Signed-off-by: Lee, Chun-Yi <[email protected]>
> > > ---
> > > include/linux/suspend.h | 5 +++++
> > > kernel/power/Kconfig | 13 +++++++++++++
> > > kernel/power/power.h | 1 +
> > > 3 files changed, 19 insertions(+)
> > >
> > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > > index 5efe743..6cd2a48 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 SWSUSP_HMAC "hmac(sha1)"
> > > +#define SWSUSP_DIGEST_SIZE 20
> >
> > I'd replace SWSUSP with HIBERNATION here, and pretty much everywhere.
> >
>
> SWSUSP is shorter than HIBERNATION, and there have some codes in hibernate
> are also using swsusp. I still want to use it.
Yes, its shorter, but its old name we are trying to move away
from. Please do the same.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri 2015-07-31 18:52:09, joeyli wrote:
> On Tue, Jul 28, 2015 at 02:21:33PM +0200, Pavel Machek wrote:
> > Hi!
> >
> > > int cmdline_find_option_bool(const char *option);
> > > #endif
> > >
> > > +#if CONFIG_RANDOMIZE_BASE
> >
> > Not ifdef?
> >
> > > +extern u16 i8254(void);
> >
> > That's rather poor name for global function...
>
> This i8254 function only used by aslr and efi_random.
> I will keep this name for do not change codes in aslr for naming.
No. That's not suitable function name for global function. And you are
making it global.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu, Jul 30, 2015 at 05:11:44PM +0100, Matt Fleming wrote:
> On Thu, 2015-07-16 at 22:25 +0800, 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 <[email protected]>
> > ---
> > arch/x86/boot/compressed/efi_random.c | 193 ++++++++++++++++++++++++++++++++++
> > include/linux/efi.h | 46 ++++++++
> > 2 files changed, 239 insertions(+)
>
> [...]
>
> > @@ -2,6 +2,191 @@
> >
> > #include <linux/efi.h>
> > #include <asm/archrandom.h>
> > +#include <asm/efi.h>
> > +
> > +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;
> > +
> > + 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");
> > + 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");
> > + goto free_handle;
> > + }
> > +
> > + return EFI_SUCCESS;
> > +
> > +free_handle:
> > + efi_call_early(free_pool, *rng_handle);
> > +
> > + return status;
> > +}
>
> I would suggest setting *rng_handle = NULL at the beginning of this
> function just because if we ever forget to set it that way in the caller
> this free_pool call might do screwy things.
>
Thanks for your suggestion, I will set NULL to *rng_handle.
>
> > +static bool efi_rng_supported(efi_system_table_t *sys_table)
> > +{
> > + const struct efi_config *efi_early = __efi_early();
> > + u32 random = 0;
> > + 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)
> > + random = efi_rng_supported64(sys_table, rng_handle);
> > + else
> > + random = efi_rng_supported32(sys_table, rng_handle);
> > +
> > + efi_call_early(free_pool, rng_handle);
> > +
> > + return random;
>
> Oops, 'random' isn't a bool but it should be.
>
I will change type of random to boot.
> > @@ -51,6 +236,14 @@ static unsigned long get_random_long(unsigned long entropy,
> > use_i8254 = false;
> > }
> >
> > + if (efi_rng_supported(sys_table)) {
> > + efi_printk(sys_table, " EFI_RNG");
> > + raw = efi_get_rng(sys_table);
> > + if (raw)
> > + random ^= raw;
> > + use_i8254 = false;
> > + }
> > +
> > if (use_i8254) {
> > efi_printk(sys_table, " i8254");
> > random ^= i8254();
>
> Have you looked at the tradeoff in terms of boot time for building a key
> array in 'unsigned long' chunks as opposed to passing the array and size
> directly for the RNG protocol?
>
I didn't really measure the speed, but directly passing array and size to
RNG protocol should a bit faster than calling the protocol a could of times.
But, the key generation process only in first time building or trigger by
user raises the rebuild flag. So, it doesn't affect to every booting time.
Due to I want let the whole key array more random, so each unsigned long
chunk was mixed(xor) by following entropy:
+ random long from RDRAND
+ RDTSC
+ random long from EFI RNG protocol
+ last unsigned long chunk
Another reason is voiding the result of EFI RNG protocol to get weight
higher than other source, in case too trust EFI RNG.
Thanks a lot!
Joey Lee
On Fri, Jul 31, 2015 at 10:59:12PM +0800, joeyli wrote:
> On Thu, Jul 30, 2015 at 05:11:44PM +0100, Matt Fleming wrote:
> > On Thu, 2015-07-16 at 22:25 +0800, 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 <[email protected]>
> > > ---
> > > arch/x86/boot/compressed/efi_random.c | 193 ++++++++++++++++++++++++++++++++++
> > > include/linux/efi.h | 46 ++++++++
> > > 2 files changed, 239 insertions(+)
> >
> > [...]
> >
> > > @@ -2,6 +2,191 @@
> > >
> > > #include <linux/efi.h>
> > > #include <asm/archrandom.h>
> > > +#include <asm/efi.h>
> > > +
> > > +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;
> > > +
> > > + 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");
> > > + 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");
> > > + goto free_handle;
> > > + }
> > > +
> > > + return EFI_SUCCESS;
> > > +
> > > +free_handle:
> > > + efi_call_early(free_pool, *rng_handle);
> > > +
> > > + return status;
> > > +}
> >
> > I would suggest setting *rng_handle = NULL at the beginning of this
> > function just because if we ever forget to set it that way in the caller
> > this free_pool call might do screwy things.
> >
>
> Thanks for your suggestion, I will set NULL to *rng_handle.
>
> >
> > > +static bool efi_rng_supported(efi_system_table_t *sys_table)
> > > +{
> > > + const struct efi_config *efi_early = __efi_early();
> > > + u32 random = 0;
> > > + 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)
> > > + random = efi_rng_supported64(sys_table, rng_handle);
> > > + else
> > > + random = efi_rng_supported32(sys_table, rng_handle);
> > > +
> > > + efi_call_early(free_pool, rng_handle);
> > > +
> > > + return random;
> >
> > Oops, 'random' isn't a bool but it should be.
> >
>
> I will change type of random to boot.
>
> > > @@ -51,6 +236,14 @@ static unsigned long get_random_long(unsigned long entropy,
> > > use_i8254 = false;
> > > }
> > >
> > > + if (efi_rng_supported(sys_table)) {
> > > + efi_printk(sys_table, " EFI_RNG");
> > > + raw = efi_get_rng(sys_table);
> > > + if (raw)
> > > + random ^= raw;
> > > + use_i8254 = false;
> > > + }
> > > +
> > > if (use_i8254) {
> > > efi_printk(sys_table, " i8254");
> > > random ^= i8254();
> >
> > Have you looked at the tradeoff in terms of boot time for building a key
> > array in 'unsigned long' chunks as opposed to passing the array and size
> > directly for the RNG protocol?
> >
>
> I didn't really measure the speed, but directly passing array and size to
> RNG protocol should a bit faster than calling the protocol a could of times.
>
> But, the key generation process only in first time building or trigger by
> user raises the rebuild flag. So, it doesn't affect to every booting time.
>
> Due to I want let the whole key array more random, so each unsigned long
> chunk was mixed(xor) by following entropy:
> + random long from RDRAND
> + RDTSC
> + random long from EFI RNG protocol
> + last unsigned long chunk
>
> Another reason is voiding the result of EFI RNG protocol to get weight
^^^^^^^^^ avoiding
Sorry for my typo!
> higher than other source, in case too trust EFI RNG.
Joey Lee
On Thu, Jul 30, 2015 at 05:20:46PM +0100, Matt Fleming wrote:
> On Thu, 2015-07-16 at 22:25 +0800, 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.
> >
> > For generating a messy number as a 20 bytes key, the codes in EFI
> > stub gets u32 random number five time and every random number is
> > rolling that last u32 random number as entropy.
> >
> > The HMAC key stored in EFI boot service variable, the GUID is
> > S4SignKey-fe141863-c070-478e-b8a3-878a5dc9ef21.
>
> [...]
>
> > @@ -1383,6 +1384,60 @@ free_mem_map:
> > return status;
> > }
> >
> > +#ifdef CONFIG_HIBERNATE_VERIFICATION
> > +#define SWSUSP_KEY \
> > + ((efi_char16_t [10]) { 'S', 'W', 'S', 'U', 'S', 'P', 'K', 'e', 'y', 0 })
> > +#define SWSUSP_KEY_ATTRIBUTE (EFI_VARIABLE_NON_VOLATILE | \
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS)
>
> You mean "SWSUSPKey" not "S4SignKey" right?
>
Oh! it's my fault. In 2013 PKI edition, there havse sign key and verify key.
The HMAC edition has only one SWSUSPKey.
I will change the variable name in patch description.
Thanks a lot!
Joey Lee
On Thu, Jul 30, 2015 at 05:23:22PM +0100, Matt Fleming wrote:
> On Thu, 2015-07-16 at 22:25 +0800, Lee, Chun-Yi wrote:
> > Moved the function of transferring EFI status to kernel error for
> > later used by EFI stub.
> >
> > Signed-off-by: Lee, Chun-Yi <[email protected]>
> > ---
> > drivers/firmware/efi/vars.c | 33 ---------------------------------
> > include/linux/efi.h | 33 +++++++++++++++++++++++++++++++++
> > 2 files changed, 33 insertions(+), 33 deletions(-)
>
> The patch contents are fine but the title could do with some work,
> "public" isn't a verb. I think "Make efi_status_to_err() public" would
> be fine.
>
Thanks for your correcting, I will change the patch title.
Joey Lee
On Thu, Jul 30, 2015 at 05:30:09PM +0100, Matt Fleming wrote:
> On Thu, 2015-07-16 at 22:25 +0800, Lee, Chun-Yi wrote:
> > For forwarding swsusp key from EFI stub to boot kernel, this patch
> > allocates setup data for carrying swsusp key, size and the status
> > of efi operating.
> >
> > Signed-off-by: Lee, Chun-Yi <[email protected]>
> > ---
> > arch/x86/boot/compressed/eboot.c | 29 +++++++++++++++++++++++++----
> > arch/x86/include/uapi/asm/bootparam.h | 1 +
> > 2 files changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index 97b356f..5e1476e 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -1392,19 +1392,23 @@ free_mem_map:
> >
> > static void setup_swsusp_keys(struct boot_params *params)
> > {
> > - unsigned long key_size, attributes;
> > + struct setup_data *setup_data, *swsusp_setup_data;
> > struct swsusp_keys *swsusp_keys;
> > + int size = 0;
> > + unsigned long key_size, attributes;
> > efi_status_t status;
>
> Why the local variable shuffling? It'd be better to leave key_size and
> attributes alone.
>
> Also, 'size' doesn't look like it should be int. If you're passing it to
> allocate_pool it really wants to be 'unsigned long'.
>
I see, I will change that.
> > /* Allocate setup_data to carry keys */
> > + size = sizeof(struct setup_data) + sizeof(struct swsusp_keys);
> > status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> > - sizeof(struct swsusp_keys), &swsusp_keys);
> > + size, &swsusp_setup_data);
> > if (status != EFI_SUCCESS) {
> > efi_printk(sys_table, "Failed to alloc mem for swsusp_keys\n");
> > return;
> > }
> >
> > - memset(swsusp_keys, 0, sizeof(struct swsusp_keys));
> > + memset(swsusp_setup_data, 0, size);
> > + swsusp_keys = (struct swsusp_keys *)swsusp_setup_data->data;
> >
> > status = efi_call_early(get_variable, SWSUSP_KEY, &EFI_SWSUSP_GUID,
> > &attributes, &key_size, swsusp_keys->swsusp_key);
> > @@ -1413,7 +1417,9 @@ static void setup_swsusp_keys(struct boot_params *params)
> > memset(swsusp_keys->swsusp_key, 0, SWSUSP_DIGEST_SIZE);
> > status = efi_call_early(set_variable, SWSUSP_KEY,
> > &EFI_SWSUSP_GUID, attributes, 0, NULL);
> > - if (status == EFI_SUCCESS) {
> > + if (status)
> > + goto clean_fail;
>
> Please use the EFI status codes explicitly.
>
Here also, I will check status explicitly.
Thanks a lot!
Joey Lee
On Tue, Jul 28, 2015 at 02:35:23PM +0200, Pavel Machek wrote:
> Typo in patch subject.
>
> > And for earsing footbprints, the codes in this patch remove setup
>
> And two typos here.
>
Sorry for subject and above typos, I will fix it.
Thanks.
> > data that carried swsusp key, and clean the memory space that
>
> And don't call it swsusp. Please fix globally.
>
OK~
> > +++ b/arch/x86/power/hibernate_keys.c
> > @@ -0,0 +1,79 @@
> > +/* Swsusp keys handler
> > + *
> > + * Copyright (C) 2015 SUSE Linux Products GmbH. All rights
>
> Are you sure?
>
Thank for your finding here, I will change the Copyright to me with my
company mail address.
> > +static int __init init_hibernate_keys(void)
> > +{
> > + struct swsusp_keys *keys;
> > + int ret = 0;
> > +
> > + if (!keys_phys_addr)
> > + return -ENODEV;
> > +
> > + keys = early_memremap(keys_phys_addr, sizeof(struct swsusp_keys));
> > +
> > + /* Copy swsusp keys to a allocated page */
> > + swsusp_keys = (struct swsusp_keys *)get_zeroed_page(GFP_KERNEL);
> > + if (swsusp_keys) {
> > + *swsusp_keys = *keys;
> > + } else {
> > + pr_err("PM: Allocate swsusp keys page failed\n");
> > + ret = -ENOMEM;
> > + }
> > +
> > + /* Earse keys data no matter copy success or failed */
> > + memset(keys, 0, sizeof(struct swsusp_keys));
> > + early_memunmap(keys, sizeof(struct swsusp_keys));
> > + memblock_free(keys_phys_addr, sizeof(struct swsusp_keys));
> > + keys_phys_addr = 0;
> > +
> > + return ret;
> > +}
> > +
> > +late_initcall(init_hibernate_keys);
>
> init_hibernation_keys.
>
>
> Pavel
I will change the function name.
Thanks a lot!
Joey Lee
On Fri, Jul 31, 2015 at 02:49:36PM +0200, Pavel Machek wrote:
> On Fri 2015-07-31 18:08:12, joeyli wrote:
> > On Tue, Jul 28, 2015 at 02:01:56PM +0200, Pavel Machek wrote:
> > > On Thu 2015-07-16 22:25:15, Lee, Chun-Yi wrote:
> > > > 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, add HIBERNATE_VERIFICATION kernel config.
> > > >
> > > > Signed-off-by: Lee, Chun-Yi <[email protected]>
> > > > ---
> > > > include/linux/suspend.h | 5 +++++
> > > > kernel/power/Kconfig | 13 +++++++++++++
> > > > kernel/power/power.h | 1 +
> > > > 3 files changed, 19 insertions(+)
> > > >
> > > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> > > > index 5efe743..6cd2a48 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 SWSUSP_HMAC "hmac(sha1)"
> > > > +#define SWSUSP_DIGEST_SIZE 20
> > >
> > > I'd replace SWSUSP with HIBERNATION here, and pretty much everywhere.
> > >
> >
> > SWSUSP is shorter than HIBERNATION, and there have some codes in hibernate
> > are also using swsusp. I still want to use it.
>
> Yes, its shorter, but its old name we are trying to move away
> from. Please do the same.
> Pavel
OK~ I will rename.
Thanks
Joey Lee
On Fri, Jul 31, 2015 at 01:01:18PM +0100, Matt Fleming wrote:
> On Fri, 2015-07-31 at 17:58 +0800, joeyli wrote:
> > >
> > > Can you do something to avoid each function having two very similar
> > > versions of these functions?
> > >
> >
> > They are similar but I want follow the style in eboot.c.
> > On the other hand, it's earlier to locate problem on 32-bit or 64-bit EFI.
> >
> > So, I will keep the above codes.
>
> FWIW, I think that's fine. I would happily accept a patch to cleanup the
> duplication, but I don't think that needs to be a prerequisite for this
> support.
>
> I've no problem with the duplication right now.
>
Thanks
> > >
> > > > --- 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 don't typedef structs usually...
> > >
> > > Make it union so you can have just one
> > >
> >
> > I want to follow the style as efi_pci_io_protocolxxx in efi.h.
> > So I will keep it.
>
> Yeah, consistency is better here than sticking with the general Linux
> coding style rules.
>
> > > > + switch (status) {
> > > > + case EFI_SUCCESS:
> > > > + str = "EFI_SUCCESS";
> > > > + break;
> > >
> > > Can you use macros to reduce code duplication here?
> > > Pavel
> > I will try to reduce duplicate code.
>
> Take a look at __stringify().
>
Thanks for suggestion, I will look at it.
Joey Lee
On Thu, 30 Jul 2015 17:23:22 +0100, Matt Fleming said:
> On Thu, 2015-07-16 at 22:25 +0800, Lee, Chun-Yi wrote:
> > Moved the function of transferring EFI status to kernel error for
> > later used by EFI stub.
> >
> > Signed-off-by: Lee, Chun-Yi <[email protected]>
> > ---
> > drivers/firmware/efi/vars.c | 33 ---------------------------------
> > include/linux/efi.h | 33 +++++++++++++++++++++++++++++++++
> > 2 files changed, 33 insertions(+), 33 deletions(-)
>
> The patch contents are fine but the title could do with some work,
> "public" isn't a verb.
In English, any noun can be verbed. :)