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

[Geert Uytterhoeven]

Hi Linus,

On Thu, Sep 3, 2020 at 10:49 PM Linus Walleij <linus.walleij at linaro.org> wrote:
> On Wed, Sep 2, 2020 at 5:36 PM Geert Uytterhoeven
> <geert+renesas at 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 at 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