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:

quoted

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 <jlee@suse.com>
---
 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.

quoted

 	/* 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