diff mbox series

[v9] ARM: boot: Validate start of physical memory against DTB

Message ID 20200902153606.13652-1-geert+renesas@glider.be
State Under Review
Delegated to: Geert Uytterhoeven
Headers show
Series [v9] ARM: boot: Validate start of physical memory against DTB | expand

Commit Message

Geert Uytterhoeven Sept. 2, 2020, 3:36 p.m. UTC
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 first
memory node in the DTB, if available  (either explicitly passed, or
appended to the kernel).  Only use the start address from DTB when
masking would yield an out-of-range address.  Prefer the traditional
method in all other cases.

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: Nicolas Pitre <nico@fluxnic.net>
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
Cc: Lukasz Stelmach <l.stelmach@samsung.com>
---
This is v9 of "ARM: boot: Obtain start of physical memory from DTB".

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 crashkernel, 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/boot/compressed/Makefile            |  5 +-
 arch/arm/boot/compressed/fdt_get_mem_start.c | 72 ++++++++++++++++++++
 arch/arm/boot/compressed/head.S              | 52 +++++++++++++-
 3 files changed, 127 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/boot/compressed/fdt_get_mem_start.c

Comments

Linus Walleij Sept. 3, 2020, 8:49 p.m. UTC | #1
Hi Geert,

I am trying to understand this thing, it looks useful!

On Wed, Sep 2, 2020 at 5:36 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> @@ -254,8 +254,56 @@ not_angel:

This looks like it happens before CONFIG_ARM_ATAG_DTB_COMPAT
meaning it will not use the DTB that is augmented with ATAGs,
especially not ATAG_MEM (0x54410002) correct?

That seems sad, because that would actually be useful to me.

Can you see if it is possible to put this in after the ATAGs have
been merged into the DTB?

