[PATCH v12] ARM: uncompress: Validate start of physical memory against passed DTB

[PATCH v12] ARM: uncompress: Validate start of physical memory against passed DTB

[Geert Uytterhoeven]

Currently, the start address of physical memory is obtained by masking
the program counter with a fixed mask of 0xf8000000.  This mask value
was chosen as a balance between the requirements of different platforms.
However, this does require that the start address of physical memory is
a multiple of 128 MiB, precluding booting Linux on platforms where this
requirement is not fulfilled.

Fix this limitation by validating the masked address against the memory
information in the passed DTB.  Only use the start address
from DTB when masking would yield an out-of-range address, prefer the
traditional method in all other cases.  Note that this applies only to the
explicitly passed DTB on modern systems, and not to a DTB appended to
the kernel, or to ATAGS.  The appended DTB may need to be augmented by
information from ATAGS, which may need to rely on knowledge of the start
address of physical memory itself.

This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
i.e. not at a multiple of 128 MiB.

Suggested-by: Nicolas Pitre <nico@fluxnic.net>
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
Acked-by: Nicolas Pitre <nico@fluxnic.net>
---
Submitted to RMK's patch tracker.

v12:
  - Drop unneeded initialization of r in get_val(),

v11:
  - Add Reviewed-by, Acked-by,
  - Document GOT fixup caveat,

