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

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

[Hans de Goede]

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@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
 }
-- 
2.1.0

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

[Marc Zyngier]

On Wed, Oct 15 2014 at 11:13:05 AM, Hans de Goede <hdegoede@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@redhat.com>

Looks good to me.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny.

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

[Albert ARIBAUD]

Hi Marc, Hans,

On Wed, 15 Oct 2014 11:18:28 +0100, Marc Zyngier <marc.zyngier@arm.com>
wrote:

> On Wed, Oct 15 2014 at 11:13:05 AM, Hans de Goede <hdegoede@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@redhat.com>
> 
> Looks good to me.
> 
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> 	M.

Should we consider this a bugfix? For instance, hHow old are these
"older kernels"?

Amicalement,
-- 
Albert.

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

[Marc Zyngier]

On Wed, Oct 15 2014 at 11:25:43 AM, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
> Hi Marc, Hans,
>
> On Wed, 15 Oct 2014 11:18:28 +0100, Marc Zyngier <marc.zyngier@arm.com>
> wrote:
>
>> On Wed, Oct 15 2014 at 11:13:05 AM, Hans de Goede <hdegoede@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@redhat.com>
>> 
>> Looks good to me.
>> 
>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>> 
>> 	M.
>
> Should we consider this a bugfix? For instance, hHow old are these
> "older kernels"?

No idea. Probably in the low 3.x range, for x <= 4. Not sure if that's a
bug fix though...

	M.
-- 
Jazz is not dead. It just smells funny.

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

[Siarhei Siamashka]

On Wed, 15 Oct 2014 12:13:05 +0200
Hans de Goede <hdegoede@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@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

-- 
Best regards,
Siarhei Siamashka

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

[Marc Zyngier]

On Wed, Oct 15 2014 at 11:25:10 AM, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:
> On Wed, 15 Oct 2014 12:13:05 +0200
> Hans de Goede <hdegoede@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@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,

	M.
-- 
Jazz is not dead. It just smells funny.

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

[Siarhei Siamashka]

On Wed, 15 Oct 2014 11:31:44 +0100
Marc Zyngier <marc.zyngier@arm.com> wrote:

> On Wed, Oct 15 2014 at 11:25:10 AM, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:
> > On Wed, 15 Oct 2014 12:13:05 +0200
> > Hans de Goede <hdegoede@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@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?

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?

-- 
Best regards,
Siarhei Siamashka

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

[Marc Zyngier]

On Wed, Oct 15 2014 at 11:40:24 AM, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:
> On Wed, 15 Oct 2014 11:31:44 +0100
> Marc Zyngier <marc.zyngier@arm.com> wrote:
>
>> On Wed, Oct 15 2014 at 11:25:10 AM, Siarhei Siamashka
>> <siarhei.siamashka@gmail.com> wrote:
>> > On Wed, 15 Oct 2014 12:13:05 +0200
>> > Hans de Goede <hdegoede@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@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'.

> 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...

	M.
-- 
Jazz is not dead. It just smells funny.

