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

[Simon Glass]

Hi Stephen,

On 5 May 2015 at 12:07, Stephen Warren <swarren at wwwdotorg.org> wrote:
> 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.

At present U-Boot has a single CONFIG for the environment
type/position. Therefore we can't have the same U-Boot behave
differently.

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

That's why we have the load-env device tree option, which uses the
default environment. But since the RO and RW U-Boot then have the same
environment, this doesn't really help.

Yes we could validate it, although it's a pain since it is one more
thing to sign and hash and it's not in the FIT.

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

This seems pretty simple to me.

Regards,
Simon