[PATCH 0/3] UEFI stub FDT handling fixes

[PATCH 0/3] UEFI stub FDT handling fixes

[Ard Biesheuvel]

This is an alternative approach to some of the issues addressed by my series
'arm64 UEFI early FDT handling', of which the latest version (v3) can be found
here: http://thread.gmane.org/gmane.linux.kernel.efi/6334

The issues that are addressed by the original series are:
- when booting via UEFI, memreserve entries are removed from the device tree but
  the /reserved-memory node is not
- memory nodes are removed from the device tree in a way that is not officially
  supported by the libfdt API (i.e., you cannot delete nodes while traversing
  the tree)
- removal of memory nodes may discard annotations (such as NUMA topology) that
  should ideally be retained, or may corrupt the tree by discarding nodes
  referenced by phandles.

After having discussed this offline between Grant, Leif and myself, we have come
to the [preliminary] conclusion that discarding the /reserved-memory is not the
way to go, and we will be far better off honoring those reservations to the
extent possible. At the same time, the /memory node binding does not contain any
annotations, so discarding those prevents more issues than it creates as long as
we do it cautiously.

This series addresses both issues: it it sanity check the /reserved-memory node
to ensure that it does not contain any reservations we will not be able to
honor (patch #1, #2) and updates the /memory removal loop to restart from the
root of the tree each time a /memory node is found and removed (patch #3)

Ard Biesheuvel (3):
  efi/libstub: move FDT sanity check out of allocation loop
  efi/libstub: sanity check the /reserved-memory DT node
  efi/libstub: fix deletion of FDT memory nodes

 drivers/firmware/efi/libstub/fdt.c | 187 ++++++++++++++++++--
 1 file changed, 169 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH 1/3] efi/libstub: move FDT sanity check out of allocation loop

[Ard Biesheuvel]

The memory allocation for the updated FDT may execute several times
if the allocation turns out to be insufficiently large. There is no
need to sanity check the original FDT each time, so take it out of
the loop.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/libstub/fdt.c | 42 ++++++++++++--------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index ef5d764e2a27..354172bee81c 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -16,6 +16,26 @@
 
 #include "efistub.h"
 
+static efi_status_t sanity_check_fdt(efi_system_table_t *sys_table,
+				     void *fdt, unsigned long fdt_size)
+{
+	if (fdt_check_header(fdt)) {
+		pr_efi_err(sys_table, "Device Tree header not valid!\n");
+		return EFI_LOAD_ERROR;
+	}
+
+	/*
+	 * We don't get the size of the FDT if we get if from a
+	 * configuration table.
+	 */
+	if (fdt_size && fdt_totalsize(fdt) > fdt_size) {
+		pr_efi_err(sys_table, "Truncated device tree! foo!\n");
+		return EFI_LOAD_ERROR;
+	}
+
+	return EFI_SUCCESS;
+}
+
 efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
 			unsigned long orig_fdt_size,
 			void *fdt, int new_fdt_size, char *cmdline_ptr,
@@ -29,22 +49,6 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
 	u32 fdt_val32;
 	u64 fdt_val64;
 
-	/* Do some checks on provided FDT, if it exists*/
-	if (orig_fdt) {
-		if (fdt_check_header(orig_fdt)) {
-			pr_efi_err(sys_table, "Device Tree header not valid!\n");
-			return EFI_LOAD_ERROR;
-		}
-		/*
-		 * We don't get the size of the FDT if we get if from a
-		 * configuration table.
-		 */
-		if (orig_fdt_size && fdt_totalsize(orig_fdt) > orig_fdt_size) {
-			pr_efi_err(sys_table, "Truncated device tree! foo!\n");
-			return EFI_LOAD_ERROR;
-		}
-	}
-
 	if (orig_fdt)
 		status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
 	else
@@ -213,6 +217,12 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 		return status;
 	}
 
+	if (fdt_addr) {
+		status = sanity_check_fdt(sys_table, (void*)fdt_addr, fdt_size);
+		if (status != EFI_SUCCESS)
+			goto fail;
+	}
+
 	pr_efi(sys_table,
 	       "Exiting boot services and installing virtual address map...\n");
 
-- 
1.9.1

Re: [PATCH 1/3] efi/libstub: move FDT sanity check out of allocation loop

[Mark Rutland]

On Wed, Oct 07, 2015 at 09:35:27AM +0100, Ard Biesheuvel wrote:
> The memory allocation for the updated FDT may execute several times
> if the allocation turns out to be insufficiently large. There is no
> need to sanity check the original FDT each time, so take it out of
> the loop.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>

