Message-ID: <ac0534138facc25c4cbcbbff68fc0ba3c2de87b6.camel@linux.intel.com>
Date:   Wed, 27 May 2020 13:31:04 +0000
From:   "Alessandrelli, Daniele" <daniele.alessandrelli@...el.com>
To:     "pavel@....cz" <pavel@....cz>, "arnd@...db.de" <arnd@...db.de>
CC:     "robh@...nel.org" <robh@...nel.org>,
        "Murphy, Paul J" <paul.j.murphy@...el.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "Shevchenko, Andriy" <andriy.shevchenko@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "daniele.alessandrelli@...ux.intel.com" 
        <daniele.alessandrelli@...ux.intel.com>
Subject: Re: [PATCH 1/1] soc: keembay: Add Keem Bay IMR driver

Hi Arnd, Pavel,

Thanks for your feedback.

> > > 
> > > The problem is that I need this code to be run early at boot, so
> > > I
> > > don't think I can make this a module.
> > 
> > How early is early enough?

Basically, before any device with direct memory access is activated
(because if anybody, except the ARM CPU, tries to access that memory,
the memory protection mechanism will be triggered and a reboot will
happen).

> > 
> > What bootloader are you using?

U-Boot

> > 
> > I believe you should simply fix your bootloader not to pass locked
> > memory to the kernel.

The bootloader is behaving like that for security reasons, so we'd like
to avoid modifying it. I'll provide more information below.

> > 
> > Alternatively, take that memory off the "memory available" maps,
> > and only re-add it once
> > it is usable.
> > 
> > Anything else is dangerous.

That sounds like an interesting solution, thanks!

Do you know any code that I can use as a reference?

Since, the protected memory is just a few megabytes large, I guess that
in theory we could just have U-Boot mark it as reserved memory and
forget about it; but if there's a way to re-add it back once in Linux
that would be great.

> 
> Agreed, this sounds like an incompatible extension of the boot
> protocol
> that we should otherwise not merge.
> 
> However, there is also a lot of missing information here, and it is
> always
> possible they are trying to something for a good reason. As long as
> the
> problem that the bootloader is trying to solve is explained well
> enough
> in the changelog, we can discuss it to see how it should be done
> properly.


Apologies, I should have provided more information. Here it is :)

Basically, at boot time U-Boot code and core memory (.text, .data,
.bss, etc.) is protected by this Isolated Memory Region (IMR) which
prevents any device or processing units other than the ARM CPU to
access/modify the memory.

This is done for security reasons, to reduce the risks that a potential
attacker can use "hijacked" HW devices to interfere with the boot
process (and break the secure boot flow in place).

Before booting the Kernel, U-Boot sets up a new IMR to protect the
Kernel image (so that the kernel can benefit of a similar protection)
and then starts the kernel, delegating to the kernel the task of
switching off the U-Boot IMR.

U-Boot doesn't turn off its own IMR because doing so would leave a
(tiny) window in which the boot execution flow is not protected.

If you have any additional questions or feedback, just let me know.

Regards,
Daniele








--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

Powered by blists - more mailing lists