[GIT PULL 0/6] EFI urgent fixes

[GIT PULL 0/6] EFI urgent fixes

[Matt Fleming]

Folks, please pull the following urgent fixes, which prevent an
infinite loop on Xen, a boot failure on Qualcomm platforms and a data
type bug in the fdt code.

The following changes since commit 694d0d0bb2030d2e36df73e2d23d5770511dbc8d:

  Linux 4.8-rc2 (2016-08-14 19:11:36 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git tags/efi-urgent

for you to fetch changes up to b7453c7a235c597b44c365887e57e7c300e72bf0:

  efi/fdt: Fix handling error value in fdt_find_uefi_params (2016-08-19 21:18:34 +0100)

----------------------------------------------------------------
 * Make for_each_efi_memory_desc_in_map() safe on Xen and prevent an
   infinte loop - Jan Beulich

 * Fix boot error on arm64 Qualcomm platforms by refactoring and
   improving the ExitBootServices() hack we already for x86 and moving
   it to the libstub - Jeffrey Hugo

 * Use correct return data type for of_get_flat_dt_subnode_by_name()
   so that we correctly handle errors - Andrzej Hajda

----------------------------------------------------------------
Andrzej Hajda (1):
      efi/fdt: Fix handling error value in fdt_find_uefi_params

Jan Beulich (1):
      efi: Make for_each_efi_memory_desc_in_map() cope with running on Xen

Jeffrey Hugo (4):
      efi/libstub: Allocate headspace in efi_get_memory_map()
      efi/libstub: Introduce ExitBootServices helper
      efi/libstub: Use efi_exit_boot_services() in FDT
      x86/efi: Use efi_exit_boot_services()

 arch/x86/boot/compressed/eboot.c               | 134 ++++++++++----------
 drivers/firmware/efi/efi.c                     |   7 +-
 drivers/firmware/efi/libstub/efi-stub-helper.c | 168 ++++++++++++++++++++-----
 drivers/firmware/efi/libstub/fdt.c             |  54 +++++---
 drivers/firmware/efi/libstub/random.c          |  12 +-
 include/linux/efi.h                            |  28 ++++-
 6 files changed, 283 insertions(+), 120 deletions(-)

[PATCH 1/6] efi: Make for_each_efi_memory_desc_in_map() cope with running on Xen

[Matt Fleming]

From: Jan Beulich <JBeulich@suse.com>

While commit 55f1ea15216 ("efi: Fix for_each_efi_memory_desc_in_map()
for empty memmaps") made an attempt to deal with empty memory maps, it
didn't address the case where the map field never gets set, as is
apparently the case when running under Xen.

Reported-by: <lists@ssl-mail.com>
Tested-by: <lists@ssl-mail.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: <stable@vger.kernel.org> # v4.7+
Signed-off-by: Jan Beulich <jbeulich@suse.com>
[ Guard the loop with a NULL check instead of pointer underflow ]
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 include/linux/efi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 7f5a58225385..23cd3ced8c1a 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -946,7 +946,7 @@ extern int efi_memattr_apply_permissions(struct mm_struct *mm,
 /* Iterate through an efi_memory_map */
 #define for_each_efi_memory_desc_in_map(m, md)				   \
 	for ((md) = (m)->map;						   \
-	     ((void *)(md) + (m)->desc_size) <= (m)->map_end;		   \
+	     (md) && ((void *)(md) + (m)->desc_size) <= (m)->map_end;	   \
 	     (md) = (void *)(md) + (m)->desc_size)
 
 /**
-- 
2.7.3

[PATCH 2/6] efi/libstub: Allocate headspace in efi_get_memory_map()

[Matt Fleming]

From: Jeffrey Hugo <jhugo@codeaurora.org>

efi_get_memory_map() allocates a buffer to store the memory map that it
retrieves.  This buffer may need to be reused by the client after
ExitBootServices() is called, at which point allocations are not longer
permitted.  To support this usecase, provide the allocated buffer size back
to the client, and allocate some additional headroom to account for any
reasonable growth in the map that is likely to happen between the call to
efi_get_memory_map() and the client reusing the buffer.

Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 arch/x86/boot/compressed/eboot.c               | 12 +++-
 drivers/firmware/efi/libstub/efi-stub-helper.c | 96 ++++++++++++++++++--------
 drivers/firmware/efi/libstub/fdt.c             | 17 +++--
 drivers/firmware/efi/libstub/random.c          | 12 +++-
 include/linux/efi.h                            | 15 ++--
 5 files changed, 107 insertions(+), 45 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index ff574dad95cc..60348c73bed4 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1008,7 +1008,7 @@ static efi_status_t exit_boot(struct boot_params *boot_params,
 			      void *handle, bool is64)
 {
 	struct efi_info *efi = &boot_params->efi_info;
-	unsigned long map_sz, key, desc_size;
+	unsigned long map_sz, key, desc_size, buff_size;
 	efi_memory_desc_t *mem_map;
 	struct setup_data *e820ext;
 	const char *signature;
@@ -1019,14 +1019,20 @@ static efi_status_t exit_boot(struct boot_params *boot_params,
 	bool called_exit = false;
 	u8 nr_entries;
 	int i;
+	efi_boottime_memory_map_t map;
 
 	nr_desc = 0;
 	e820ext = NULL;
 	e820ext_size = 0;
+	map.map = &mem_map;
+	map.map_size = &map_sz;
+	map.desc_size = &desc_size;
+	map.desc_ver = &desc_version;
+	map.key_ptr = &key;
+	map.buff_size = &buff_size;
 
 get_map:
-	status = efi_get_memory_map(sys_table, &mem_map, &map_sz, &desc_size,
-				    &desc_version, &key);
+	status = efi_get_memory_map(sys_table, &map);
 
 	if (status != EFI_SUCCESS)
 		return status;
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 3bd127f95315..bbb5166e8fd0 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -41,6 +41,8 @@ static unsigned long __chunk_size = EFI_READ_CHUNK_SIZE;
 #define EFI_ALLOC_ALIGN		EFI_PAGE_SIZE
 #endif
 
+#define EFI_MMAP_NR_SLACK_SLOTS	8
+
 struct file_info {
 	efi_file_handle_t *handle;
 	u64 size;
@@ -63,49 +65,62 @@ void efi_printk(efi_system_table_t *sys_table_arg, char *str)
 	}
 }
 
+static inline bool mmap_has_headroom(unsigned long buff_size,
+				     unsigned long map_size,
+				     unsigned long desc_size)
+{
+	unsigned long slack = buff_size - map_size;
+
+	return slack / desc_size >= EFI_MMAP_NR_SLACK_SLOTS;
+}
+
 efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
-				efi_memory_desc_t **map,
-				unsigned long *map_size,
-				unsigned long *desc_size,
-				u32 *desc_ver,
-				unsigned long *key_ptr)
+				efi_boottime_memory_map_t *map)
 {
 	efi_memory_desc_t *m = NULL;
 	efi_status_t status;
 	unsigned long key;
 	u32 desc_version;
 
-	*map_size = sizeof(*m) * 32;
+	*map->desc_size = sizeof(*m);
+	*map->map_size = *map->desc_size * 32;
+	*map->buff_size = *map->map_size;
 again:
-	/*
-	 * Add an additional efi_memory_desc_t because we're doing an
-	 * allocation which may be in a new descriptor region.
-	 */
-	*map_size += sizeof(*m);
 	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
-				*map_size, (void **)&m);
+				*map->map_size, (void **)&m);
 	if (status != EFI_SUCCESS)
 		goto fail;
 
-	*desc_size = 0;
+	*map->desc_size = 0;
 	key = 0;
-	status = efi_call_early(get_memory_map, map_size, m,
-				&key, desc_size, &desc_version);
-	if (status == EFI_BUFFER_TOO_SMALL) {
+	status = efi_call_early(get_memory_map, map->map_size, m,
+				&key, map->desc_size, &desc_version);
+	if (status == EFI_BUFFER_TOO_SMALL ||
+	    !mmap_has_headroom(*map->buff_size, *map->map_size,
+			       *map->desc_size)) {
 		efi_call_early(free_pool, m);
+		/*
+		 * Make sure there is some entries of headroom so that the
+		 * buffer can be reused for a new map after allocations are
+		 * no longer permitted.  Its unlikely that the map will grow to
+		 * exceed this headroom once we are ready to trigger
+		 * ExitBootServices()
+		 */
+		*map->map_size += *map->desc_size * EFI_MMAP_NR_SLACK_SLOTS;
+		*map->buff_size = *map->map_size;
 		goto again;
 	}
 
 	if (status != EFI_SUCCESS)
 		efi_call_early(free_pool, m);
 
-	if (key_ptr && status == EFI_SUCCESS)
-		*key_ptr = key;
-	if (desc_ver && status == EFI_SUCCESS)
-		*desc_ver = desc_version;
+	if (map->key_ptr && status == EFI_SUCCESS)
+		*map->key_ptr = key;
+	if (map->desc_ver && status == EFI_SUCCESS)
+		*map->desc_ver = desc_version;
 
 fail:
-	*map = m;
+	*map->map = m;
 	return status;
 }
 
@@ -113,13 +128,20 @@ fail:
 unsigned long get_dram_base(efi_system_table_t *sys_table_arg)
 {
 	efi_status_t status;
-	unsigned long map_size;
+	unsigned long map_size, buff_size;
 	unsigned long membase  = EFI_ERROR;
 	struct efi_memory_map map;
 	efi_memory_desc_t *md;
+	efi_boottime_memory_map_t boot_map;
 
-	status = efi_get_memory_map(sys_table_arg, (efi_memory_desc_t **)&map.map,
-				    &map_size, &map.desc_size, NULL, NULL);
+	boot_map.map = (efi_memory_desc_t **)&map.map;
+	boot_map.map_size = &map_size;
+	boot_map.desc_size = &map.desc_size;
+	boot_map.desc_ver = NULL;
+	boot_map.key_ptr = NULL;
+	boot_map.buff_size = &buff_size;
+
+	status = efi_get_memory_map(sys_table_arg, &boot_map);
 	if (status != EFI_SUCCESS)
 		return membase;
 
@@ -144,15 +166,22 @@ efi_status_t efi_high_alloc(efi_system_table_t *sys_table_arg,
 			    unsigned long size, unsigned long align,
 			    unsigned long *addr, unsigned long max)
 {
-	unsigned long map_size, desc_size;
+	unsigned long map_size, desc_size, buff_size;
 	efi_memory_desc_t *map;
 	efi_status_t status;
 	unsigned long nr_pages;
 	u64 max_addr = 0;
 	int i;
+	efi_boottime_memory_map_t boot_map;
+
+	boot_map.map = &map;
+	boot_map.map_size = &map_size;
+	boot_map.desc_size = &desc_size;
+	boot_map.desc_ver = NULL;
+	boot_map.key_ptr = NULL;
+	boot_map.buff_size = &buff_size;
 
-	status = efi_get_memory_map(sys_table_arg, &map, &map_size, &desc_size,
-				    NULL, NULL);
+	status = efi_get_memory_map(sys_table_arg, &boot_map);
 	if (status != EFI_SUCCESS)
 		goto fail;
 
@@ -230,14 +259,21 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 			   unsigned long size, unsigned long align,
 			   unsigned long *addr)
 {
-	unsigned long map_size, desc_size;
+	unsigned long map_size, desc_size, buff_size;
 	efi_memory_desc_t *map;
 	efi_status_t status;
 	unsigned long nr_pages;
 	int i;
+	efi_boottime_memory_map_t boot_map;
+
+	boot_map.map = &map;
+	boot_map.map_size = &map_size;
+	boot_map.desc_size = &desc_size;
+	boot_map.desc_ver = NULL;
+	boot_map.key_ptr = NULL;
+	boot_map.buff_size = &buff_size;
 
-	status = efi_get_memory_map(sys_table_arg, &map, &map_size, &desc_size,
-				    NULL, NULL);
+	status = efi_get_memory_map(sys_table_arg, &boot_map);
 	if (status != EFI_SUCCESS)
 		goto fail;
 
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index e58abfa953cc..e94846d498fb 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -175,13 +175,21 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 					    unsigned long fdt_addr,
 					    unsigned long fdt_size)
 {
-	unsigned long map_size, desc_size;
+	unsigned long map_size, desc_size, buff_size;
 	u32 desc_ver;
 	unsigned long mmap_key;
 	efi_memory_desc_t *memory_map, *runtime_map;
 	unsigned long new_fdt_size;
 	efi_status_t status;
 	int runtime_entry_count = 0;
+	efi_boottime_memory_map_t map;
+
+	map.map = &runtime_map;
+	map.map_size = &map_size;
+	map.desc_size = &desc_size;
+	map.desc_ver = &desc_ver;
+	map.key_ptr = &mmap_key;
+	map.buff_size = &buff_size;
 
 	/*
 	 * Get a copy of the current memory map that we will use to prepare
@@ -189,8 +197,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	 * subsequent allocations adding entries, since they could not affect
 	 * the number of EFI_MEMORY_RUNTIME regions.
 	 */
-	status = efi_get_memory_map(sys_table, &runtime_map, &map_size,
-				    &desc_size, &desc_ver, &mmap_key);
+	status = efi_get_memory_map(sys_table, &map);
 	if (status != EFI_SUCCESS) {
 		pr_efi_err(sys_table, "Unable to retrieve UEFI memory map.\n");
 		return status;
@@ -199,6 +206,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	pr_efi(sys_table,
 	       "Exiting boot services and installing virtual address map...\n");
 
+	map.map = &memory_map;
 	/*
 	 * Estimate size of new FDT, and allocate memory for it. We
 	 * will allocate a bigger buffer if this ends up being too
@@ -218,8 +226,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 		 * we can get the memory map key  needed for
 		 * exit_boot_services().
 		 */
-		status = efi_get_memory_map(sys_table, &memory_map, &map_size,
-					    &desc_size, &desc_ver, &mmap_key);
+		status = efi_get_memory_map(sys_table, &map);
 		if (status != EFI_SUCCESS)
 			goto fail_free_new_fdt;
 
diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
index 53f6d3fe6d86..46c14ee99db7 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -73,12 +73,20 @@ efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg,
 			      unsigned long random_seed)
 {
 	unsigned long map_size, desc_size, total_slots = 0, target_slot;
+	unsigned long buff_size;
 	efi_status_t status;
 	efi_memory_desc_t *memory_map;
 	int map_offset;
+	efi_boottime_memory_map_t map;
 
-	status = efi_get_memory_map(sys_table_arg, &memory_map, &map_size,
-				    &desc_size, NULL, NULL);
+	map.map = &memory_map;
+	map.map_size = &map_size;
+	map.desc_size = &desc_size;
+	map.desc_ver = NULL;
+	map.key_ptr = NULL;
+	map.buff_size = &buff_size;
+
+	status = efi_get_memory_map(sys_table_arg, &map);
 	if (status != EFI_SUCCESS)
 		return status;
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 23cd3ced8c1a..c468010ac245 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -118,6 +118,15 @@ typedef struct {
 	u32 imagesize;
 } efi_capsule_header_t;
 
+typedef struct {
+	efi_memory_desc_t **map;
+	unsigned long *map_size;
+	unsigned long *desc_size;
+	u32 *desc_ver;
+	unsigned long *key_ptr;
+	unsigned long *buff_size;
+} efi_boottime_memory_map_t;
+
 /*
  * EFI capsule flags
  */
@@ -1371,11 +1380,7 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
 			  efi_loaded_image_t *image, int *cmd_line_len);
 
 efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
-				efi_memory_desc_t **map,
-				unsigned long *map_size,
-				unsigned long *desc_size,
-				u32 *desc_ver,
-				unsigned long *key_ptr);
+				efi_boottime_memory_map_t *map);
 
 efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 			   unsigned long size, unsigned long align,
-- 
2.7.3

[PATCH 3/6] efi/libstub: Introduce ExitBootServices helper

[Matt Fleming]

From: Jeffrey Hugo <jhugo@codeaurora.org>

The spec allows ExitBootServices to fail with EFI_INVALID_PARAMETER if a
race condition has occurred where the EFI has updated the memory map after
the stub grabbed a reference to the map.  The spec defines a retry
proceedure with specific requirements to handle this scenario.

This scenario was previously observed on x86 - commit d3768d885c6c ("x86,
efi: retry ExitBootServices() on failure") but the current fix is not spec
compliant and the scenario is now observed on the Qualcomm Technologies
QDF2432 via the FDT stub which does not handle the error and thus causes
boot failures.

Add a helper to the stub library that correctly adhears to the spec in the
case of EFI_INVALID_PARAMETER from ExitBootServices and can be universally
used across all stub implementations.

Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/libstub/efi-stub-helper.c | 72 ++++++++++++++++++++++++++
 include/linux/efi.h                            | 11 ++++
 2 files changed, 83 insertions(+)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index bbb5166e8fd0..ca0a4f9ac1e5 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -740,3 +740,75 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
 	*cmd_line_len = options_bytes;
 	return (char *)cmdline_addr;
 }
+
+/*
+ * Handle calling ExitBootServices according to the requirements set out by the
+ * spec.  Obtains the current memory map, and returns that info after calling
+ * ExitBootServices.  The client must specify a function to perform any
+ * processing of the memory map data prior to ExitBootServices.  A client
+ * specific structure may be passed to the function via priv.  The client
+ * function may be called multiple times.
+ */
+efi_status_t efi_exit_boot_services(efi_system_table_t *sys_table_arg,
+				    void *handle,
+				    efi_boottime_memory_map_t *map,
+				    void *priv,
+				    efi_exit_boot_map_processing priv_func)
+{
+	efi_status_t status;
+
+	status = efi_get_memory_map(sys_table_arg, map);
+
+	if (status != EFI_SUCCESS)
+		goto fail;
+
+	status = priv_func(sys_table_arg, map, priv);
+	if (status != EFI_SUCCESS)
+		goto free_map;
+
+	status = efi_call_early(exit_boot_services, handle, *map->key_ptr);
+
+	if (status == EFI_INVALID_PARAMETER) {
+		/*
+		 * The memory map changed between efi_get_memory_map() and
+		 * exit_boot_services().  Per the spec we need to get the
+		 * updated map, and try again.  The spec implies one retry
+		 * should be sufficent, which is confirmed against the EDK2
+		 * implementation.  Per the spec, we can only invoke
+		 * get_memory_map() and exit_boot_services() - we cannot alloc
+		 * so efi_get_memory_map() cannot be used, and we must reuse
+		 * the buffer.  For all practical purposes, the headroom in the
+		 * buffer should account for any changes in the map so the call
+		 * to get_memory_map() is expected to succeed here.
+		 */
+		*map->map_size = *map->buff_size;
+		status = efi_call_early(get_memory_map,
+					map->map_size,
+					*map->map,
+					map->key_ptr,
+					map->desc_size,
+					map->desc_ver);
+		if (status != EFI_SUCCESS)
+			/* exit_boot_services() was called, thus cannot free*/
+			goto fail;
+
+		status = priv_func(sys_table_arg, map, priv);
+		if (status != EFI_SUCCESS)
+			/* exit_boot_services() was called, thus cannot free*/
+			goto fail;
+
+		status = efi_call_early(exit_boot_services, handle,
+					*map->key_ptr);
+	}
+
+	if (status != EFI_SUCCESS)
+		/* exit_boot_services() was called, thus cannot free*/
+		goto fail;
+
+	return EFI_SUCCESS;
+
+free_map:
+	efi_call_early(free_pool, *map->map);
+fail:
+	return status;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c468010ac245..1f39dd7c600d 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1462,4 +1462,15 @@ extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
 	arch_efi_call_virt_teardown();					\
 })
 
