[00/11] Introduce variables whitelisting in environment

Message ID cover.85b371954a92a5f2af96f471effacfaedd8b5271.1513975247.git-series.quentin.schulz@free-electrons.com
Headers show
Series
  • Introduce variables whitelisting in environment
Related show

Message

Quentin Schulz Dec. 22, 2017, 9:13 p.m.
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.

Thanks,
Quentin

[1] https://patchwork.ozlabs.org/cover/842057/

Quentin Schulz (11):
  env: fix ret not being set and fails when no env could have been init
  lib: hashtable: support whitelisting env variables
  env: add support for whitelisting variables from secondary environments
  env: make nowhere an env medium like the others
  cmd: saveenv: enable the saveenv command when ENV_IS_NOWHERE is defined but another env medium is enabled too
  env: add env_driver to env_driver functions' arguments
  env: gd flags without ENV_READY is enough to discriminate in env_get_default
  env: add env_driver parameter to env_import_redund
  env: make env_locations a global variable
  env: introducing env_info struct for storing info per env
  env: store information about each environment in gd

 board/sunxi/board.c               |   2 +-
 cmd/nvedit.c                      |  16 ++-
 common/board_r.c                  |   8 +-
 env/Kconfig                       |  29 +++---
 env/common.c                      |  45 ++++++----
 env/eeprom.c                      |  40 ++++-----
 env/env.c                         | 142 +++++++++++++++++++++++++------
 env/ext4.c                        |   4 +-
 env/fat.c                         |   4 +-
 env/flash.c                       |  58 ++++++-------
 env/mmc.c                         |  14 +--
 env/nand.c                        |  46 +++++-----
 env/nowhere.c                     |  12 ++-
 env/nvram.c                       |  18 ++--
 env/onenand.c                     |   6 +-
 env/remote.c                      |  10 +-
 env/sata.c                        |   4 +-
 env/sf.c                          |  34 +++----
 env/ubi.c                         |  14 +--
 include/asm-generic/global_data.h |   5 +-
 include/environment.h             |  59 ++++++++-----
 include/search.h                  |   2 +-
 lib/hashtable.c                   |  17 +++-
 23 files changed, 379 insertions(+), 210 deletions(-)

base-commit: 5d41e28058e7b378c9fa5c61ecc074a682ba2db4

Comments

Lukasz Majewski Dec. 26, 2017, 11:25 a.m. | #1
Hi Quentin,

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


It is perfectly valid to use ENV_IS_NOWHERE to get envs from hardcoded
board file (for example for production u-boot).

However, some time ago we had extension in the envs to get the support
for envs from different mediums (with set priorities).

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Simon Glass Dec. 29, 2017, 3:13 a.m. | #2
Hi Quentin,

On 22 December 2017 at 14:13, Quentin Schulz
<quentin.schulz@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:

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

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

Regards,
Simon

> Thanks,
> Quentin
>
> [1] https://patchwork.ozlabs.org/cover/842057/
>
> Quentin Schulz (11):
>   env: fix ret not being set and fails when no env could have been init
>   lib: hashtable: support whitelisting env variables
>   env: add support for whitelisting variables from secondary environments
>   env: make nowhere an env medium like the others
>   cmd: saveenv: enable the saveenv command when ENV_IS_NOWHERE is defined but another env medium is enabled too
>   env: add env_driver to env_driver functions' arguments
>   env: gd flags without ENV_READY is enough to discriminate in env_get_default
>   env: add env_driver parameter to env_import_redund
>   env: make env_locations a global variable
>   env: introducing env_info struct for storing info per env
>   env: store information about each environment in gd
>
>  board/sunxi/board.c               |   2 +-
>  cmd/nvedit.c                      |  16 ++-
>  common/board_r.c                  |   8 +-
>  env/Kconfig                       |  29 +++---
>  env/common.c                      |  45 ++++++----
>  env/eeprom.c                      |  40 ++++-----
>  env/env.c                         | 142 +++++++++++++++++++++++++------
>  env/ext4.c                        |   4 +-
>  env/fat.c                         |   4 +-
>  env/flash.c                       |  58 ++++++-------
>  env/mmc.c                         |  14 +--
>  env/nand.c                        |  46 +++++-----
>  env/nowhere.c                     |  12 ++-
>  env/nvram.c                       |  18 ++--
>  env/onenand.c                     |   6 +-
>  env/remote.c                      |  10 +-
>  env/sata.c                        |   4 +-
>  env/sf.c                          |  34 +++----
>  env/ubi.c                         |  14 +--
>  include/asm-generic/global_data.h |   5 +-
>  include/environment.h             |  59 ++++++++-----
>  include/search.h                  |   2 +-
>  lib/hashtable.c                   |  17 +++-
>  23 files changed, 379 insertions(+), 210 deletions(-)
>
> base-commit: 5d41e28058e7b378c9fa5c61ecc074a682ba2db4
> --
> git-series 0.9.1
Quentin Schulz Jan. 3, 2018, 9:18 a.m. | #3
Hi Simon,

On 29/12/2017 04:13, Simon Glass wrote:
> Hi Quentin,
> 
> On 22 December 2017 at 14:13, Quentin Schulz
> <quentin.schulz@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
Simon Glass Jan. 8, 2018, 4:50 a.m. | #4
Hi Quentin,

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