Mark.

> ---
>  drivers/firmware/efi/libstub/fdt.c | 42 ++++++++++++--------
>  1 file changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index ef5d764e2a27..354172bee81c 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -16,6 +16,26 @@
>  
>  #include "efistub.h"
>  
> +static efi_status_t sanity_check_fdt(efi_system_table_t *sys_table,
> +				     void *fdt, unsigned long fdt_size)
> +{
> +	if (fdt_check_header(fdt)) {
> +		pr_efi_err(sys_table, "Device Tree header not valid!\n");
> +		return EFI_LOAD_ERROR;
> +	}
> +
> +	/*
> +	 * We don't get the size of the FDT if we get if from a
> +	 * configuration table.
> +	 */
> +	if (fdt_size && fdt_totalsize(fdt) > fdt_size) {
> +		pr_efi_err(sys_table, "Truncated device tree! foo!\n");
> +		return EFI_LOAD_ERROR;
> +	}
> +
> +	return EFI_SUCCESS;
> +}
> +
>  efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>  			unsigned long orig_fdt_size,
>  			void *fdt, int new_fdt_size, char *cmdline_ptr,
> @@ -29,22 +49,6 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>  	u32 fdt_val32;
>  	u64 fdt_val64;
>  
> -	/* Do some checks on provided FDT, if it exists*/
> -	if (orig_fdt) {
> -		if (fdt_check_header(orig_fdt)) {
> -			pr_efi_err(sys_table, "Device Tree header not valid!\n");
> -			return EFI_LOAD_ERROR;
> -		}
> -		/*
> -		 * We don't get the size of the FDT if we get if from a
> -		 * configuration table.
> -		 */
> -		if (orig_fdt_size && fdt_totalsize(orig_fdt) > orig_fdt_size) {
> -			pr_efi_err(sys_table, "Truncated device tree! foo!\n");
> -			return EFI_LOAD_ERROR;
> -		}
> -	}
> -
>  	if (orig_fdt)
>  		status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
>  	else
> @@ -213,6 +217,12 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>  		return status;
>  	}
>  
> +	if (fdt_addr) {
> +		status = sanity_check_fdt(sys_table, (void*)fdt_addr, fdt_size);
> +		if (status != EFI_SUCCESS)
> +			goto fail;
> +	}
> +
>  	pr_efi(sys_table,
>  	       "Exiting boot services and installing virtual address map...\n");
>  
> -- 
> 1.9.1
> 

Re: [PATCH 1/3] efi/libstub: move FDT sanity check out of allocation loop

[Ard Biesheuvel]

On 17 November 2015 at 12:47, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Wed, Oct 07, 2015 at 09:35:27AM +0100, Ard Biesheuvel wrote:
>> The memory allocation for the updated FDT may execute several times
>> if the allocation turns out to be insufficiently large. There is no
>> need to sanity check the original FDT each time, so take it out of
>> the loop.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
>

Thanks.

In the mean time, I noticed that this could be refactored even
further, since checking the size only makes sense if the FDT was
loaded from a file, and checking the header is redundant if the FDT
was loaded from a config table.

-- 
Ard.