>  #ifdef CONFIG_AUTO_ZRELADDR
> +#ifdef CONFIG_USE_OF
>                 /*
> -                * Find the start of physical memory.  As we are executing
> +                * Find the start of physical memory.
> +                * Try the DTB first, if available.
> +                */
> +               adr     r0, LC1
> +               ldr     sp, [r0]        @ get stack location
> +               add     sp, sp, r0      @ apply relocation
> +
> +#ifdef CONFIG_ARM_APPENDED_DTB
> +               /*
> +                * Look for an appended DTB. If found, use it and
> +                * move stack away from it.
> +                */
> +               ldr     r6, [r0, #4]    @ get &_edata
> +               add     r6, r6, r0      @ relocate it
> +               ldmia   r6, {r0, r5}    @ get DTB signature and size
> +#ifndef __ARMEB__
> +               ldr     r1, =0xedfe0dd0 @ sig is 0xd00dfeed big endian
> +               /* convert DTB size to little endian */
> +               eor     r2, r5, r5, ror #16
> +               bic     r2, r2, #0x00ff0000
> +               mov     r5, r5, ror #8
> +               eor     r5, r5, r2, lsr #8
> +#else
> +               ldr     r1, =0xd00dfeed
> +#endif

We now have two and even three copies of this code,
sort of. Right above  CONFIG_ARM_ATAG_DTB_COMPAT
there is this:

#ifdef CONFIG_ARM_APPENDED_DTB
(...)
        ldr    lr, [r6, #0]
#ifndef __ARMEB__
        ldr    r1, =0xedfe0dd0        @ sig is 0xd00dfeed big endian
#else
        ldr    r1, =0xd00dfeed
#endif
        cmp    lr, r1
        bne    dtb_check_done        @ not found

Then inside CONFIG_ARM_ATAG_DTB_COMPAT:

        /* Get the initial DTB size */
        ldr    r5, [r6, #4]
#ifndef __ARMEB__
        /* convert to little endian */
        eor    r1, r5, r5, ror #16
        bic    r1, r1, #0x00ff0000
        mov    r5, r5, ror #8
        eor    r5, r5, r1, lsr #8
#endif

Then at the end after deciding to use the appended
device tree we get the DTB size *again* and moves
the sp beyond the DTB permanently as we do not
want to damage the DTB of course.

To me it looks like the DTB size gets added to sp
a second time? First it is bumped by your code,
then when the appended DTB is detected and
augmented further down, it gets bumped again
for no reason here:

/* relocate some pointers past the appended dtb */
add    r6, r6, r5
add    r10, r10, r5
add    sp, sp, r5
dtb_check_done:

I don't know if I'm right, I may be confused...

It would be better if we could avoid copy/paste and
merge the code in here so we first check if there is a
DTB, else there is not much to do, and if there is, we
get the size, move the sp and do both operations:

1. Check for ATAGs and augment the DTB
2. Check for memory size in the DTB

This way the ATAG-augmented DTB can be used
for checking for the memory start.

I understand that you need the physical address
before turning on the caches, so what I am thinking
is if it is possible to move the check for appended
DTB and ATAG augmentation business up before
the cache enablement in a separate patch and then
modify it from there. Then you could potentially
merge these two things.

Maybe there is something conceptually wrong with
this that I have overlooked... :/

Yours,
Linus Walleij
Geert Uytterhoeven Sept. 8, 2020, 6:55 a.m. UTC | #2
Hi Linus,

On Thu, Sep 3, 2020 at 10:49 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> I am trying to understand this thing, it looks useful!

Thanks for your (initial ;-) comments!

> On Wed, Sep 2, 2020 at 5:36 PM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > @@ -254,8 +254,56 @@ not_angel:
>
> This looks like it happens before CONFIG_ARM_ATAG_DTB_COMPAT
> meaning it will not use the DTB that is augmented with ATAGs,
> especially not ATAG_MEM (0x54410002) correct?
>
> That seems sad, because that would actually be useful to me.
>
> Can you see if it is possible to put this in after the ATAGs have
> been merged into the DTB?

Right, this is done before the DTB is augmented with ATAGs.
Hence if the memory node in DT is bogus, my validation code may
do the wrong thing, and return a bogus address, too :-(

> >  #ifdef CONFIG_AUTO_ZRELADDR
> > +#ifdef CONFIG_USE_OF
> >                 /*
> > -                * Find the start of physical memory.  As we are executing
> > +                * Find the start of physical memory.
> > +                * Try the DTB first, if available.
> > +                */
> > +               adr     r0, LC1
> > +               ldr     sp, [r0]        @ get stack location
> > +               add     sp, sp, r0      @ apply relocation
> > +
> > +#ifdef CONFIG_ARM_APPENDED_DTB
> > +               /*
> > +                * Look for an appended DTB. If found, use it and
> > +                * move stack away from it.
> > +                */
> > +               ldr     r6, [r0, #4]    @ get &_edata
> > +               add     r6, r6, r0      @ relocate it
> > +               ldmia   r6, {r0, r5}    @ get DTB signature and size
> > +#ifndef __ARMEB__
> > +               ldr     r1, =0xedfe0dd0 @ sig is 0xd00dfeed big endian
> > +               /* convert DTB size to little endian */
> > +               eor     r2, r5, r5, ror #16
> > +               bic     r2, r2, #0x00ff0000
> > +               mov     r5, r5, ror #8
> > +               eor     r5, r5, r2, lsr #8
> > +#else
> > +               ldr     r1, =0xd00dfeed
> > +#endif
>
> We now have two and even three copies of this code,
> sort of.

Indeed.

> Then at the end after deciding to use the appended
> device tree we get the DTB size *again* and moves
> the sp beyond the DTB permanently as we do not
> want to damage the DTB of course.
>
> To me it looks like the DTB size gets added to sp
> a second time? First it is bumped by your code,
> then when the appended DTB is detected and
> augmented further down, it gets bumped again
> for no reason here:
>
> /* relocate some pointers past the appended dtb */
> add    r6, r6, r5
> add    r10, r10, r5
> add    sp, sp, r5
> dtb_check_done:
>
> I don't know if I'm right, I may be confused...

Before that, it has started with a "fresh" stack pointer:

    restart:        adr     r0, LC1
                    ldr     sp, [r0]
                    ldr     r6, [r0, #4]
                    add     sp, sp, r0

So the addition is done only once (ignoring the "temporarily
relocate..." cakewalk, doing "add sp, sp, r5", and "sub sp, sp, r5"
later).

> It would be better if we could avoid copy/paste and
> merge the code in here so we first check if there is a
> DTB, else there is not much to do, and if there is, we
> get the size, move the sp and do both operations:
>
> 1. Check for ATAGs and augment the DTB
> 2. Check for memory size in the DTB
>
> This way the ATAG-augmented DTB can be used
> for checking for the memory start.
>
> I understand that you need the physical address
> before turning on the caches, so what I am thinking
> is if it is possible to move the check for appended
> DTB and ATAG augmentation business up before
> the cache enablement in a separate patch and then
> modify it from there. Then you could potentially
> merge these two things.
>
> Maybe there is something conceptually wrong with
> this that I have overlooked... :/

I agree there are plenty of opportunities to improve of head.S.
Unfortunately there are also plenty of opportunities to break someone's
boot process ;-(

Nicolas' patch to reshuffle the registers looks like a good first step...

Gr{oetje,eeting}s,

                        Geert
Linus Walleij Sept. 16, 2020, 8:24 a.m. UTC | #3
On Tue, Sep 8, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> I agree there are plenty of opportunities to improve of head.S.
> Unfortunately there are also plenty of opportunities to break someone's
> boot process ;-(
>
> Nicolas' patch to reshuffle the registers looks like a good first step...

I must have missed this patch! I'll try to find it.

Yours,
Linus Walleij
Geert Uytterhoeven Sept. 16, 2020, 8:55 a.m. UTC | #4
Hi Linus,

On Wed, Sep 16, 2020 at 10:24 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Sep 8, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > I agree there are plenty of opportunities to improve of head.S.
> > Unfortunately there are also plenty of opportunities to break someone's
> > boot process ;-(
> >
> > Nicolas' patch to reshuffle the registers looks like a good first step...
>
> I must have missed this patch! I'll try to find it.

https://lore.kernel.org/linux-arm-kernel/nycvar.YSQ.7.78.906.2009041431440.4095746@knanqh.ubzr/

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Nov. 19, 2020, 12:46 p.m. UTC | #5
On Wed, Sep 16, 2020 at 10:55 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Sep 16, 2020 at 10:24 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Sep 8, 2020 at 8:55 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > I agree there are plenty of opportunities to improve of head.S.
> > > Unfortunately there are also plenty of opportunities to break someone's
> > > boot process ;-(
> > >
> > > Nicolas' patch to reshuffle the registers looks like a good first step...
> >
> > I must have missed this patch! I'll try to find it.
>
> https://lore.kernel.org/linux-arm-kernel/nycvar.YSQ.7.78.906.2009041431440.4095746@knanqh.ubzr/

FTR: that one is not for boot/compressed/head.S, but for the normal
head.S, so it doesn't help...

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Nov. 25, 2020, 3:28 p.m. UTC | #6
Hi Linus,

On Thu, Sep 3, 2020 at 10:49 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Sep 2, 2020 at 5:36 PM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > @@ -254,8 +254,56 @@ not_angel:
>
> This looks like it happens before CONFIG_ARM_ATAG_DTB_COMPAT
> meaning it will not use the DTB that is augmented with ATAGs,
> especially not ATAG_MEM (0x54410002) correct?
>
> That seems sad, because that would actually be useful to me.
>
> Can you see if it is possible to put this in after the ATAGs have
> been merged into the DTB?

I made a deep dive into arch/arm/boot/compressed/head.S, to analyze all
possible combinations of the various options (your article [1] was a
great help, thanks for that!).  I considered all of the following:

  - Passed by bootloader in r2:
      [A] ATAGS,
      [B] DTB (with proper memory nodes),
      [C] Rubbish.

  - Optional appended DTB (CONFIG_ARM_APPENDED_DTB=y):
      [D] DTB (with proper memory nodes),
      [E] DTB (without memory nodes).
    Notes:
      - The appended DTB takes precedence over the DTB passed via r2
        (case [B])!
      - This is meant as a backward compatibility convenience for
        systems with a bootloader that can't be upgraded to accommodate
        the documented boot protocol using a device tree.

  - ATAGS to augment appended DTB (CONFIG_ARM_ATAG_DTB_COMPAT=y):
      [F] Passed via r2,
      [G] Stored at start of memory + 0x100.
    Notes:
      - [F] is tried first; it it fails, [H] is tried next,
      - This is meant as another backward compatibility convenience for
        systems with an old bootloader,

  - [H] The Kdump kernel is deliberately not stored in the first 128 MiB
    of RAM.

Of course not all combinations are possible/sensible.

Note that there exists another option (handover from EFI), which is not
relevant here, as it follows a different code path, and passes the image
base explicitly.

> It would be better if we could avoid copy/paste and
> merge the code in here so we first check if there is a
> DTB, else there is not much to do, and if there is, we
> get the size, move the sp and do both operations:
>
> 1. Check for ATAGs and augment the DTB
> 2. Check for memory size in the DTB
>
> This way the ATAG-augmented DTB can be used
> for checking for the memory start.
>
> I understand that you need the physical address
> before turning on the caches, so what I am thinking
> is if it is possible to move the check for appended
> DTB and ATAG augmentation business up before
> the cache enablement in a separate patch and then
> modify it from there. Then you could potentially
> merge these two things.
>
> Maybe there is something conceptually wrong with
> this that I have overlooked... :/

Augmenting the appended DTB (case [E]) with information passed in ATAGS
via r2 (case [F]) can indeed be moved up.
However, augmenting the appended DTB with information stored in ATAGS at
the start of physical RAM + 0x100 (case [G]) cannot be moved up, as it
relies on knowing the start of memory.   Hence this can only be done on
systems where the start of RAM is a multiple of 128 MiB, and masking the
PC with 0xf8000000 yields a valid RAM address, thus leading to a
chicken-and-egg problem...

Given the appended DTB, and augmenting it with information from ATAGS,
is clearly marked as a backward compatibility convenience for systems
with a bootloader that cannot be upgraded, I think it is reasonable to
take a step back, and limit the validation to modern systems which do
pass the DTB in r2.  This would simplify the code, and avoid
regressions.

Does that sound right?

Later, we can easily add on top support for kdump adding a
"linux,usable-memory-range" property to the DTB (passed in r2), cfr. [2]
and [3].

Thanks for your comments!

[1] https://people.kernel.org/linusw/how-the-arm32-linux-kernel-decompresses
[2] "[PATCH] ARM: Parse kdump DT properties"
    (https://lore.kernel.org/r/20200902154538.6807-1-geert+renesas@glider.be)
[3] "PATCH] arm: kdump: Add DT properties to crash dump kernel's DTB"
    (https://lore.kernel.org/r/20200902154129.6358-1-geert+renesas@glider.be).


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
diff mbox series

Patch

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index b1147b7f2c8d372e..b1c09faf276e9193 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -81,10 +81,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_get_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_get_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_get_mem_start.c b/arch/arm/boot/compressed/fdt_get_mem_start.c
new file mode 100644
index 0000000000000000..fd501ec3c14b4ae4
--- /dev/null
+++ b/arch/arm/boot/compressed/fdt_get_mem_start.c
@@ -0,0 +1,72 @@ 
+// 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 == -FDT_ERR_NOTFOUND)
+		return NULL;
+
+	prop = fdt_getprop(fdt, offset, property, &len);
+	if (!prop || len < minlen)
+		return NULL;
+
+	return prop;
+}
+
+static uint32_t get_size(const void *fdt, const char *name)
+{
+	const __be32 *prop = get_prop(fdt, "/", name, 4);
+
+	if (!prop) {
+		/* default */
+		return 1;
+	}
+
+	return fdt32_to_cpu(*prop);
+}
+
+/*
+ * Get the start of physical memory
+ */
+
+unsigned long fdt_get_mem_start(const void *fdt)
+{
+	uint32_t addr_size, size_size, mem_start, masked_pc;
+	const __be32 *memory;
+
+	if (!fdt)
+		return -1;
+
+	if (*(__be32 *)fdt != cpu_to_fdt32(FDT_MAGIC))
+		return -1;
+
+	/* There may be multiple cells on LPAE platforms */
+	addr_size = get_size(fdt, "#address-cells");
+	size_size = get_size(fdt, "#size-cells");
+
+	/* Find the first memory node */
+	memory = get_prop(fdt, "/memory", "reg", addr_size + size_size);
+	if (!memory)
+		return -1;
+
+	mem_start = fdt32_to_cpu(memory[addr_size - 1]);
+
+	/* Must be a multiple of 16 MiB for phys/virt patching */
+	mem_start = round_up(mem_start, SZ_16M);
+
+	/*
+	 * Traditionally, the start of memory is obtained by masking the
+	 * program counter.  Prefer that method, unless it would yield an
+	 * out-of-range address.
+	 */
+	masked_pc = _RET_IP_ & 0xf8000000;
+	return masked_pc < mem_start ? mem_start : masked_pc;
+}
diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index 434a16982e344fe4..802621756ac0480b 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -254,8 +254,56 @@  not_angel:
 		.text
 
 #ifdef CONFIG_AUTO_ZRELADDR
+#ifdef CONFIG_USE_OF
 		/*
-		 * Find the start of physical memory.  As we are executing
+		 * Find the start of physical memory.
+		 * Try the DTB first, if available.
+		 */
+		adr	r0, LC1
+		ldr	sp, [r0]	@ get stack location
+		add	sp, sp, r0	@ apply relocation
+
+#ifdef CONFIG_ARM_APPENDED_DTB
+		/*
+		 * Look for an appended DTB. If found, use it and
+		 * move stack away from it.
+		 */
+		ldr	r6, [r0, #4]	@ get &_edata
+		add	r6, r6, r0	@ relocate it
+		ldmia	r6, {r0, r5}	@ get DTB signature and size
+#ifndef __ARMEB__
+		ldr	r1, =0xedfe0dd0	@ sig is 0xd00dfeed big endian
+		/* convert DTB size to little endian */
+		eor	r2, r5, r5, ror #16
+		bic	r2, r2, #0x00ff0000
+		mov	r5, r5, ror #8
+		eor	r5, r5, r2, lsr #8
+#else
+		ldr	r1, =0xd00dfeed
+#endif
+		cmp	r0, r1		@ do we have a DTB there?
+		bne	1f
+
+		/* preserve 64-bit alignment */
+		add	r5, r5, #7
+		bic	r5, r5, #7
+		add	sp, sp, r5	@ if so, move stack above DTB
+		mov	r0, r6		@ and extract memory start from DTB
+		b	2f
+
+1:
+#endif /* CONFIG_ARM_APPENDED_DTB */
+
+		mov	r0, r8
+2:
+		bl	fdt_get_mem_start
+		mov	r4, r0
+		cmp	r0, #-1
+		bne	1f
+#endif /* CONFIG_USE_OF */
+
+		/*
+		 * Fall back to the traditional method.  As we are executing
 		 * without the MMU on, we are in the physical address space.
 		 * We just need to get rid of any offset by aligning the
 		 * address.
@@ -273,6 +321,8 @@  not_angel:
 		 */
 		mov	r4, pc
 		and	r4, r4, #0xf8000000
+
+1:
 		/* Determine final kernel image address. */
 		add	r4, r4, #TEXT_OFFSET
 #else