[U-Boot] [PATCH 21/24] tegra124: Implement spl_was_boot_source()

[Stephen Warren]

On 05/05/2015 10:19 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 5 May 2015 at 10:10, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> On 05/05/2015 10:02 AM, Simon Glass wrote:
>>>
>>> Hi Stephen,
>>>
>>> On 5 May 2015 at 09:54, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>>
>>>> On 05/04/2015 11:31 AM, Simon Glass wrote:
>>>>>
>>>>>
>>>>> Add an implementation of this function for Tegra.
>>>>
>>>>
>>>>
>>>>> diff --git a/arch/arm/mach-tegra/board.c b/arch/arm/mach-tegra/board.c
>>>>
>>>>
>>>>
>>>>> +#ifndef CONFIG_SPL_BUILD
>>>>> +void save_boot_params(u32 r0, u32 r1, u32 r2, u32 r3)
>>>>> +{
>>>>> +       from_spl = r0 != SPL_RUNNING_FROM_UBOOT;
>>>>> +       save_boot_params_ret();
>>>>> +}
>>>>> +#endif
>>>>
>>>>
>>>>
>>>> (Using terminology from:
>>>> https://patchwork.ozlabs.org/patch/467771/
>>>> arm: spl: Enable detecting when U-Boot is started from SPL
>>>> )
>>>>
>>>> That doesn't look right. Surely (at least on Tegra), if the r/o U-Boot
>>>> chain-loads to the r/w U-Boot, then the chain-loaded U-Boot has no SPL
>>>> and
>>>> is just the main CPU build of U-Boot.
>>>>
>>>> Hence, "SPL_RUNNING_FROM_UBOOT" seems incorrectly named, since the r/o
>>>> U-Boot doesn't chain to SPL but rather to U-Boot.
>>>
>>>
>>> What name do you suggest? I was trying to add a prefix indicating that
>>> it relates to non-SPL start-up of U-Boot.
>>
>>
>> Well, that name specifically states that it's SPL that's running, whereas
>> the exact opposite is true.
>>
>> Perhaps UBOOT_CHAIN_LOADED_FROM_UBOOT?
>
> I really want to say that it is not chain-loaded from SPL. Maybe
> UBOOT_NOT_LOADED_FROM_SPL?

OK, that highlights that better.

>>>> This approach sounds a little brittle; what happens if r0 just happens to
>>>> have that value. Won't the code get confused?
>>>
>>>
>>> Yes, but SPL does not set that value in r0, and we have control over this.
>>>
>>>> Why does U-Boot care whether it's been chain-loaded? Shouldn't it always
>>>> behave identically in all cases, so it's independent of what caused it to
>>>> run?
>>>
>>>
>>> In the case of read-only U-Boot it must find the read-write one to
>>> jump to. In the case of read-write U-Boot it must boot a kernel.
>>
>>
>> Surely that should be taken care of by placing the correct boot scripts into
>> the U-Boot environment, rather than hard-coding specific boot behaviour into
>> the U-Boot binary?
>
> Two problems here:
>
> 1. The two U-Boot will use the same environment (as they are identical
> after all)

That's a design decision. There's absolutely no need for that to be true.

> 2. Loading the environment is a security risk (since anyone can change
> it in Linux, for example) so cannot be loaded.

Well, the environment could be the default/built-in environment and 
hence validated as part of the validation of the U-Boot binary. Or, even 
if loaded separately, could also be validated  in the same way (but 
perhaps there's not much point in that, since a fall-back to the 
built-in environment would be required in case the external environment 
validation failed).

>> This feature seems really use-case-specific; I wonder if it's
>> useful/generic-enough to upstream even?
>
> I am keen to upstream this use case (upgrading U-Boot in a secure way)
> as I think it has wide application.

OK. I worry that there are many many possible ways of doing that, and 
the selection of the best option depends on the system use-cases, 
security model, and environments. We might not want to lock people into 
a specific method. So long as the existence of this code doesn't prevent 
doing things some other way if they need, or upstreaming support for 
other methods, nor make the code too complex, then it's probably fine.