+typedef efi_status_t (*efi_exit_boot_map_processing)(
+	efi_system_table_t *sys_table_arg,
+	efi_boottime_memory_map_t *map,
+	void *priv);
+
+efi_status_t efi_exit_boot_services(efi_system_table_t *sys_table,
+				    void *handle,
+				    efi_boottime_memory_map_t *map,
+				    void *priv,
+				    efi_exit_boot_map_processing priv_func);
+
 #endif /* _LINUX_EFI_H */
-- 
2.7.3

[PATCH 4/6] efi/libstub: Use efi_exit_boot_services() in FDT

[Matt Fleming]

From: Jeffrey Hugo <jhugo@codeaurora.org>

The FDT code directly calls ExitBootServices.  This is inadvisable as the
UEFI spec details a complex set of errors, race conditions, and API
interactions that the caller of ExitBootServices must get correct.  The
FDT code does not handle EFI_INVALID_PARAMETER as required by the spec,
which causes intermittent boot failures on the Qualcomm Technologies
QDF2432.  The efi_exit_boot_services() helper handles the
EFI_INVALID_PARAMETER scenario.

Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/libstub/fdt.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index e94846d498fb..b66dc4021efa 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -152,6 +152,27 @@ fdt_set_fail:
 #define EFI_FDT_ALIGN EFI_PAGE_SIZE
 #endif
 