>> ---
>>  drivers/firmware/efi/libstub/fdt.c | 42 ++++++++++++--------
>>  1 file changed, 26 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
>> index ef5d764e2a27..354172bee81c 100644
>> --- a/drivers/firmware/efi/libstub/fdt.c
>> +++ b/drivers/firmware/efi/libstub/fdt.c
>> @@ -16,6 +16,26 @@
>>
>>  #include "efistub.h"
>>
>> +static efi_status_t sanity_check_fdt(efi_system_table_t *sys_table,
>> +                                  void *fdt, unsigned long fdt_size)
>> +{
>> +     if (fdt_check_header(fdt)) {
>> +             pr_efi_err(sys_table, "Device Tree header not valid!\n");
>> +             return EFI_LOAD_ERROR;
>> +     }
>> +
>> +     /*
>> +      * We don't get the size of the FDT if we get if from a
>> +      * configuration table.
>> +      */
>> +     if (fdt_size && fdt_totalsize(fdt) > fdt_size) {
>> +             pr_efi_err(sys_table, "Truncated device tree! foo!\n");
>> +             return EFI_LOAD_ERROR;
>> +     }
>> +
>> +     return EFI_SUCCESS;
>> +}
>> +
>>  efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>                       unsigned long orig_fdt_size,
>>                       void *fdt, int new_fdt_size, char *cmdline_ptr,
>> @@ -29,22 +49,6 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>       u32 fdt_val32;
>>       u64 fdt_val64;
>>
>> -     /* Do some checks on provided FDT, if it exists*/
>> -     if (orig_fdt) {
>> -             if (fdt_check_header(orig_fdt)) {
>> -                     pr_efi_err(sys_table, "Device Tree header not valid!\n");
>> -                     return EFI_LOAD_ERROR;
>> -             }
>> -             /*
>> -              * We don't get the size of the FDT if we get if from a
>> -              * configuration table.
>> -              */
>> -             if (orig_fdt_size && fdt_totalsize(orig_fdt) > orig_fdt_size) {
>> -                     pr_efi_err(sys_table, "Truncated device tree! foo!\n");
>> -                     return EFI_LOAD_ERROR;
>> -             }
>> -     }
>> -
>>       if (orig_fdt)
>>               status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
>>       else
>> @@ -213,6 +217,12 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>>               return status;
>>       }
>>
>> +     if (fdt_addr) {
>> +             status = sanity_check_fdt(sys_table, (void*)fdt_addr, fdt_size);
>> +             if (status != EFI_SUCCESS)
>> +                     goto fail;
>> +     }
>> +
>>       pr_efi(sys_table,
>>              "Exiting boot services and installing virtual address map...\n");
>>
>> --
>> 1.9.1
>>

[PATCH 2/3] efi/libstub: sanity check the /reserved-memory DT node

[Ard Biesheuvel]

One of the DT bindings supported by the Linux kernel is the
/reserved-memory node, which may contain both static and dynamic
reservations that the kernel honors very early on, before using any
memory outside of the static footprint of the kernel.

