diff mbox

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

Message ID 1413985502-19257-1-git-send-email-hdegoede@redhat.com
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Commit Message

Hans de Goede Oct. 22, 2014, 1:45 p.m. UTC 
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>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Acked-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>

--
Changes in v2:
-Allow changing the default boot mode to secure through defining
 CONFIG_ARMV7_SEC_BY_DEFAULT, this is useful for archs which have a Kconfig
 option for compatibility with older kernels
---
 arch/arm/lib/bootm.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

Comments

Ian Campbell Oct. 22, 2014, 6:55 p.m. UTC | #1 
On Wed, 2014-10-22 at 15:45 +0200, Hans de Goede wrote:
> 	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);

There's a subtle different here, which is that this final kernel_entry
call used to be in the #else clause, and so emitted for the NONSEC ||
VIRT case. So if the _do_nonsec_entry call were to fail (not currently
possible) and return you'd end up trying again via the sec path.

I'm not sure that's a bad thing, but it is a difference so it'd be good
to know it was a deliberate choice (or not).

Ian.
Hans de Goede Oct. 23, 2014, 8:22 a.m. UTC | #2 
Hi Ian,

On 10/22/2014 08:55 PM, Ian Campbell wrote:
> On Wed, 2014-10-22 at 15:45 +0200, Hans de Goede wrote:
>> 	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);
> 
> There's a subtle different here, which is that this final kernel_entry
> call used to be in the #else clause, and so emitted for the NONSEC ||
> VIRT case. So if the _do_nonsec_entry call were to fail (not currently
> possible) and return you'd end up trying again via the sec path.
> 
> I'm not sure that's a bad thing, but it is a difference so it'd be good
> to know it was a deliberate choice (or not).

I was under the assumption that do_nonsec_entry would never fail, and would
not return, which is why I wrote this code the way I wrote it. I'm not sure
if retrying in secure mode meets the principle of least surprise, so I guess
the #if .. #endif block should probably get an "else" added before the #endif,
do you agree?

Regards,

Hans
Ian Campbell Oct. 23, 2014, 9:30 a.m. UTC | #3 
On Thu, 2014-10-23 at 10:22 +0200, Hans de Goede wrote:
> Hi Ian,
> 
> On 10/22/2014 08:55 PM, Ian Campbell wrote:
> > On Wed, 2014-10-22 at 15:45 +0200, Hans de Goede wrote:
> >> 	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);
> > 
> > There's a subtle different here, which is that this final kernel_entry
> > call used to be in the #else clause, and so emitted for the NONSEC ||
> > VIRT case. So if the _do_nonsec_entry call were to fail (not currently
> > possible) and return you'd end up trying again via the sec path.
> > 
> > I'm not sure that's a bad thing, but it is a difference so it'd be good
> > to know it was a deliberate choice (or not).
> 
> I was under the assumption that do_nonsec_entry would never fail, and would
> not return, which is why I wrote this code the way I wrote it.

AFAICT in practice it can't fail today, but if it were somehow modified
in the future to do so this would expose some slightly surprising
behaviour.

>  I'm not sure
> if retrying in secure mode meets the principle of least surprise, so I guess
> the #if .. #endif block should probably get an "else" added before the #endif,
> do you agree?

Yes.

BTW, if you put the #ifdef around boot_nonsec() instead and make the
#else case #define boot_nonsec() (0) then does that end up looking
cleaner here at the caller? Maybe that causes knockons with the
prototypes for the unused functions in that case, in which case I doubt
it is worth it.

Ian.
Hans de Goede Oct. 23, 2014, 9:37 a.m. UTC | #4 
Hi,

On 10/23/2014 11:30 AM, Ian Campbell wrote:
> On Thu, 2014-10-23 at 10:22 +0200, Hans de Goede wrote:
>> Hi Ian,
>>
>> On 10/22/2014 08:55 PM, Ian Campbell wrote:
>>> On Wed, 2014-10-22 at 15:45 +0200, Hans de Goede wrote:
>>>> 	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);
>>>
>>> There's a subtle different here, which is that this final kernel_entry
>>> call used to be in the #else clause, and so emitted for the NONSEC ||
>>> VIRT case. So if the _do_nonsec_entry call were to fail (not currently
>>> possible) and return you'd end up trying again via the sec path.
>>>
>>> I'm not sure that's a bad thing, but it is a difference so it'd be good
>>> to know it was a deliberate choice (or not).
>>
>> I was under the assumption that do_nonsec_entry would never fail, and would
>> not return, which is why I wrote this code the way I wrote it.
> 
> AFAICT in practice it can't fail today, but if it were somehow modified
> in the future to do so this would expose some slightly surprising
> behaviour.
> 
>>  I'm not sure
>> if retrying in secure mode meets the principle of least surprise, so I guess
>> the #if .. #endif block should probably get an "else" added before the #endif,
>> do you agree?
> 
> Yes.
> 
> BTW, if you put the #ifdef around boot_nonsec() instead and make the
> #else case #define boot_nonsec() (0) then does that end up looking
> cleaner here at the caller? Maybe that causes knockons with the
> prototypes for the unused functions in that case, in which case I doubt
> it is worth it.

The problem there is that do_nonsec_entry is not defined in that case, so
we really need an #ifdef there.

Regards,

Hans
diff mbox

Patch

diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 39fe7a1..ff0170a 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -235,6 +235,26 @@  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");
+#ifdef CONFIG_ARMV7_SEC_BY_DEFAULT
+	bool nonsec = false;
+#else
+	bool nonsec = true;
+#endif
+
+	if (s && !strcmp(s, "sec"))
+		nonsec = false;
+
+	if (s && !strcmp(s, "nonsec"))
+		nonsec = true;
+
+	return nonsec;
+}
+#endif
+
 /* Subcommand: GO */
 static void boot_jump_linux(bootm_headers_t *images, int flag)
 {
@@ -283,12 +303,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
 }