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

[Simon Glass]

Hi Quentin,

On 3 January 2018 at 02:18, Quentin Schulz
<quentin.schulz at free-electrons.com> wrote:
> 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.

Do you mean selecting from different locations? Yes that is a good
thing, but is orthogonal to this series.

Here you are trying to add functionality which will apply to any env
location, or to them as a whole. So we should think of your change as
implementing a new policy rather than a new env location.

Regards,
Simon