[U-Boot] [PATCH 00/11] Introduce variables whitelisting in environment

[Quentin Schulz]

Hi Simon,

On 29/12/2017 04:13, Simon Glass wrote:
> Hi Quentin,
> 
> On 22 December 2017 at 14:13, Quentin Schulz
> <quentin.schulz at free-electrons.com> wrote:
>> This patch series is based on this[1] patch series from Maxime.
>>
>> This is an RFC. It's been only tested in a specific use case on a custom
>> i.MX6 board. It's known to break compilation on a few boards.
>>
>> I have a use case where we want some variables from a first environment to
>> be overriden by variables from a second environment. For example, we want
>> to load variables from the default env (ENV_IS_NOWHERE) and then load only
>> a handful of other variables from, e.g., NAND.
>>
>> In our use case, we basically can be sure that the default env in the
>> U-Boot binary is secure but we want only a few variables to be modified,
>> thus keeping control over the overall behaviour of U-Boot in secure mode.
>>
>> It works in that way:
>>   - from highest to lowest priority, the first environment that can be
>>   loaded (that has successfully init and whose load function has returned
>>   no errors) will be the main environment,
>>   - then, all the following environment that could be successfully loaded
>>   (same conditions as the main environment) are secondary environment. The
>>   env variables that are defined both in CONFIG_ENV_VAR_WHITELIST_LIST and
>>   in the secondary environments override the ones in the main environment,
>>   - for saving, we save the whole environment to all environments
>>   available, be they main or secondary (it does not matter to save the
>>   whole environment on secondary environments as only the whitelisted
>>   variables will be overriden in the loading process,
>>
>> I have also a few questions that could help me to get the whole thing to
>> work.
>>
>> 1) I can't really get my head around the use of gd->env_addr, what is it used
>> for? It is set in a bunch of different places but only once is it
>> explicitly used (basically to alternate the env_addr between the one
>> associated to main and redundant environment (in NAND for example)).
>>
>> 2) Why do we consider ENV_IS_NOWHERE an invalid environment? The only place I
>> found a use for it was to just say that if the environment is invalid, we
>> should set to default environment (in env_relocate in env/common.c). With
>> my patch series I guess that we could remove this fallback and force
>> ENV_IS_NOWHERE to be always there.
>>
>> 3) There are a few (20) boards that set gd->env_addr and gd->env_valid in
>> their board file. What is the reason to do such a thing? Isn't those
>> overriden anyway by the environment driver?
>>
>> I'm looking forward to getting your feedback on this patch series.
>>
> 
> I certainly understand the goal and it seems generally useful.
> 
> But I wonder whether this is the best way to implement it.
> 
> We could have a UCLASS_ENV uclass, with driver-model drivers which
> load the environment (i.e. have load() and save() methods). Config for
> the drivers would come from the device tree. Useful drivers would be:
> 

I'm not sure the device tree is the place to set/get such info. That has
nothing to do with hardware, it's only logic for the bootloader.

> - one that loads the env from a single location
> - one that loads multiple envs from different locations in priority order
> - one that does what you want
> 
> Then people could set their own policy by adding a driver.
> 

I'll have to document myself on driver-model and how U-Boot implement it
then :)

> I worry that what we have here is quite heavyweight, and will be
> imposed on all users, e.g. the size increase of gd, the new logic.
> Where does it end? I think splitting things up into different use
> cases makes sense.
> 

Agree on that.

> When I did the env refactor I envisaged using driver-model for the
> post-reloc env load/save at some point in the future. It solves the
> problem of getting the config (can use device tree) and different
> boards wanting to do different things (boards can provide an env
> driver).
> 

Overall, I prefer Lukasz's suggestion as it's quicker and easier to
implement and require (I think) less changes in the code.

Thanks for the review,
Quentin
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com