v10:
  - Update for commit 9443076e4330a14a ("ARM: p2v: reduce p2v alignment
    requirement to 2 MiB"),
  - Use OF_DT_MAGIC macro,
  - Rename fdt_get_mem_start() to fdt_check_mem_start(),
  - Skip validation if there is an appended DTB,
  - Pass mem_start to fdt_check_mem_start() to streamline code,
  - Optimize register allocation,
  - Update CONFIG_AUTO_ZRELADDR help text,
  - Check all memory nodes and ranges (not just the first one), and
    "linux,usable-memory", similar to early_init_dt_scan_memory(),
  - Drop Reviewed-by, Tested-by tags,

v9:
  - Add minlen parameter to getprop(), for better validation of
    memory properties,
  - Only use start of memory from the DTB if masking would yield an
    out-of-range address, to fix kdump, as suggested by Ard.

v8:
  - Rebase on top of commit 893ab00439a45513 ("kbuild: remove cc-option
    test of -fno-stack-protector"),

v7:
  - Rebase on top of commit 161e04a5bae58a65 ("ARM: decompressor: split
    off _edata and stack base into separate object"),

v6:
  - Rebase on top of commit 7ae4a78daacf240a ("ARM: 8969/1:
    decompressor: simplify libfdt builds"),
  - Include <linux/libfdt.h> instead of <libfdt.h>,

v5:
  - Add Tested-by, Reviewed-by,
  - Round up start of memory to satisfy 16 MiB alignment rule,

v4:
  - Fix stack location after commit 184bf653a7a452c1 ("ARM:
    decompressor: factor out routine to obtain the inflated image
    size"),

v3:
  - Add Reviewed-by,
  - Fix ATAGs with appended DTB,
  - Add Tested-by,

v2:
  - Use "cmp r0, #-1", instead of "cmn r0, #1",
  - Add missing stack setup,
  - Support appended DTB.
---
 arch/arm/Kconfig                              |   7 +-
 arch/arm/boot/compressed/Makefile             |   5 +-
 .../arm/boot/compressed/fdt_check_mem_start.c | 131 ++++++++++++++++++
 arch/arm/boot/compressed/head.S               |  36 ++++-
 4 files changed, 172 insertions(+), 7 deletions(-)
 create mode 100644 arch/arm/boot/compressed/fdt_check_mem_start.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 138248999df7421e..9860057775ee72a9 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1875,9 +1875,10 @@ config AUTO_ZRELADDR
 	help
 	  ZRELADDR is the physical address where the decompressed kernel
 	  image will be placed. If AUTO_ZRELADDR is selected, the address
-	  will be determined at run-time by masking the current IP with
-	  0xf8000000. This assumes the zImage being placed in the first 128MB
-	  from start of memory.
+	  will be determined at run-time, either by masking the current IP
+	  with 0xf8000000, or, if invalid, from the DTB passed in r2.
+	  This assumes the zImage being placed in the first 128MB from
+	  start of memory.
 
 config EFI_STUB
 	bool
diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index fb521efcc6c20a4f..fd94e27ba4fa3d12 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -87,10 +87,13 @@ libfdt_objs := fdt_rw.o fdt_ro.o fdt_wip.o fdt.o
 ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y)
 OBJS	+= $(libfdt_objs) atags_to_fdt.o
 endif
+ifeq ($(CONFIG_USE_OF),y)
+OBJS	+= $(libfdt_objs) fdt_check_mem_start.o
+endif
 
 # -fstack-protector-strong triggers protection checks in this code,
 # but it is being used too early to link to meaningful stack_chk logic.
-$(foreach o, $(libfdt_objs) atags_to_fdt.o, \
+$(foreach o, $(libfdt_objs) atags_to_fdt.o fdt_check_mem_start.o, \
 	$(eval CFLAGS_$(o) := -I $(srctree)/scripts/dtc/libfdt -fno-stack-protector))
 
 # These were previously generated C files. When you are building the kernel
diff --git a/arch/arm/boot/compressed/fdt_check_mem_start.c b/arch/arm/boot/compressed/fdt_check_mem_start.c
new file mode 100644
index 0000000000000000..62450d824c3ca180
--- /dev/null
+++ b/arch/arm/boot/compressed/fdt_check_mem_start.c
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/kernel.h>
+#include <linux/libfdt.h>
+#include <linux/sizes.h>
+
+static const void *get_prop(const void *fdt, const char *node_path,
+			    const char *property, int minlen)
+{
+	const void *prop;
+	int offset, len;
+
+	offset = fdt_path_offset(fdt, node_path);
+	if (offset < 0)
+		return NULL;
+
+	prop = fdt_getprop(fdt, offset, property, &len);
+	if (!prop || len < minlen)
+		return NULL;
+
+	return prop;
+}
+
+static uint32_t get_cells(const void *fdt, const char *name)
+{
+	const fdt32_t *prop = get_prop(fdt, "/", name, sizeof(fdt32_t));
+
+	if (!prop) {
+		/* default */
+		return 1;
+	}
+
+	return fdt32_ld(prop);
+}
+
+static uint64_t get_val(const fdt32_t *cells, uint32_t ncells)
+{
+	uint64_t r;
+
+	r = fdt32_ld(cells);
+	if (ncells > 1)
+		r = (r << 32) | fdt32_ld(cells + 1);
+
+	return r;
+}
+
+/*
+ * Check the start of physical memory
+ *
+ * Traditionally, the start address of physical memory is obtained by masking
+ * the program counter.  However, this does require that this address is a
+ * multiple of 128 MiB, precluding booting Linux on platforms where this
+ * requirement is not fulfilled.
+ * Hence validate the calculated address against the memory information in the
+ * DTB, and, if out-of-range, replace it by the real start address.
+ * To preserve backwards compatibility (systems reserving a block of memory
+ * at the start of physical memory, kdump, ...), the traditional method is
+ * always used if it yields a valid address.
+ *
+ * Return value: start address of physical memory to use
+ */
+uint32_t fdt_check_mem_start(uint32_t mem_start, const void *fdt)
+{
+	uint32_t addr_cells, size_cells, base;
+	uint32_t fdt_mem_start = 0xffffffff;
+	const fdt32_t *reg, *endp;
+	uint64_t size, end;
+	const char *type;
+	int offset, len;
+
+	if (!fdt)
+		return mem_start;
+
+	if (fdt_magic(fdt) != FDT_MAGIC)
+		return mem_start;
+
+	/* There may be multiple cells on LPAE platforms */
+	addr_cells = get_cells(fdt, "#address-cells");
+	size_cells = get_cells(fdt, "#size-cells");
+	if (addr_cells > 2 || size_cells > 2)
+		return mem_start;
+
+	/* Walk all memory nodes and regions */
+	for (offset = fdt_next_node(fdt, -1, NULL); offset >= 0;
+	     offset = fdt_next_node(fdt, offset, NULL)) {
+		type = fdt_getprop(fdt, offset, "device_type", NULL);
+		if (!type || strcmp(type, "memory"))
+			continue;
+
+		reg = fdt_getprop(fdt, offset, "linux,usable-memory", &len);
+		if (!reg)
+			reg = fdt_getprop(fdt, offset, "reg", &len);
+		if (!reg)
+			continue;
+
+		for (endp = reg + (len / sizeof(fdt32_t));
+		     endp - reg >= addr_cells + size_cells;
+		     reg += addr_cells + size_cells) {
+			size = get_val(reg + addr_cells, size_cells);
+			if (!size)
+				continue;
+
+			if (addr_cells > 1 && fdt32_ld(reg)) {
+				/* Outside 32-bit address space, skipping */
+				continue;
+			}
+
+			base = fdt32_ld(reg + addr_cells - 1);
+			end = base + size;
+			if (mem_start >= base && mem_start < end) {
+				/* Calculated address is valid, use it */
+				return mem_start;
+			}
+
+			if (base < fdt_mem_start)
+				fdt_mem_start = base;
+		}
+	}
+
+	if (fdt_mem_start == 0xffffffff) {
+		/* No usable memory found, falling back to default */
+		return mem_start;
+	}
+
+	/*
+	 * The calculated address is not usable.
+	 * Use the lowest usable physical memory address from the DTB instead,
+	 * and make sure this is a multiple of 2 MiB for phys/virt patching.
+	 */
+	return round_up(fdt_mem_start, SZ_2M);
+}
diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index d9cce7238a365081..04f6b7bb7c43b66c 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -282,10 +282,40 @@ not_angel:
 		 * are already placing their zImage in (eg) the top 64MB
 		 * of this range.
 		 */
-		mov	r4, pc
-		and	r4, r4, #0xf8000000
+		mov	r0, pc
+		and	r0, r0, #0xf8000000
+#ifdef CONFIG_USE_OF
+		adr	r1, LC1
+#ifdef CONFIG_ARM_APPENDED_DTB
+		/*
+		 * Look for an appended DTB.  If found, we cannot use it to
+		 * validate the calculated start of physical memory, as its
+		 * memory nodes may need to be augmented by ATAGS stored at
+		 * an offset from the same start of physical memory.
+		 */
+		ldr	r2, [r1, #4]	@ get &_edata
+		add	r2, r2, r1	@ relocate it
+		ldr	r2, [r2]	@ get DTB signature
+		ldr	r3, =OF_DT_MAGIC
+		cmp	r2, r3		@ do we have a DTB there?
+		beq	1f		@ if yes, skip validation
+#endif /* CONFIG_ARM_APPENDED_DTB */
+
+		/*
+		 * Make sure we have some stack before calling C code.
+		 * No GOT fixup has occurred yet, but none of the code we're
+		 * about to call uses any global variables.
+		 */
+		ldr	sp, [r1]	@ get stack location
+		add	sp, sp, r1	@ apply relocation
+
+		/* Validate calculated start against passed DTB */
+		mov	r1, r8
+		bl	fdt_check_mem_start
+1:
+#endif /* CONFIG_USE_OF */
 		/* Determine final kernel image address. */
-		add	r4, r4, #TEXT_OFFSET
+		add	r4, r0, #TEXT_OFFSET
 #else
 		ldr	r4, =zreladdr
 #endif
-- 
2.25.1

Re: [PATCH v12] ARM: uncompress: Validate start of physical memory against passed DTB

[Marek Szyprowski]

Hi Geert,

On 04.01.2021 14:01, Geert Uytterhoeven wrote:
> Currently, the start address of physical memory is obtained by masking
> the program counter with a fixed mask of 0xf8000000.  This mask value
> was chosen as a balance between the requirements of different platforms.
> However, this does require that the start address of physical memory is
> a multiple of 128 MiB, precluding booting Linux on platforms where this
> requirement is not fulfilled.
>
> Fix this limitation by validating the masked address against the memory
> information in the passed DTB.  Only use the start address
> from DTB when masking would yield an out-of-range address, prefer the
> traditional method in all other cases.  Note that this applies only to the
> explicitly passed DTB on modern systems, and not to a DTB appended to
> the kernel, or to ATAGS.  The appended DTB may need to be augmented by
> information from ATAGS, which may need to rely on knowledge of the start
> address of physical memory itself.
>
> This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
> on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
> i.e. not at a multiple of 128 MiB.
>
> Suggested-by: Nicolas Pitre <nico@fluxnic.net>
> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> Acked-by: Nicolas Pitre <nico@fluxnic.net>

I've checked all of my arm 32bit test systems and they still boot fine 
with this patch. Feel free to add my:

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

although I didn't test exactly the new features added by it.

> ---
> Submitted to RMK's patch tracker.
>
> v12:
>    - Drop unneeded initialization of r in get_val(),
>
> v11:
>    - Add Reviewed-by, Acked-by,
>    - Document GOT fixup caveat,
>
> v10:
>    - Update for commit 9443076e4330a14a ("ARM: p2v: reduce p2v alignment
>      requirement to 2 MiB"),
>    - Use OF_DT_MAGIC macro,
>    - Rename fdt_get_mem_start() to fdt_check_mem_start(),
>    - Skip validation if there is an appended DTB,
>    - Pass mem_start to fdt_check_mem_start() to streamline code,
>    - Optimize register allocation,
>    - Update CONFIG_AUTO_ZRELADDR help text,
>    - Check all memory nodes and ranges (not just the first one), and
>      "linux,usable-memory", similar to early_init_dt_scan_memory(),
>    - Drop Reviewed-by, Tested-by tags,
>
> v9:
>    - Add minlen parameter to getprop(), for better validation of
>      memory properties,
>    - Only use start of memory from the DTB if masking would yield an
>      out-of-range address, to fix kdump, as suggested by Ard.
>
> v8:
>    - Rebase on top of commit 893ab00439a45513 ("kbuild: remove cc-option
>      test of -fno-stack-protector"),
>
> v7:
>    - Rebase on top of commit 161e04a5bae58a65 ("ARM: decompressor: split
>      off _edata and stack base into separate object"),
>
> v6:
>    - Rebase on top of commit 7ae4a78daacf240a ("ARM: 8969/1:
>      decompressor: simplify libfdt builds"),
>    - Include <linux/libfdt.h> instead of <libfdt.h>,
>
> v5:
>    - Add Tested-by, Reviewed-by,
>    - Round up start of memory to satisfy 16 MiB alignment rule,
>
> v4:
>    - Fix stack location after commit 184bf653a7a452c1 ("ARM:
>      decompressor: factor out routine to obtain the inflated image
>      size"),
>
> v3:
>    - Add Reviewed-by,
>    - Fix ATAGs with appended DTB,
>    - Add Tested-by,
>
> v2:
>    - Use "cmp r0, #-1", instead of "cmn r0, #1",
>    - Add missing stack setup,
>    - Support appended DTB.
> ---
>   arch/arm/Kconfig                              |   7 +-
>   arch/arm/boot/compressed/Makefile             |   5 +-
>   .../arm/boot/compressed/fdt_check_mem_start.c | 131 ++++++++++++++++++
>   arch/arm/boot/compressed/head.S               |  36 ++++-
>   4 files changed, 172 insertions(+), 7 deletions(-)
>   create mode 100644 arch/arm/boot/compressed/fdt_check_mem_start.c
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 138248999df7421e..9860057775ee72a9 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1875,9 +1875,10 @@ config AUTO_ZRELADDR
>   	help
>   	  ZRELADDR is the physical address where the decompressed kernel
>   	  image will be placed. If AUTO_ZRELADDR is selected, the address
> -	  will be determined at run-time by masking the current IP with
> -	  0xf8000000. This assumes the zImage being placed in the first 128MB
> -	  from start of memory.
> +	  will be determined at run-time, either by masking the current IP
> +	  with 0xf8000000, or, if invalid, from the DTB passed in r2.
> +	  This assumes the zImage being placed in the first 128MB from
> +	  start of memory.
>   
>   config EFI_STUB
>   	bool
> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> index fb521efcc6c20a4f..fd94e27ba4fa3d12 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -87,10 +87,13 @@ libfdt_objs := fdt_rw.o fdt_ro.o fdt_wip.o fdt.o
>   ifeq ($(CONFIG_ARM_ATAG_DTB_COMPAT),y)
>   OBJS	+= $(libfdt_objs) atags_to_fdt.o
>   endif
> +ifeq ($(CONFIG_USE_OF),y)
> +OBJS	+= $(libfdt_objs) fdt_check_mem_start.o
> +endif
>   
>   # -fstack-protector-strong triggers protection checks in this code,
>   # but it is being used too early to link to meaningful stack_chk logic.
> -$(foreach o, $(libfdt_objs) atags_to_fdt.o, \
> +$(foreach o, $(libfdt_objs) atags_to_fdt.o fdt_check_mem_start.o, \
>   	$(eval CFLAGS_$(o) := -I $(srctree)/scripts/dtc/libfdt -fno-stack-protector))
>   
>   # These were previously generated C files. When you are building the kernel
> diff --git a/arch/arm/boot/compressed/fdt_check_mem_start.c b/arch/arm/boot/compressed/fdt_check_mem_start.c
> new file mode 100644
> index 0000000000000000..62450d824c3ca180
> --- /dev/null
> +++ b/arch/arm/boot/compressed/fdt_check_mem_start.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/kernel.h>
> +#include <linux/libfdt.h>
> +#include <linux/sizes.h>
> +
> +static const void *get_prop(const void *fdt, const char *node_path,
> +			    const char *property, int minlen)
> +{
> +	const void *prop;
> +	int offset, len;
> +
> +	offset = fdt_path_offset(fdt, node_path);
> +	if (offset < 0)
> +		return NULL;
> +
> +	prop = fdt_getprop(fdt, offset, property, &len);
> +	if (!prop || len < minlen)
> +		return NULL;
> +
> +	return prop;
> +}
> +
> +static uint32_t get_cells(const void *fdt, const char *name)
> +{
> +	const fdt32_t *prop = get_prop(fdt, "/", name, sizeof(fdt32_t));
> +
> +	if (!prop) {
> +		/* default */
> +		return 1;
> +	}
> +
> +	return fdt32_ld(prop);
> +}
> +
> +static uint64_t get_val(const fdt32_t *cells, uint32_t ncells)
> +{
> +	uint64_t r;
> +
> +	r = fdt32_ld(cells);
> +	if (ncells > 1)
> +		r = (r << 32) | fdt32_ld(cells + 1);
> +
> +	return r;
> +}
> +
> +/*
> + * Check the start of physical memory
> + *
> + * Traditionally, the start address of physical memory is obtained by masking
> + * the program counter.  However, this does require that this address is a
> + * multiple of 128 MiB, precluding booting Linux on platforms where this
> + * requirement is not fulfilled.
> + * Hence validate the calculated address against the memory information in the
> + * DTB, and, if out-of-range, replace it by the real start address.
> + * To preserve backwards compatibility (systems reserving a block of memory
> + * at the start of physical memory, kdump, ...), the traditional method is
> + * always used if it yields a valid address.
> + *
> + * Return value: start address of physical memory to use
> + */
> +uint32_t fdt_check_mem_start(uint32_t mem_start, const void *fdt)
> +{
> +	uint32_t addr_cells, size_cells, base;
> +	uint32_t fdt_mem_start = 0xffffffff;
> +	const fdt32_t *reg, *endp;
> +	uint64_t size, end;
> +	const char *type;
> +	int offset, len;
> +
> +	if (!fdt)
> +		return mem_start;
> +
> +	if (fdt_magic(fdt) != FDT_MAGIC)
> +		return mem_start;
> +
> +	/* There may be multiple cells on LPAE platforms */
> +	addr_cells = get_cells(fdt, "#address-cells");
> +	size_cells = get_cells(fdt, "#size-cells");
> +	if (addr_cells > 2 || size_cells > 2)
> +		return mem_start;
> +
> +	/* Walk all memory nodes and regions */
> +	for (offset = fdt_next_node(fdt, -1, NULL); offset >= 0;
> +	     offset = fdt_next_node(fdt, offset, NULL)) {
> +		type = fdt_getprop(fdt, offset, "device_type", NULL);
> +		if (!type || strcmp(type, "memory"))
> +			continue;
> +
> +		reg = fdt_getprop(fdt, offset, "linux,usable-memory", &len);
> +		if (!reg)
> +			reg = fdt_getprop(fdt, offset, "reg", &len);
> +		if (!reg)
> +			continue;
> +
> +		for (endp = reg + (len / sizeof(fdt32_t));
> +		     endp - reg >= addr_cells + size_cells;
> +		     reg += addr_cells + size_cells) {
> +			size = get_val(reg + addr_cells, size_cells);
> +			if (!size)
> +				continue;
> +
> +			if (addr_cells > 1 && fdt32_ld(reg)) {
> +				/* Outside 32-bit address space, skipping */
> +				continue;
> +			}
> +
> +			base = fdt32_ld(reg + addr_cells - 1);
> +			end = base + size;
> +			if (mem_start >= base && mem_start < end) {
> +				/* Calculated address is valid, use it */
> +				return mem_start;
> +			}
> +
> +			if (base < fdt_mem_start)
> +				fdt_mem_start = base;
> +		}
> +	}
> +
> +	if (fdt_mem_start == 0xffffffff) {
> +		/* No usable memory found, falling back to default */
> +		return mem_start;
> +	}
> +
> +	/*
> +	 * The calculated address is not usable.
> +	 * Use the lowest usable physical memory address from the DTB instead,
> +	 * and make sure this is a multiple of 2 MiB for phys/virt patching.
> +	 */
> +	return round_up(fdt_mem_start, SZ_2M);
> +}
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index d9cce7238a365081..04f6b7bb7c43b66c 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -282,10 +282,40 @@ not_angel:
>   		 * are already placing their zImage in (eg) the top 64MB
>   		 * of this range.
>   		 */
> -		mov	r4, pc
> -		and	r4, r4, #0xf8000000
> +		mov	r0, pc
> +		and	r0, r0, #0xf8000000
> +#ifdef CONFIG_USE_OF
> +		adr	r1, LC1
> +#ifdef CONFIG_ARM_APPENDED_DTB
> +		/*
> +		 * Look for an appended DTB.  If found, we cannot use it to
> +		 * validate the calculated start of physical memory, as its
> +		 * memory nodes may need to be augmented by ATAGS stored at
> +		 * an offset from the same start of physical memory.
> +		 */
> +		ldr	r2, [r1, #4]	@ get &_edata
> +		add	r2, r2, r1	@ relocate it
> +		ldr	r2, [r2]	@ get DTB signature
> +		ldr	r3, =OF_DT_MAGIC
> +		cmp	r2, r3		@ do we have a DTB there?
> +		beq	1f		@ if yes, skip validation
> +#endif /* CONFIG_ARM_APPENDED_DTB */
> +
> +		/*
> +		 * Make sure we have some stack before calling C code.
> +		 * No GOT fixup has occurred yet, but none of the code we're
> +		 * about to call uses any global variables.
> +		 */
> +		ldr	sp, [r1]	@ get stack location
> +		add	sp, sp, r1	@ apply relocation
> +
> +		/* Validate calculated start against passed DTB */
> +		mov	r1, r8
> +		bl	fdt_check_mem_start
> +1:
> +#endif /* CONFIG_USE_OF */
>   		/* Determine final kernel image address. */
> -		add	r4, r4, #TEXT_OFFSET
> +		add	r4, r0, #TEXT_OFFSET
>   #else
>   		ldr	r4, =zreladdr
>   #endif

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v12] ARM: uncompress: Validate start of physical memory against passed DTB

[Geert Uytterhoeven]

Hi Marek,

On Thu, Jan 7, 2021 at 11:36 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> On 04.01.2021 14:01, Geert Uytterhoeven wrote:
> > Currently, the start address of physical memory is obtained by masking
> > the program counter with a fixed mask of 0xf8000000.  This mask value
> > was chosen as a balance between the requirements of different platforms.
> > However, this does require that the start address of physical memory is
> > a multiple of 128 MiB, precluding booting Linux on platforms where this
> > requirement is not fulfilled.
> >
> > Fix this limitation by validating the masked address against the memory
> > information in the passed DTB.  Only use the start address
> > from DTB when masking would yield an out-of-range address, prefer the
> > traditional method in all other cases.  Note that this applies only to the
> > explicitly passed DTB on modern systems, and not to a DTB appended to
> > the kernel, or to ATAGS.  The appended DTB may need to be augmented by
> > information from ATAGS, which may need to rely on knowledge of the start
> > address of physical memory itself.
> >
> > This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
> > on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
> > i.e. not at a multiple of 128 MiB.
> >
> > Suggested-by: Nicolas Pitre <nico@fluxnic.net>
> > Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > Acked-by: Nicolas Pitre <nico@fluxnic.net>
>
> I've checked all of my arm 32bit test systems and they still boot fine
> with this patch. Feel free to add my:
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
> although I didn't test exactly the new features added by it.

Thank you, regression-testing is very valuable!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v12] ARM: uncompress: Validate start of physical memory against passed DTB

[Linus Walleij]

On Mon, Jan 4, 2021 at 2:01 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> Currently, the start address of physical memory is obtained by masking
> the program counter with a fixed mask of 0xf8000000.  This mask value
> was chosen as a balance between the requirements of different platforms.
> However, this does require that the start address of physical memory is
> a multiple of 128 MiB, precluding booting Linux on platforms where this
> requirement is not fulfilled.
>
> Fix this limitation by validating the masked address against the memory
> information in the passed DTB.  Only use the start address
> from DTB when masking would yield an out-of-range address, prefer the
> traditional method in all other cases.  Note that this applies only to the
> explicitly passed DTB on modern systems, and not to a DTB appended to
> the kernel, or to ATAGS.  The appended DTB may need to be augmented by
> information from ATAGS, which may need to rely on knowledge of the start
> address of physical memory itself.
>
> This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
> on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
> i.e. not at a multiple of 128 MiB.
>
> Suggested-by: Nicolas Pitre <nico@fluxnic.net>
> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> Acked-by: Nicolas Pitre <nico@fluxnic.net>

Sorry for the long delay in reading the patch :(

I really like the looks of this now, moreover it is very useful.
I suppose it is already in the patch tracker, but for the record:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

> +               reg = fdt_getprop(fdt, offset, "linux,usable-memory", &len);

I suppose we already had a discussion of why this property
is undocumented? Or should we document it? Obviously
it is already in widespread use.

Yours,
Linus Walleij

Re: [PATCH v12] ARM: uncompress: Validate start of physical memory against passed DTB

[Geert Uytterhoeven]

Hi Linus,

CC Rob, Grant, Michael, Heinrich, DT

On Sat, Jan 9, 2021 at 12:57 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Jan 4, 2021 at 2:01 PM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > Currently, the start address of physical memory is obtained by masking
> > the program counter with a fixed mask of 0xf8000000.  This mask value
> > was chosen as a balance between the requirements of different platforms.
> > However, this does require that the start address of physical memory is
> > a multiple of 128 MiB, precluding booting Linux on platforms where this
> > requirement is not fulfilled.
> >
> > Fix this limitation by validating the masked address against the memory
> > information in the passed DTB.  Only use the start address
> > from DTB when masking would yield an out-of-range address, prefer the
> > traditional method in all other cases.  Note that this applies only to the
> > explicitly passed DTB on modern systems, and not to a DTB appended to
> > the kernel, or to ATAGS.  The appended DTB may need to be augmented by
> > information from ATAGS, which may need to rely on knowledge of the start
> > address of physical memory itself.
> >
> > This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
> > on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
> > i.e. not at a multiple of 128 MiB.
> >
> > Suggested-by: Nicolas Pitre <nico@fluxnic.net>
> > Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> > Acked-by: Nicolas Pitre <nico@fluxnic.net>
>
> Sorry for the long delay in reading the patch :(
>
> I really like the looks of this now, moreover it is very useful.
> I suppose it is already in the patch tracker, but for the record:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks a lot!

> > +               reg = fdt_getprop(fdt, offset, "linux,usable-memory", &len);
>
> I suppose we already had a discussion of why this property
> is undocumented? Or should we document it? Obviously
> it is already in widespread use.

This comes from commit 51975db0b7333cf3 ("of/flattree: merge
early_init_dt_scan_memory() common code"), which combined existing
practises on Microblaze (commit 12e8414263f47352 ("microblaze_v8: Open
firmware files")) and PowerPC (ba7594852f4e7121 ("[PATCH] powerpc: Add
support for "linux,usable-memory" on memory nodes")), with the former
obviously just copying the latter.
Unfortunately none of this is documented in The DeviceTree
Specification, ePAPR, or P1275.

Heinrich tried to document it, but his patch was ignored:
[PATCH] Documentation: devicetree: "linux,usable-memory" property
https://lore.kernel.org/linux-devicetree/20161223161747.9986-1-xypron.glpk@gmx.de/
https://lkml.org/lkml/2016/12/23/175
https://lore.kernel.org/patchwork/patch/745784/
Note that Heinrichs address is mangled in lore (imported from gmane?,
but lkml and patchwork have it right.


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v12] ARM: uncompress: Validate start of physical memory against passed DTB

[Linus Walleij]

On Mon, Jan 11, 2021 at 5:19 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > I suppose we already had a discussion of why this property
> > is undocumented? Or should we document it? Obviously
> > it is already in widespread use.
>
> This comes from commit 51975db0b7333cf3 ("of/flattree: merge
> early_init_dt_scan_memory() common code"), which combined existing
> practises on Microblaze (commit 12e8414263f47352 ("microblaze_v8: Open
> firmware files")) and PowerPC (ba7594852f4e7121 ("[PATCH] powerpc: Add
> support for "linux,usable-memory" on memory nodes")), with the former
> obviously just copying the latter.
> Unfortunately none of this is documented in The DeviceTree
> Specification, ePAPR, or P1275.
>
> Heinrich tried to document it, but his patch was ignored:
> [PATCH] Documentation: devicetree: "linux,usable-memory" property
> https://lore.kernel.org/linux-devicetree/20161223161747.9986-1-xypron.glpk@gmx.de/
> https://lkml.org/lkml/2016/12/23/175
> https://lore.kernel.org/patchwork/patch/745784/
> Note that Heinrichs address is mangled in lore (imported from gmane?,
> but lkml and patchwork have it right.

I bet it's just a mishap.
Rob, can you pick up and apply this patch?

Yours,
Linus Walleij

Re: [PATCH v12] ARM: uncompress: Validate start of physical memory against passed DTB

[Dmitry Osipenko]

04.01.2021 16:01, Geert Uytterhoeven пишет:
> Currently, the start address of physical memory is obtained by masking
> the program counter with a fixed mask of 0xf8000000.  This mask value
> was chosen as a balance between the requirements of different platforms.
> However, this does require that the start address of physical memory is
> a multiple of 128 MiB, precluding booting Linux on platforms where this
> requirement is not fulfilled.
> 
> Fix this limitation by validating the masked address against the memory
> information in the passed DTB.  Only use the start address
> from DTB when masking would yield an out-of-range address, prefer the
> traditional method in all other cases.  Note that this applies only to the
> explicitly passed DTB on modern systems, and not to a DTB appended to
> the kernel, or to ATAGS.  The appended DTB may need to be augmented by
> information from ATAGS, which may need to rely on knowledge of the start
> address of physical memory itself.
> 
> This allows to boot Linux on r7s9210/rza2mevb using the 64 MiB of SDRAM
> on the RZA2MEVB sub board, which is located at 0x0C000000 (CS3 space),
> i.e. not at a multiple of 128 MiB.
> 
> Suggested-by: Nicolas Pitre <nico@fluxnic.net>
> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> Acked-by: Nicolas Pitre <nico@fluxnic.net>
> ---
> Submitted to RMK's patch tracker.
> 
> v12:
>   - Drop unneeded initialization of r in get_val(),

I tested this patch on NVIDIA Tegra and haven't spotted any problems.

Tested-by: Dmitry Osipenko <digetx@gmail.com>