+struct exit_boot_struct {
+	efi_memory_desc_t *runtime_map;
+	int *runtime_entry_count;
+};
+
+static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
+			    efi_boottime_memory_map_t *map,
+			    void *priv)
+{
+	struct exit_boot_struct *p = priv;
+	/*
+	 * Update the memory map with virtual addresses. The function will also
+	 * populate @runtime_map with copies of just the EFI_MEMORY_RUNTIME
+	 * entries so that we can pass it straight into SetVirtualAddressMap()
+	 */
+	efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
+			p->runtime_map, p->runtime_entry_count);
+
+	return EFI_SUCCESS;
+}
+
 /*
  * Allocate memory for a new FDT, then add EFI, commandline, and
  * initrd related fields to the FDT.  This routine increases the
@@ -183,6 +204,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	efi_status_t status;
 	int runtime_entry_count = 0;
 	efi_boottime_memory_map_t map;
+	struct exit_boot_struct priv;
 
 	map.map = &runtime_map;
 	map.map_size = &map_size;
@@ -257,16 +279,11 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 		}
 	}
 
-	/*
-	 * Update the memory map with virtual addresses. The function will also
-	 * populate @runtime_map with copies of just the EFI_MEMORY_RUNTIME
-	 * entries so that we can pass it straight into SetVirtualAddressMap()
-	 */
-	efi_get_virtmap(memory_map, map_size, desc_size, runtime_map,
-			&runtime_entry_count);
-
-	/* Now we are ready to exit_boot_services.*/
-	status = sys_table->boottime->exit_boot_services(handle, mmap_key);
+	sys_table->boottime->free_pool(memory_map);
+	priv.runtime_map = runtime_map;
+	priv.runtime_entry_count = &runtime_entry_count;
+	status = efi_exit_boot_services(sys_table, handle, &map, &priv,
+					exit_boot_func);
 
 	if (status == EFI_SUCCESS) {
 		efi_set_virtual_address_map_t *svam;
-- 
2.7.3

[PATCH 5/6] x86/efi: Use efi_exit_boot_services()

[Matt Fleming]

From: Jeffrey Hugo <jhugo@codeaurora.org>

The eboot code directly calls ExitBootServices.  This is inadvisable as the
UEFI spec details a complex set of errors, race conditions, and API
interactions that the caller of ExitBootServices must get correct.  The
eboot code attempts allocations after calling ExitBootSerives which is
not permitted per the spec.  The efi_exit_boot_services() helper handles
this scenario.

Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 arch/x86/boot/compressed/eboot.c | 122 +++++++++++++++++++--------------------
 1 file changed, 60 insertions(+), 62 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 60348c73bed4..54b5d5a9cbe5 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1004,85 +1004,87 @@ static efi_status_t alloc_e820ext(u32 nr_desc, struct setup_data **e820ext,
 	return status;
 }
 
+struct exit_boot_struct {
+	struct boot_params *boot_params;
+	struct efi_info *efi;
+	struct setup_data *e820ext;
+	__u32 e820ext_size;
+	bool is64;
+};
+
+static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
+				   efi_boottime_memory_map_t *map,
+				   void *priv)
+{
+	static bool first = true;
+	const char *signature;
+	__u32 nr_desc;
+	efi_status_t status;
+	struct exit_boot_struct *p = priv;
+
+	if (first) {
+		nr_desc = *map->buff_size / *map->desc_size;
+		if (nr_desc > ARRAY_SIZE(p->boot_params->e820_map)) {
+			u32 nr_e820ext = nr_desc -
+					ARRAY_SIZE(p->boot_params->e820_map);
+
+			status = alloc_e820ext(nr_e820ext, &p->e820ext,
+					       &p->e820ext_size);
+			if (status != EFI_SUCCESS)
+				return status;
+		}
+		first = false;
+	}
+
+	signature = p->is64 ? EFI64_LOADER_SIGNATURE : EFI32_LOADER_SIGNATURE;
+	memcpy(&p->efi->efi_loader_signature, signature, sizeof(__u32));
+
+	p->efi->efi_systab = (unsigned long)sys_table_arg;
+	p->efi->efi_memdesc_size = *map->desc_size;
+	p->efi->efi_memdesc_version = *map->desc_ver;
+	p->efi->efi_memmap = (unsigned long)*map->map;
+	p->efi->efi_memmap_size = *map->map_size;
+
+#ifdef CONFIG_X86_64
+	p->efi->efi_systab_hi = (unsigned long)sys_table_arg >> 32;
+	p->efi->efi_memmap_hi = (unsigned long)*map->map >> 32;
+#endif
+
+	return EFI_SUCCESS;
+}
+
 static efi_status_t exit_boot(struct boot_params *boot_params,
 			      void *handle, bool is64)
 {
-	struct efi_info *efi = &boot_params->efi_info;
 	unsigned long map_sz, key, desc_size, buff_size;
 	efi_memory_desc_t *mem_map;
 	struct setup_data *e820ext;
-	const char *signature;
 	__u32 e820ext_size;
-	__u32 nr_desc, prev_nr_desc;
 	efi_status_t status;
 	__u32 desc_version;
-	bool called_exit = false;
-	u8 nr_entries;
-	int i;
 	efi_boottime_memory_map_t map;
+	struct exit_boot_struct priv;
 
-	nr_desc = 0;
-	e820ext = NULL;
-	e820ext_size = 0;
 	map.map = &mem_map;
 	map.map_size = &map_sz;
 	map.desc_size = &desc_size;
 	map.desc_ver = &desc_version;
 	map.key_ptr = &key;
 	map.buff_size = &buff_size;
+	priv.boot_params = boot_params;
+	priv.efi = &boot_params->efi_info;
+	priv.e820ext = NULL;
+	priv.e820ext_size = 0;
+	priv.is64 = is64;
 
-get_map:
-	status = efi_get_memory_map(sys_table, &map);
-
+	/* Might as well exit boot services now */
+	status = efi_exit_boot_services(sys_table, handle, &map, &priv,
+					exit_boot_func);
 	if (status != EFI_SUCCESS)
 		return status;
 
-	prev_nr_desc = nr_desc;
-	nr_desc = map_sz / desc_size;
-	if (nr_desc > prev_nr_desc &&
-	    nr_desc > ARRAY_SIZE(boot_params->e820_map)) {
-		u32 nr_e820ext = nr_desc - ARRAY_SIZE(boot_params->e820_map);
-
-		status = alloc_e820ext(nr_e820ext, &e820ext, &e820ext_size);
-		if (status != EFI_SUCCESS)
-			goto free_mem_map;
-
-		efi_call_early(free_pool, mem_map);
-		goto get_map; /* Allocated memory, get map again */
-	}
-
-	signature = is64 ? EFI64_LOADER_SIGNATURE : EFI32_LOADER_SIGNATURE;
-	memcpy(&efi->efi_loader_signature, signature, sizeof(__u32));
-
-	efi->efi_systab = (unsigned long)sys_table;
-	efi->efi_memdesc_size = desc_size;
-	efi->efi_memdesc_version = desc_version;
-	efi->efi_memmap = (unsigned long)mem_map;
-	efi->efi_memmap_size = map_sz;
-
-#ifdef CONFIG_X86_64
-	efi->efi_systab_hi = (unsigned long)sys_table >> 32;
-	efi->efi_memmap_hi = (unsigned long)mem_map >> 32;
-#endif
-
-	/* Might as well exit boot services now */
-	status = efi_call_early(exit_boot_services, handle, key);
-	if (status != EFI_SUCCESS) {
-		/*
-		 * ExitBootServices() will fail if any of the event
-		 * handlers change the memory map. In which case, we
-		 * must be prepared to retry, but only once so that
-		 * we're guaranteed to exit on repeated failures instead
-		 * of spinning forever.
-		 */
-		if (called_exit)
-			goto free_mem_map;
-
-		called_exit = true;
-		efi_call_early(free_pool, mem_map);
-		goto get_map;
-	}
-
+	e820ext = priv.e820ext;
+	e820ext_size = priv.e820ext_size;
 	/* Historic? */
 	boot_params->alt_mem_k = 32 * 1024;
 
@@ -1091,10 +1093,6 @@ get_map:
 		return status;
 
 	return EFI_SUCCESS;
-
-free_mem_map:
-	efi_call_early(free_pool, mem_map);
-	return status;
 }
 
 /*
-- 
2.7.3

[PATCH 6/6] efi/fdt: Fix handling error value in fdt_find_uefi_params

[Matt Fleming]

From: Andrzej Hajda <a.hajda@samsung.com>

of_get_flat_dt_subnode_by_name can return negative value in case of error.

The problem has been detected using semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Cc: Shannon Zhao <shannon.zhao@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Mark Salter <msalter@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/efi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 5a2631af7410..7dd2e2d37231 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -657,9 +657,12 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
 		}
 
 		if (subnode) {
-			node = of_get_flat_dt_subnode_by_name(node, subnode);
-			if (node < 0)
+			int err = of_get_flat_dt_subnode_by_name(node, subnode);
+
+			if (err < 0)
 				return 0;
+
+			node = err;
 		}
 
 		return __find_uefi_params(node, info, dt_params[i].params);
-- 
2.7.3

Re: [PATCH 2/6] efi/libstub: Allocate headspace in efi_get_memory_map()

[Ingo Molnar]

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> +	efi_boottime_memory_map_t map;
>  
>  	nr_desc = 0;
>  	e820ext = NULL;
>  	e820ext_size = 0;
> +	map.map = &mem_map;
> +	map.map_size = &map_sz;
> +	map.desc_size = &desc_size;
> +	map.desc_ver = &desc_version;
> +	map.key_ptr = &key;
> +	map.buff_size = &buff_size;

> +	*map->desc_size = sizeof(*m);
> +	*map->map_size = *map->desc_size * 32;
> +	*map->buff_size = *map->map_size;

> +	boot_map.map = (efi_memory_desc_t **)&map.map;
> +	boot_map.map_size = &map_size;
> +	boot_map.desc_size = &map.desc_size;
> +	boot_map.desc_ver = NULL;
> +	boot_map.key_ptr = NULL;
> +	boot_map.buff_size = &buff_size;

> +	boot_map.map = &map;
> +	boot_map.map_size = &map_size;
> +	boot_map.desc_size = &desc_size;
> +	boot_map.desc_ver = NULL;
> +	boot_map.key_ptr = NULL;
> +	boot_map.buff_size = &buff_size;

> +	efi_boottime_memory_map_t boot_map;
> +
> +	boot_map.map = &map;
> +	boot_map.map_size = &map_size;
> +	boot_map.desc_size = &desc_size;
> +	boot_map.desc_ver = NULL;
> +	boot_map.key_ptr = NULL;
> +	boot_map.buff_size = &buff_size;

> +	efi_boottime_memory_map_t map;
> +
> +	map.map = &runtime_map;
> +	map.map_size = &map_size;
> +	map.desc_size = &desc_size;
> +	map.desc_ver = &desc_ver;
> +	map.key_ptr = &mmap_key;
> +	map.buff_size = &buff_size;

> +	efi_boottime_memory_map_t map;
>  
> -	status = efi_get_memory_map(sys_table_arg, &memory_map, &map_size,
> -				    &desc_size, NULL, NULL);
> +	map.map = &memory_map;
> +	map.map_size = &map_size;
> +	map.desc_size = &desc_size;
> +	map.desc_ver = NULL;
> +	map.key_ptr = NULL;
> +	map.buff_size = &buff_size;

That's really ugly - if we do such initializations then at minimum they should be 
aligned vertically.

>  	u32 imagesize;
>  } efi_capsule_header_t;
>  
> +typedef struct {
> +	efi_memory_desc_t **map;
> +	unsigned long *map_size;
> +	unsigned long *desc_size;
> +	u32 *desc_ver;
> +	unsigned long *key_ptr;
> +	unsigned long *buff_size;
> +} efi_boottime_memory_map_t;

Ditto for structure definitions:

typedef struct {
	efi_memory_desc_t	**map;
	unsigned long		*map_size;
	unsigned long		*desc_size;
	u32			*desc_ver;
	unsigned long		*key_ptr;
	unsigned long		*buff_size;
} efi_boottime_memory_map_t;

Plus it would be nice to just use a proper structure name instead of a typedef - 
such as:

struct efi_boot_memmap {
	...
};

(Note that this name is also shorter)

... in the kernel we generally only use typedefs for short, synthetic types - 
bigger objects like this should be explicit structs - unless there's some strong 
reason to do it via a typedef.

Thanks,

	Ingo

Re: [PATCH 3/6] efi/libstub: Introduce ExitBootServices helper

[Ingo Molnar]

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> From: Jeffrey Hugo <jhugo@codeaurora.org>
> 
> The spec allows ExitBootServices to fail with EFI_INVALID_PARAMETER if a
> race condition has occurred where the EFI has updated the memory map after
> the stub grabbed a reference to the map.  The spec defines a retry
> proceedure with specific requirements to handle this scenario.
> 
> This scenario was previously observed on x86 - commit d3768d885c6c ("x86,
> efi: retry ExitBootServices() on failure") but the current fix is not spec
> compliant and the scenario is now observed on the Qualcomm Technologies
> QDF2432 via the FDT stub which does not handle the error and thus causes
> boot failures.

It's unclear what this 'scenario' is. How does the user notice?

> Add a helper to the stub library that correctly adhears to the spec in the
> case of EFI_INVALID_PARAMETER from ExitBootServices and can be universally
> used across all stub implementations.

s/'adheres to'

> +		status = efi_call_early(exit_boot_services, handle,
> +					*map->key_ptr);

Please don't add spurious linebreaks in such a case - just keep it a bit longer 
than col80.

> +	if (status != EFI_SUCCESS)
> +		/* exit_boot_services() was called, thus cannot free*/
> +		goto fail;

