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

On Thu, Aug 20, 2015 at 09:40:44PM +0100, Matt Fleming wrote:

On Tue, 11 Aug, at 02:16:26PM, Lee, Chun-Yi wrote:

quoted

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

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

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

In description, I want to explain why the length of key is 20 bytes.
I will try to explain more clear, if it still confuses reviewer then
I will remove the description.
 

quoted

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

 
I'd really like to see some of the explanation from your cover letter
included in the commit message for this patch, and in particular why
signing hibernate images is a good thing.

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

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

quoted

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

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 0ffb6db..463aa9b 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c

@@ -12,6 +12,7 @@
 #include <asm/efi.h>
 #include <asm/setup.h>
 #include <asm/desc.h>
+#include <asm/suspend.h>
 
 #include "../string.h"
 #include "eboot.h"

@@ -1383,6 +1384,63 @@ free_mem_map:
 	return status;
 }
 
+#ifdef CONFIG_HIBERNATE_VERIFICATION
+#define HIBERNATION_KEY \
+	((efi_char16_t [15]) { 'H', 'I', 'B', 'E', 'R', 'N', 'A', 'T', 'I', 'O', 'N', 'K', 'e', 'y', 0 })
+#define HIBERNATION_KEY_ATTRIBUTE	(EFI_VARIABLE_NON_VOLATILE | \
+					EFI_VARIABLE_BOOTSERVICE_ACCESS)
+
+static void setup_hibernation_keys(struct boot_params *params)
+{
+	unsigned long key_size;
+	unsigned long attributes;
+	struct hibernation_keys *keys;
+	efi_status_t status;
+
+	/* Allocate setup_data to carry keys */
+	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+				sizeof(struct hibernation_keys), &keys);
+	if (status != EFI_SUCCESS) {
+		efi_printk(sys_table, "Failed to alloc mem for hibernation keys\n");
+		return;
+	}
+
+	memset(keys, 0, sizeof(struct hibernation_keys));
+
+	status = efi_call_early(get_variable, HIBERNATION_KEY,
+				&EFI_HIBERNATION_GUID, &attributes,
+				&key_size, keys->hibernation_key);

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

OK, I will add new line here.
 

quoted

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

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

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

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

quoted

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

You're leaking 'keys' here.

Oh, you are right, I will free keys here.
 

quoted

diff --git a/arch/x86/include/asm/suspend.h b/arch/x86/include/asm/suspend.h
index 2fab6c2..ab463c4 100644
--- a/arch/x86/include/asm/suspend.h
+++ b/arch/x86/include/asm/suspend.h

@@ -3,3 +3,12 @@
 #else
 # include <asm/suspend_64.h>
 #endif
+
+#ifdef CONFIG_HIBERNATE_VERIFICATION
+#include <linux/suspend.h>
+
+struct hibernation_keys {
+	unsigned long hkey_status;
+	u8 hibernation_key[HIBERNATION_DIGEST_SIZE];
+};
+#endif

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

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

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

-- 
Matt Fleming, Intel Open Source Technology Center

Thanks a lot!
Joey Lee