On arm64, we started out by ignoring all memory reservations, but after
merging commit 0ceac9e094b0 ("009800efi/arm64: Fix fdt-related memory
reservation inadvertently"), the /reserved-memory node is preserved,
which may result in reservations that conflict with memory regions that
have already been allocated for other purposes by UEFI.

So let's sanity check the contents of this node in the EFI stub, by
trying to allocate each static reservation as EfiLoaderData (which will
result in UEFI leaving this memory alone until it releases it to the
kernel), and if that fails, check whether the region is either already
reserved in the UEFI memory map (as EfiReservedMemoryType memory), or
is completely disjoint from the UEFI memory map to begin with. If all
of that fails, the region is apparently allocated for some other purpose
by UEFI, which means we will not be able to honor the reservation. If
this happens, we abort the boot, since we cannot really be sure that it
is safe to proceed.

NOTE: there is an implicit assumption in this approach that reservations
can tolerate being allocated and freed again for temporary use by UEFI.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/libstub/fdt.c | 144 +++++++++++++++++++-
 1 file changed, 142 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 354172bee81c..0ef16923b4f0 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -13,12 +13,140 @@
 #include <linux/efi.h>
 #include <linux/libfdt.h>
 #include <asm/efi.h>
+#include <asm/unaligned.h>
 
 #include "efistub.h"
 
+/*
+ * Return whether the reserved region [addr, addr + size) can be considered as
+ * 'reserved' from the POV of UEFI. This is the case if the region is entirely
+ * covered by a EfiReservedMemoryType region or if the region does not appear in
+ * the UEFI memory map at all.
+ */
+static bool region_is_reserved(efi_system_table_t *sys_table_arg,
+			       u64 addr, u64 size,
+			       efi_memory_desc_t *memory_map,
+			       unsigned long map_size,
+			       unsigned long desc_size)
+{
+	int i;
+
+	for (i = 0; i < map_size; i += desc_size) {
+		efi_memory_desc_t *entry = (void *)memory_map + i;
+		u64 entry_end = entry->phys_addr +
+				entry->num_pages * EFI_PAGE_SIZE;
+
+		/* disregard this entry if it does not intersect the region */
+		if (entry_end <= addr || entry->phys_addr >= (addr + size))
+			continue;
+
+		/* if it does intersect the region, it must cover it entirely */
+		if (addr < entry->phys_addr || (addr + size) > entry_end)
+			return false;
+
+		return (entry->type == EFI_RESERVED_TYPE);
+	}
+
+	/* not covered by the UEFI memory map */
+	return true;
+}
+
+static u64 get_node_val(const void *prop, u32 num_cells)
+{
+	if (num_cells == 1)
+		return be32_to_cpup(prop);
+	else
+		return get_unaligned_be64(prop);
+}
+
+static efi_status_t handle_reserved_memory(efi_system_table_t *sys_table_arg,
+					   void *fdt, int node,
+					   efi_memory_desc_t *memory_map,
+					   unsigned long map_size,
+					   unsigned long desc_size)
+{
+	efi_status_t status;
+	const void *prop;
+	u32 addr_cells, size_cells, stride;
+	int len;
+
+	prop = fdt_getprop(fdt, node, "#address-cells", &len);
+	if (!prop || !len)
+		goto err_missing_node;
+	addr_cells = be32_to_cpup(prop);
+
+	prop = fdt_getprop(fdt, node, "#size-cells", &len);
+	if (!prop || !len)
+		goto err_missing_node;
+	size_cells = be32_to_cpup(prop);
+
+	stride = (addr_cells + size_cells) * sizeof(u32);
+
+	/* enumerate each subnode and look for 'reg' properties */
+	for (;;) {
+		int depth, i;
+
+		node = fdt_next_node(fdt, node, &depth);
+		if (node < 0 || depth < 1)
+			/* done with /reserved-memory node */
+			break;
+
+		/*
+		 * Check for a 'reg' property. If it does not have one, this is
+		 * a dynamic allocation that we can ignore.
+		 */
+		prop = fdt_getprop(fdt, node, "reg", &len);
+		if (!prop)
+			continue;
+
+		for (i = 0; i < len; i += stride) {
+			const u32 *val = prop + i;
+			u64 addr, size;
+			u32 pages;
+
+			/* discover each addr/size tuple in the reg property */
+			addr = get_node_val(val, addr_cells);
+			size = get_node_val(val + addr_cells, size_cells);
+
+			pages = round_up(size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
+
+			/* allocate the region as EfiLoaderData */
+			status = efi_call_early(allocate_pages,
+						EFI_ALLOCATE_ADDRESS,
+						EFI_LOADER_DATA,
+						pages, &addr);
+
+			/*
+			 * If we fail to allocate the reserved region, it could
+			 * be reserved already (i.e., as EfiReservedMemoryType)
+			 * or not appear in the UEFI memory map at all. In both
+			 * cases, we can be sure that UEFI will not touch it, so
+			 * we can simply ignore it.
+			 */
+			if (status != EFI_SUCCESS &&
+			    !region_is_reserved(sys_table_arg, addr, size,
+						memory_map, map_size,
+						desc_size)) {
+				pr_efi_err(sys_table_arg, "Failed to reserve /reserved-memory node!\n");
+				return status;
+			}
+		}
+	}
+	return EFI_SUCCESS;
+
+err_missing_node:
+	pr_efi_err(sys_table_arg, "Missing /reserved-memory #cells properties!\n");
+	return EFI_LOAD_ERROR;
+}
+
 static efi_status_t sanity_check_fdt(efi_system_table_t *sys_table,
-				     void *fdt, unsigned long fdt_size)
+				     void *fdt, unsigned long fdt_size,
+				     efi_memory_desc_t *memory_map,
+				     unsigned long map_size,
+				     unsigned long desc_size)
 {
+	int node;
+
 	if (fdt_check_header(fdt)) {
 		pr_efi_err(sys_table, "Device Tree header not valid!\n");
 		return EFI_LOAD_ERROR;
@@ -33,6 +161,17 @@ static efi_status_t sanity_check_fdt(efi_system_table_t *sys_table,
 		return EFI_LOAD_ERROR;
 	}
 
+	/*
+	 * If we have a /reserved-memory node, ensure that all regions are
+	 * free by explicitly allocating them as LoaderData. If this fails,
+	 * we bail since we have no clue if the system can actually boot
+	 * without these reservations.
+	 */
+	node = fdt_path_offset(fdt, "/reserved-memory");
+	if (node >= 0)
+		return handle_reserved_memory(sys_table, fdt, node, memory_map,
+					      map_size, desc_size);
+
 	return EFI_SUCCESS;
 }
 
@@ -218,7 +357,8 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	}
 
 	if (fdt_addr) {
-		status = sanity_check_fdt(sys_table, (void*)fdt_addr, fdt_size);
+		status = sanity_check_fdt(sys_table, (void *)fdt_addr, fdt_size,
+					  runtime_map, map_size, desc_size);
 		if (status != EFI_SUCCESS)
 			goto fail;
 	}
-- 
1.9.1

[PATCH 3/3] efi/libstub: fix deletion of FDT memory nodes

[Ard Biesheuvel]

This fixes two issues with the EFI libstub code that removes the memory
nodes from the FDT:
a) fdt_del_node() invalidates the iterator, so we should restart from the
   top after calling it;