[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@arm.com> wrote:

> On Wed, Oct 15 2014 at 11:40:24 AM, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:
> > On Wed, 15 Oct 2014 11:31:44 +0100
> > Marc Zyngier <marc.zyngier@arm.com> wrote:
> >
> >> On Wed, Oct 15 2014 at 11:25:10 AM, Siarhei Siamashka
> >> <siarhei.siamashka@gmail.com> wrote:
> >> > On Wed, 15 Oct 2014 12:13:05 +0200
> >> > Hans de Goede <hdegoede@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@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

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

[Mark Rutland]

[...]

> 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?

U-Boot must be running in secure mode in order to boot a kernel in
secure mode. If U-Boot has been placed in secure mode with such an
option, there is obviously nothing in the secure world to protect. As
the user is in charge of booting the kernel, there is nothing in the
normal world to protect.

There is no security breach here.

> > > 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.

FEX vs DT is specific to sunxi, whereas an explcit boot mode option is
more generally useful. It is possible to have a kernel which can boot in
either mode, where the security state the kernel runs in is a user
choice, regardless of the presence or absence of a DTB.

Trying to guess how an OS will react and working around that is only
going to cause problems when that OS changes over time.

> 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?

It would be completely orthogonal, just as the presence or absence of a
DTB is orthogonal to the presence or absence of 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?

There are already differences between what U-Boot needs to know and the
kernel needs to know, e.g. secure peripherals if the kernel is booted in
a non-secure mode. So in general you might need separate DTBs; the
physical address spaces are different.

> 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.

Printing a message would make sense regardless of how the mode is
selected.

Thanks,
Mark.

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

[Siarhei Siamashka]

On Wed, 15 Oct 2014 15:39:32 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> [...]
> 
> > 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?
> 
> U-Boot must be running in secure mode in order to boot a kernel in
> secure mode. If U-Boot has been placed in secure mode with such an
> option, there is obviously nothing in the secure world to protect. As
> the user is in charge of booting the kernel, there is nothing in the
> normal world to protect.
> 
> There is no security breach here.

OK, it's good to know that this change does not increase the attack
surface.

> > > > 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.
> 
> FEX vs DT is specific to sunxi, whereas an explcit boot mode option is
> more generally useful. It is possible to have a kernel which can boot in
> either mode, where the security state the kernel runs in is a user
> choice, regardless of the presence or absence of a DTB.

Indeed. The patch from Hans primarily targets sunxi hardware, but
clearly has a broader scope than just this.

> Trying to guess how an OS will react and working around that is only
> going to cause problems when that OS changes over time.

True, but in some cases we are the ones who can control and/or predict
the evolution of both the OS and the u-boot development. I believe
that's exactly the case with the community maintained legacy 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?
> 
> It would be completely orthogonal, just as the presence or absence of a
> DTB is orthogonal to the presence or absence of 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?
> 
> There are already differences between what U-Boot needs to know and the
> kernel needs to know, e.g. secure peripherals if the kernel is booted in
> a non-secure mode. So in general you might need separate DTBs; the
> physical address spaces are different.

Sounds like this may be a hell to maintain and keep working nicely
together

> > 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.
> 
> Printing a message would make sense regardless of how the mode is
> selected.
> 
> Thanks,
> Mark.

Thanks for the explanations.

-- 
Best regards,
Siarhei Siamashka

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

[Marc Zyngier]

On Wed, Oct 15 2014 at 03:05:24 PM, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:
> On Wed, 15 Oct 2014 13:42:33 +0100
> Marc Zyngier <marc.zyngier@arm.com> wrote:
>
>> On Wed, Oct 15 2014 at 11:40:24 AM, Siarhei Siamashka
>> <siarhei.siamashka@gmail.com> wrote:
>> > On Wed, 15 Oct 2014 11:31:44 +0100
>> > Marc Zyngier <marc.zyngier@arm.com> wrote:
>> >
>> >> On Wed, Oct 15 2014 at 11:25:10 AM, Siarhei Siamashka
>> >> <siarhei.siamashka@gmail.com> wrote:
>> >> > On Wed, 15 Oct 2014 12:13:05 +0200
>> >> > Hans de Goede <hdegoede@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@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.

My position in a nice ivory tower allows me to see further than you seem
to do, and makes me prefer robust solutions, as opposed to random hacks
that impact unsuspecting users by modifying the way things gets booted
based on decisions made by someone who may or may not have any idea of
what said users are trying to achieve.

Hans' solution is simple, robust, and doesn't randomly change
behaviour. That's definitely good enough for me.

	M.
-- 
Jazz is not dead. It just smells funny.

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

[Siarhei Siamashka]

On Wed, 15 Oct 2014 17:40:38 +0100
Marc Zyngier <marc.zyngier@arm.com> wrote:

> On Wed, Oct 15 2014 at 03:05:24 PM, Siarhei Siamashka <siarhei.siamashka@gmail.com> wrote:
> > On Wed, 15 Oct 2014 13:42:33 +0100
> > Marc Zyngier <marc.zyngier@arm.com> wrote:
> >
> >> On Wed, Oct 15 2014 at 11:40:24 AM, Siarhei Siamashka
> >> <siarhei.siamashka@gmail.com> wrote:
> >> > On Wed, 15 Oct 2014 11:31:44 +0100
> >> > Marc Zyngier <marc.zyngier@arm.com> wrote:
> >> >
> >> >> On Wed, Oct 15 2014 at 11:25:10 AM, Siarhei Siamashka
> >> >> <siarhei.siamashka@gmail.com> wrote:
> >> >> > On Wed, 15 Oct 2014 12:13:05 +0200
> >> >> > Hans de Goede <hdegoede@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@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.
> 
> My position in a nice ivory tower allows me to see further than you seem
> to do, and makes me prefer robust solutions, as opposed to random hacks
> that impact unsuspecting users by modifying the way things gets booted
> based on decisions made by someone who may or may not have any idea of
> what said users are trying to achieve.

Basically, your reasoning boils down to "I'm a cool guy, so I know
it better than you". And you just quoted only the irrelevant part,
skipping the rest. Not sure if participating in this kind of debate
makes any sense for me.

> Hans' solution is simple, robust, and doesn't randomly change
> behaviour. That's definitely good enough for me.

To sum it up:

1) I'm in favour of having a way to toggle between booting in secure and
   non-secure mode
2) I'm neutral to the idea of adding this new environment variable and
   have no objections. Especially if it is also useful on non-sunxi
   hardware.
3) I still think that u-boot has enough information at runtime to make
   a reliable decision (on sunxi hardware) without any need for the
   extra environment variable, but we just agree to disagree.

And since the patch from Hans is moving things in the right direction

Acked-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>

Have a nice day :-)

-- 
Best regards,
Siarhei Siamashka

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

[Hans de Goede]

Hi,

On Oct. 15, 2014, 10:25 a.m., Albert ARIBAUD wrote:

> Hi Marc, Hans,
>
> On Wed, 15 Oct 2014 11:18:28 +0100, Marc Zyngier <marc.zyngier@arm.com>
> wrote:
>
> > On Wed, Oct 15 2014 at 11:13:05 AM, Hans de Goede <hdegoede@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@redhat.com>
> >
> > Looks good to me.
> >
> > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> >
> > 	M.
>
> Should we consider this a bugfix?

This is not really a bug fix, this adds a mechanism to select between secure /
non secure boot on non-secure boot capable systems, as some older kernels do not
work in non-secure mode. Even with this in place old (broken) kernels will still
not magically work. Some sunxi specific patches are needed for that, as well as
the user actually setting the environment variable.

I'm targetting the next u-boot release for getting old sunxi kernels to work for
people who prefer using those kernels.

> For instance, hHow old are these "older kernels"?

The linux-sunxi kernel I'm specifically targetting is a 3.4 kernel, but this should
be useful in general on all platforms which support booting in non-secure mode.

Regards,

Hans