[PATCH 1/6 v14] ARM: Handle a device tree in lowmem

[Ard Biesheuvel]

Hi Linus,

Thanks for the elaborate explanation.

On Sun, 4 Oct 2020 at 22:50, Linus Walleij <linus.walleij at linaro.org> wrote:
>
> On Fri, Oct 2, 2020 at 1:01 PM Ard Biesheuvel <ardb at kernel.org> wrote:
> > On Thu, 1 Oct 2020 at 17:22, Linus Walleij <linus.walleij at linaro.org> wrote:
>
> > OK, so if I am understanding this correctly, the root problem is that
> > the kernel unmaps the memory that the attached DTB resides in, right?
>
> Yups, or, well in some places the kernel knows that the DT is there
> so it sets up two 1MB sections over it, and then it assumes "no-one
> is ever gonna touch those two section mappings, OK".
>

OK, so from your explanation, I gathered that:
- the 'appended DTB' is not appended anymore when the core kernel
boots, and the problem is caused by the fact that the decompressor
blindly chooses a location for it, whereas the firmware makes a more
informed decision when it places the DT in memory
- the memory holding the DT may be unmapped inadvertently
- the memory holding the DT may be wiped inadvertently.

...
> The kernel actually fits in the first memblock, but then it clears
> the lowmem right below itself and by that point the MMU
> has figured out that there is another memblock below it
> that it will happily use for lowmem and just goes ahead and
> wipes that. The code has no awareness
> that there might be a DTB there.
>

Where in the code does this happen? It seems to me that at this point,
the DT memory should have been memblock_reserve()d, and the code in
question should disregard it from wiping.

> The decompressor seems to have always been blissfully ignorant
> about what happens with the attached DTB after it just pushed
> it a bit upwards in memory (if relocating) it just passes the location
> in r2 in accordance with the boot specification, to me it seems
> more like something the kernel proper should handle.
>
> I don't think that loading the DTB separately to some high
> address as advocated by many peope is much better.
> It can create the same problem if loaded in the wrong
> place and possibly be placed in other dangerous areas
> like inside the VMALLOC area where it can get its
> mappings destroyed at any instance. (We don't check for that
> either.)
>

I think putting the mapping of the DT outside of the linear region is
reasonable, tbh, and this is what we do on arm64. That way, the
firmware does not have to care at all about the MM configuration of
the kernel, and it could simply put it in high memory as well.

One option would be to create a virtual mapping for the DT at the base
of the modules region. This takes up 1 MB in the typical (non-LPAE)
case, and 4 MB in the worst case, making it more likely that you will
run out of module space, but we already have infrastructure to deal
with that.

...

> So to summarize:
>
> - The kernel decompressor just moves the kernel and the
>   attached DTB upward in memory so it will not be
>   overwritten by the decompressor.
>
> - This will sometimes make part of the compressed kernel and
>   DTB end up above the first memblock.
>
> - This works because there is more memory above
>   the first memblock, in an adjacent memblock.
>   (The decompressor has always assumed so much)
>
> - The kernel then puts the lowmem mappings from
>   the end of the first memblock to VMALLOC_START
>   and clear the PMDs
>
> - If the DTB has been pushed up to lowmem, the two
>   PMDs over the DTB will be cleared, resulting in a crash.
>
> Maybe I should just put all of this into the commit
> message so people can see the mess :D
>

I'd prefer to fix this in a more structural way, tbh.

Let me see if I can code up a PoC