b) use fixed length of 6 when matching against 'memory' using strncmp(),
   since otherwise, substrings like 'm' or 'mem' could match as well.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/libstub/fdt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 0ef16923b4f0..2c7d09936479 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -200,6 +200,7 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
 	 * Delete any memory nodes present. We must delete nodes which
 	 * early_init_dt_scan_memory may try to use.
 	 */
+restart:
 	prev = 0;
 	for (;;) {
 		const char *type;
@@ -210,9 +211,9 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
 			break;
 
 		type = fdt_getprop(fdt, node, "device_type", &len);
-		if (type && strncmp(type, "memory", len) == 0) {
+		if (type && strncmp(type, "memory", 6) == 0) {
 			fdt_del_node(fdt, node);
-			continue;
+			goto restart;
 		}
 
 		prev = node;
-- 
1.9.1

Re: [PATCH 3/3] efi/libstub: fix deletion of FDT memory nodes

[Mark Salter]

On Wed, 2015-10-07 at 09:35 +0100, Ard Biesheuvel wrote:
> This fixes two issues with the EFI libstub code that removes the memory
> nodes from the FDT:
> a) fdt_del_node() invalidates the iterator, so we should restart from the
>    top after calling it;
> b) use fixed length of 6 when matching against 'memory' using strncmp(),
>    since otherwise, substrings like 'm' or 'mem' could match as well.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/libstub/fdt.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 0ef16923b4f0..2c7d09936479 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -200,6 +200,7 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>  	 * Delete any memory nodes present. We must delete nodes which
>  	 * early_init_dt_scan_memory may try to use.
>  	 */
> +restart:
>  	prev = 0;
>  	for (;;) {
>  		const char *type;
> @@ -210,9 +211,9 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>  			break;
>  
>  		type = fdt_getprop(fdt, node, "device_type", &len);
> -		if (type && strncmp(type, "memory", len) == 0) {
> +		if (type && strncmp(type, "memory", 6) == 0) {

I see what you're trying to fix here, but I think using 6 is wrong
also. A plain strcmp() would work as long as property is a string.


>  			fdt_del_node(fdt, node);
> -			continue;
> +			goto restart;
>  		}
>  
>  		prev = node;

Maybe do:

 		if (type && strncmp(type, "memory", len) == 0) {
 			fdt_del_node(fdt, node);
-			continue;
-		}
-
-		prev = node;
+			prev = NULL;
+		} else
+			prev = node;
 	}


instead of adding a goto.

Re: [PATCH 3/3] efi/libstub: fix deletion of FDT memory nodes

[Ard Biesheuvel]

On 7 October 2015 at 16:49, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 2015-10-07 at 09:35 +0100, Ard Biesheuvel wrote:
>> This fixes two issues with the EFI libstub code that removes the memory
>> nodes from the FDT:
>> a) fdt_del_node() invalidates the iterator, so we should restart from the
>>    top after calling it;
>> b) use fixed length of 6 when matching against 'memory' using strncmp(),
>>    since otherwise, substrings like 'm' or 'mem' could match as well.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/firmware/efi/libstub/fdt.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
>> index 0ef16923b4f0..2c7d09936479 100644
>> --- a/drivers/firmware/efi/libstub/fdt.c
>> +++ b/drivers/firmware/efi/libstub/fdt.c
>> @@ -200,6 +200,7 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>        * Delete any memory nodes present. We must delete nodes which
>>        * early_init_dt_scan_memory may try to use.
>>        */
>> +restart:
>>       prev = 0;
>>       for (;;) {
>>               const char *type;
>> @@ -210,9 +211,9 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>                       break;
>>
>>               type = fdt_getprop(fdt, node, "device_type", &len);
>> -             if (type && strncmp(type, "memory", len) == 0) {
>> +             if (type && strncmp(type, "memory", 6) == 0) {
>
> I see what you're trying to fix here, but I think using 6 is wrong
> also. A plain strcmp() would work as long as property is a string.
>

That is what Leif suggested as well, but it pulls in yet another
string function into the stub, so I tried to avoid it. I think this
particular case is safe, since the property names are in the string
table, which is after the nodes in the DTB structure, so 'type' is
guaranteed not to point right before the end of a mapped region.

>
>>                       fdt_del_node(fdt, node);
>> -                     continue;
>> +                     goto restart;
>>               }
>>
>>               prev = node;
>
> Maybe do:
>
>                 if (type && strncmp(type, "memory", len) == 0) {
>                         fdt_del_node(fdt, node);
> -                       continue;
> -               }
> -
> -               prev = node;
> +                       prev = NULL;
> +               } else
> +                       prev = node;
>         }
>
>
> instead of adding a goto.
>