Non-standard comment format and comment placement.

Thanks,

	Ingo

Re: [PATCH 4/6] efi/libstub: Use efi_exit_boot_services() in FDT

[Ingo Molnar]

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> From: Jeffrey Hugo <jhugo@codeaurora.org>
> 
> The FDT code directly calls ExitBootServices.  This is inadvisable as the
> UEFI spec details a complex set of errors, race conditions, and API
> interactions that the caller of ExitBootServices must get correct.  The
> FDT code does not handle EFI_INVALID_PARAMETER as required by the spec,
> which causes intermittent boot failures on the Qualcomm Technologies
> QDF2432.  The efi_exit_boot_services() helper handles the
> EFI_INVALID_PARAMETER scenario.

So I had to look twice to understand what this commit tries to do.

Had it ended with this sentence:

> Call the efi_exit_boot_services() helper intead, which handles the 
> EFI_INVALID_PARAMETER scenario properly.

I wouldn't have looked twice! ;-)

> +	/*
> +	 * Update the memory map with virtual addresses. The function will also
> +	 * populate @runtime_map with copies of just the EFI_MEMORY_RUNTIME
> +	 * entries so that we can pass it straight into SetVirtualAddressMap()

s/straight to

?

Thanks,

	Ingo

Re: [PATCH 6/6] efi/fdt: Fix handling error value in fdt_find_uefi_params

[Ingo Molnar]

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> From: Andrzej Hajda <a.hajda@samsung.com>
> 
> of_get_flat_dt_subnode_by_name can return negative value in case of error.

... which is a problem because <X>, and we solve it by doing <Y>?

Thanks,

	Ingo

Re: [PATCH 6/6] efi/fdt: Fix handling error value in fdt_find_uefi_params

[Matt Fleming]

On Mon, 22 Aug, at 06:45:08PM, Ingo Molnar wrote:
> 
> * Matt Fleming <matt@codeblueprint.co.uk> wrote:
> 
> > From: Andrzej Hajda <a.hajda@samsung.com>
> > 
> > of_get_flat_dt_subnode_by_name can return negative value in case of error.
> 
> ... which is a problem because <X>, and we solve it by doing <Y>?

Andrzej, could you please provide an answer to Ingo's question?

[PATCH v2] efi: fix handling error value in fdt_find_uefi_params

[Andrzej Hajda]

of_get_flat_dt_subnode_by_name can return negative value in case of error.
Assigning the result to unsigned variable and checking if the variable
is lesser than zero is incorrect and always false.
The patch fixes it by using signed variable to check the result.

The problem has been detected using semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/firmware/efi/efi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 5a2631a..7dd2e2d 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -657,9 +657,12 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
 		}
 
 		if (subnode) {
-			node = of_get_flat_dt_subnode_by_name(node, subnode);
-			if (node < 0)
+			int err = of_get_flat_dt_subnode_by_name(node, subnode);
+
+			if (err < 0)
 				return 0;
+
+			node = err;
 		}
 
 		return __find_uefi_params(node, info, dt_params[i].params);
-- 
1.9.1

Re: [PATCH v2] efi: fix handling error value in fdt_find_uefi_params

[Matt Fleming]

On Tue, 30 Aug, at 12:41:37PM, Andrzej Hajda wrote:
> of_get_flat_dt_subnode_by_name can return negative value in case of error.
> Assigning the result to unsigned variable and checking if the variable
> is lesser than zero is incorrect and always false.
> The patch fixes it by using signed variable to check the result.
> 
> The problem has been detected using semantic patch
> scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/firmware/efi/efi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 5a2631a..7dd2e2d 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -657,9 +657,12 @@ static int __init fdt_find_uefi_params(unsigned long node, const char *uname,
>  		}
>  
>  		if (subnode) {
> -			node = of_get_flat_dt_subnode_by_name(node, subnode);
> -			if (node < 0)
> +			int err = of_get_flat_dt_subnode_by_name(node, subnode);
> +
> +			if (err < 0)
>  				return 0;
> +
> +			node = err;
>  		}
>  
>  		return __find_uefi_params(node, info, dt_params[i].params);

Thanks Andrzej, applied.