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