Yes, that is actually better. Thanks.

Re: [PATCH 3/3] efi/libstub: fix deletion of FDT memory nodes

[Ard Biesheuvel]

On 7 October 2015 at 17:02, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 7 October 2015 at 16:49, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On Wed, 2015-10-07 at 09:35 +0100, Ard Biesheuvel wrote:
>>> This fixes two issues with the EFI libstub code that removes the memory
>>> nodes from the FDT:
>>> a) fdt_del_node() invalidates the iterator, so we should restart from the
>>>    top after calling it;
>>> b) use fixed length of 6 when matching against 'memory' using strncmp(),
>>>    since otherwise, substrings like 'm' or 'mem' could match as well.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> ---
>>>  drivers/firmware/efi/libstub/fdt.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
>>> index 0ef16923b4f0..2c7d09936479 100644
>>> --- a/drivers/firmware/efi/libstub/fdt.c
>>> +++ b/drivers/firmware/efi/libstub/fdt.c
>>> @@ -200,6 +200,7 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>>        * Delete any memory nodes present. We must delete nodes which
>>>        * early_init_dt_scan_memory may try to use.
>>>        */
>>> +restart:
>>>       prev = 0;
>>>       for (;;) {
>>>               const char *type;
>>> @@ -210,9 +211,9 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>>                       break;
>>>
>>>               type = fdt_getprop(fdt, node, "device_type", &len);
>>> -             if (type && strncmp(type, "memory", len) == 0) {
>>> +             if (type && strncmp(type, "memory", 6) == 0) {
>>
>> I see what you're trying to fix here, but I think using 6 is wrong
>> also. A plain strcmp() would work as long as property is a string.
>>
>
> That is what Leif suggested as well, but it pulls in yet another
> string function into the stub, so I tried to avoid it. I think this
> particular case is safe, since the property names are in the string
> table, which is after the nodes in the DTB structure, so 'type' is
> guaranteed not to point right before the end of a mapped region.
>

... or is your concern about matching on longer strings that only
start with 'memory'?

I guess it makes sense to use strcmp() after all, only arm64 does not
have an asm implementation, so under the stricter rules that I am
proposing for fencing off the EFI stub code from the kernel proper, I
will have to go and add a strcmp() implementation to
drivers/firmware/efi/libstub/string.c.

But still better than this, I guess ...


>>
>>>                       fdt_del_node(fdt, node);
>>> -                     continue;
>>> +                     goto restart;
>>>               }
>>>
>>>               prev = node;
>>
>> Maybe do:
>>
>>                 if (type && strncmp(type, "memory", len) == 0) {
>>                         fdt_del_node(fdt, node);
>> -                       continue;
>> -               }
>> -
>> -               prev = node;
>> +                       prev = NULL;
>> +               } else
>> +                       prev = node;
>>         }
>>
>>
>> instead of adding a goto.
>>
>
> Yes, that is actually better. Thanks.

Re: [PATCH 3/3] efi/libstub: fix deletion of FDT memory nodes

[Ard Biesheuvel]

