[U-Boot] [PATCH] ARM: bootm: Allow booting in secure mode on hyp capable systems

[Siarhei Siamashka]

On Wed, 15 Oct 2014 13:42:33 +0100
Marc Zyngier <marc.zyngier at arm.com> wrote:

> On Wed, Oct 15 2014 at 11:40:24 AM, Siarhei Siamashka <siarhei.siamashka at gmail.com> wrote:
> > On Wed, 15 Oct 2014 11:31:44 +0100
> > Marc Zyngier <marc.zyngier at arm.com> wrote:
> >
> >> On Wed, Oct 15 2014 at 11:25:10 AM, Siarhei Siamashka
> >> <siarhei.siamashka at gmail.com> wrote:
> >> > On Wed, 15 Oct 2014 12:13:05 +0200
> >> > Hans de Goede <hdegoede at redhat.com> wrote:
> >> >
> >> >> Older Linux kernels will not properly boot in hype mode, add support for a
> >> >> bootm_boot_mode environment variable, which when set to "sec" will cause
> >> >> u-boot to boot in secure mode even when build with non-sec (and hyp) support.
> >> >> 
> >> >> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >> >> ---
> >> >>  arch/arm/lib/bootm.c | 23 ++++++++++++++++++-----
> >> >>  1 file changed, 18 insertions(+), 5 deletions(-)
> >> >> 
> >> >> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> >> >> index 39fe7a1..037fc8d 100644
> >> >> --- a/arch/arm/lib/bootm.c
> >> >> +++ b/arch/arm/lib/bootm.c
> >> >> @@ -235,6 +235,18 @@ static void boot_prep_linux(bootm_headers_t *images)
> >> >>  	}
> >> >>  }
> >> >>  
> >> >> +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
> >> >> +static bool boot_nonsec(void)
> >> >> +{
> >> >> +	char *s = getenv("bootm_boot_mode");
> >> >> +
> >> >> +	if (s && !strcmp(s, "sec"))
> >> >> +		return false;
> >> >> +
> >> >> +	return true;
> >> >> +}
> >> >> +#endif
> >> >> +
> >> >>  /* Subcommand: GO */
> >> >>  static void boot_jump_linux(bootm_headers_t *images, int flag)
> >> >>  {
> >> >> @@ -283,12 +295,13 @@ static void boot_jump_linux(bootm_headers_t *images, int flag)
> >> >>  
> >> >>  	if (!fake) {
> >> >>  #if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
> >> >> -		armv7_init_nonsec();
> >> >> -		secure_ram_addr(_do_nonsec_entry)(kernel_entry,
> >> >> -						  0, machid, r2);
> >> >> -#else
> >> >> -		kernel_entry(0, machid, r2);
> >> >> +		if (boot_nonsec()) {
> >> >> +			armv7_init_nonsec();
> >> >> +			secure_ram_addr(_do_nonsec_entry)(kernel_entry,
> >> >> +							  0, machid, r2);
> >> >> +		}
> >> >>  #endif
> >> >> +		kernel_entry(0, machid, r2);
> >> >>  	}
> >> >>  #endif
> >> >>  }
> >> >
> >> > BTW, I'm currently achieving the same result, but without the need to do
> >> > extra environment configuration gymnastics for the end users:
> >> >     https://github.com/ssvb/u-boot-sunxi-dram/commit/e914abe551e712a59a8fc1ac9a4b8490e8e7c866
> >> 
> >> And thus preventing kernels with appended DT (not provided by u-boot),
> >> but still using HYP/non-sec/PSCI from booting.
> >> 
> >> Sorry, but using DT as a way to distinguish between all these cases is
> >> just wrong.
> >> 
> >> Thanks,
> >
> > Do you see the HACK prefix in the summary of that patch?
> 
> I do. But the fact that you even mention it here makes me think that you
> believe it is a better approach that Hans'.

The need to meddle with the environment variables makes Hans' approach
immensely less useful for the end users.

In your Ivory Tower, you may think that you have resolved the problem.
But in reality, u-boot will still have to be patched before deploying
it to the end users in order to prevent them from running into troubles.

Other than this, are you really happy about granting the users full
rights to allow booting the kernel in the secure mode via a simple
environment variables tweak? Can't it potentially become a security
breach in some scenarios?

> > Or are you saying that it is really impossible to distinguish your
> > use case of having the appended DT without resorting to the use of the
> > environment config options?
> 
> Think of it. How do you find out about what the kernel wants? This is
> just a blob...

The FDT blob has a header with an easily recognisable signature. So we
can see the difference between the FDT and FEX blobs if the blob is
provided to u-boot. And if no blob is provided at all, then we are sure
that it can't be booted by the sunxi-3.4 kernel.

I can see only one theoretically problematic scenario, where u-boot is
provided with the non-FDT and non-FEX blob, but loads a kernel, which
has FDT statically compiled in. How does this actually play with PSCI?
And what about the new device drivers model, which is going to depend
on FDT information itself? Are we really happy allowing to use different
FDT blobs for the u-boot and the kernel in the same system?

Or have I missed something?

Either way, following the least surprise principle, IMHO u-boot should
log the reason for making a decision about whether it is switching to
the non-secure mode or not. This is useful for troubleshooting.

-- 
Best regards,
Siarhei Siamashka