On 7 October 2015 at 18:56, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 7 October 2015 at 17:02, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On 7 October 2015 at 16:49, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> On Wed, 2015-10-07 at 09:35 +0100, Ard Biesheuvel wrote:
>>>> This fixes two issues with the EFI libstub code that removes the memory
>>>> nodes from the FDT:
>>>> a) fdt_del_node() invalidates the iterator, so we should restart from the
>>>>    top after calling it;
>>>> b) use fixed length of 6 when matching against 'memory' using strncmp(),
>>>>    since otherwise, substrings like 'm' or 'mem' could match as well.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>> ---
>>>>  drivers/firmware/efi/libstub/fdt.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
>>>> index 0ef16923b4f0..2c7d09936479 100644
>>>> --- a/drivers/firmware/efi/libstub/fdt.c
>>>> +++ b/drivers/firmware/efi/libstub/fdt.c
>>>> @@ -200,6 +200,7 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>>>        * Delete any memory nodes present. We must delete nodes which
>>>>        * early_init_dt_scan_memory may try to use.
>>>>        */
>>>> +restart:
>>>>       prev = 0;
>>>>       for (;;) {
>>>>               const char *type;
>>>> @@ -210,9 +211,9 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>>>                       break;
>>>>
>>>>               type = fdt_getprop(fdt, node, "device_type", &len);
>>>> -             if (type && strncmp(type, "memory", len) == 0) {
>>>> +             if (type && strncmp(type, "memory", 6) == 0) {
>>>
>>> I see what you're trying to fix here, but I think using 6 is wrong
>>> also. A plain strcmp() would work as long as property is a string.
>>>
>>
>> That is what Leif suggested as well, but it pulls in yet another
>> string function into the stub, so I tried to avoid it. I think this
>> particular case is safe, since the property names are in the string
>> table, which is after the nodes in the DTB structure, so 'type' is
>> guaranteed not to point right before the end of a mapped region.
>>
>
> ... or is your concern about matching on longer strings that only
> start with 'memory'?
>
> I guess it makes sense to use strcmp() after all, only arm64 does not
> have an asm implementation, so under the stricter rules that I am
> proposing for fencing off the EFI stub code from the kernel proper, I
> will have to go and add a strcmp() implementation to
> drivers/firmware/efi/libstub/string.c.
>


OK, strike that. It does have an asm implementation, I just have to
expose it to the stub,

Thanks,
Ard.

Re: [PATCH 3/3] efi/libstub: fix deletion of FDT memory nodes

[Mark Salter]

On Wed, 2015-10-07 at 18:56 +0100, Ard Biesheuvel wrote:
> On 7 October 2015 at 17:02, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On 7 October 2015 at 16:49, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > > On Wed, 2015-10-07 at 09:35 +0100, Ard Biesheuvel wrote:
> > > > This fixes two issues with the EFI libstub code that removes the memory
> > > > nodes from the FDT:
> > > > a) fdt_del_node() invalidates the iterator, so we should restart from the
> > > >    top after calling it;
> > > > b) use fixed length of 6 when matching against 'memory' using strncmp(),
> > > >    since otherwise, substrings like 'm' or 'mem' could match as well.
> > > > 
> > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > > ---
> > > >  drivers/firmware/efi/libstub/fdt.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> > > > index 0ef16923b4f0..2c7d09936479 100644
> > > > --- a/drivers/firmware/efi/libstub/fdt.c
> > > > +++ b/drivers/firmware/efi/libstub/fdt.c
> > > > @@ -200,6 +200,7 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
> > > >        * Delete any memory nodes present. We must delete nodes which
> > > >        * early_init_dt_scan_memory may try to use.
> > > >        */
> > > > +restart:
> > > >       prev = 0;
> > > >       for (;;) {
> > > >               const char *type;
> > > > @@ -210,9 +211,9 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
> > > >                       break;
> > > > 
> > > >               type = fdt_getprop(fdt, node, "device_type", &len);
> > > > -             if (type && strncmp(type, "memory", len) == 0) {
> > > > +             if (type && strncmp(type, "memory", 6) == 0) {
> > > 
> > > I see what you're trying to fix here, but I think using 6 is wrong
> > > also. A plain strcmp() would work as long as property is a string.
> > > 
> > 
> > That is what Leif suggested as well, but it pulls in yet another
> > string function into the stub, so I tried to avoid it. I think this
> > particular case is safe, since the property names are in the string
> > table, which is after the nodes in the DTB structure, so 'type' is
> > guaranteed not to point right before the end of a mapped region.
> > 
> 
> ... or is your concern about matching on longer strings that only
> start with 'memory'?

Yes. And the old code would match shorted strings than "memory".

> 
> I guess it makes sense to use strcmp() after all, only arm64 does not
> have an asm implementation, so under the stricter rules that I am
> proposing for fencing off the EFI stub code from the kernel proper, I
> will have to go and add a strcmp() implementation to
> drivers/firmware/efi/libstub/string.c.
> 
> But still better than this, I guess ...

You could also add an explicit length comparison but adding strcmp
seems pretty reasonable.

> 
> 
> > > 
> > > >                       fdt_del_node(fdt, node);
> > > > -                     continue;
> > > > +                     goto restart;
> > > >               }
> > > > 
> > > >               prev = node;
> > > 
> > > Maybe do:
> > > 
> > >                 if (type && strncmp(type, "memory", len) == 0) {
> > >                         fdt_del_node(fdt, node);
> > > -                       continue;
> > > -               }
> > > -
> > > -               prev = node;
> > > +                       prev = NULL;
> > > +               } else
> > > +                       prev = node;
> > >         }
> > > 
> > > 
> > > instead of adding a goto.
> > > 
> > 
> > Yes, that is actually better. Thanks.

Re: [PATCH 0/3] UEFI stub FDT handling fixes

[Roy Franz]

On Wed, Oct 7, 2015 at 1:35 AM, Ard Biesheuvel
<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> This is an alternative approach to some of the issues addressed by my series
> 'arm64 UEFI early FDT handling', of which the latest version (v3) can be found
> here: http://thread.gmane.org/gmane.linux.kernel.efi/6334
>
> The issues that are addressed by the original series are:
> - when booting via UEFI, memreserve entries are removed from the device tree but
>   the /reserved-memory node is not
> - memory nodes are removed from the device tree in a way that is not officially
>   supported by the libfdt API (i.e., you cannot delete nodes while traversing
>   the tree)
> - removal of memory nodes may discard annotations (such as NUMA topology) that
>   should ideally be retained, or may corrupt the tree by discarding nodes
>   referenced by phandles.
>
> After having discussed this offline between Grant, Leif and myself, we have come
> to the [preliminary] conclusion that discarding the /reserved-memory is not the
> way to go, and we will be far better off honoring those reservations to the
> extent possible. At the same time, the /memory node binding does not contain any
> annotations, so discarding those prevents more issues than it creates as long as
> we do it cautiously.
So if/when NUMA annotations are added to the memory node, we will need
to revisit this?

>
> This series addresses both issues: it it sanity check the /reserved-memory node
> to ensure that it does not contain any reservations we will not be able to
> honor (patch #1, #2) and updates the /memory removal loop to restart from the
> root of the tree each time a /memory node is found and removed (patch #3)
>
> Ard Biesheuvel (3):
>   efi/libstub: move FDT sanity check out of allocation loop
>   efi/libstub: sanity check the /reserved-memory DT node
>   efi/libstub: fix deletion of FDT memory nodes
>
>  drivers/firmware/efi/libstub/fdt.c | 187 ++++++++++++++++++--
>  1 file changed, 169 insertions(+), 18 deletions(-)
>
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] UEFI stub FDT handling fixes

[Ard Biesheuvel]

I haven't followed the associativity discussion closely, but perhaps it needs to be folden in there instead.

Ard.

> On 8 okt. 2015, at 03:25, Roy Franz <roy.franz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> On Wed, Oct 7, 2015 at 1:35 AM, Ard Biesheuvel
> <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> This is an alternative approach to some of the issues addressed by my series
>> 'arm64 UEFI early FDT handling', of which the latest version (v3) can be found
>> here: http://thread.gmane.org/gmane.linux.kernel.efi/6334
>> 
>> The issues that are addressed by the original series are:
>> - when booting via UEFI, memreserve entries are removed from the device tree but
>>  the /reserved-memory node is not
>> - memory nodes are removed from the device tree in a way that is not officially
>>  supported by the libfdt API (i.e., you cannot delete nodes while traversing
>>  the tree)
>> - removal of memory nodes may discard annotations (such as NUMA topology) that
>>  should ideally be retained, or may corrupt the tree by discarding nodes
>>  referenced by phandles.
>> 
>> After having discussed this offline between Grant, Leif and myself, we have come
>> to the [preliminary] conclusion that discarding the /reserved-memory is not the
>> way to go, and we will be far better off honoring those reservations to the
>> extent possible. At the same time, the /memory node binding does not contain any
>> annotations, so discarding those prevents more issues than it creates as long as
>> we do it cautiously.
> So if/when NUMA annotations are added to the memory node, we will need
> to revisit this?

Yes. But now, I am not so convinced anymore that /memory is where these annotations should go in the first place. Preserving but ignoring /memory means you are preserving info that we don't know is accurate (i.e., in sync with the UEFI memory map)

> 
>> 
>> This series addresses both issues: it it sanity check the /reserved-memory node
>> to ensure that it does not contain any reservations we will not be able to
>> honor (patch #1, #2) and updates the /memory removal loop to restart from the
>> root of the tree each time a /memory node is found and removed (patch #3)
>> 
>> Ard Biesheuvel (3):
>>  efi/libstub: move FDT sanity check out of allocation loop
>>  efi/libstub: sanity check the /reserved-memory DT node
>>  efi/libstub: fix deletion of FDT memory nodes
>> 
>> drivers/firmware/efi/libstub/fdt.c | 187 ++++++++++++++++++--
>> 1 file changed, 169 insertions(+), 18 deletions(-)
>> 
>> --
>> 1.9.1
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html