[PATCH] env: common: load read-only variables after reset

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] env: common: load read-only variables after reset

Yaniv Levinsky
U-Boot fails to load read-only variables from storage after a reset. It
happens because the environment hash table prevents creating read-only
variables unless the H_FORCE flag is passed.

In the following example, the variable "test" is set to read-only in the
board header file (#define CONFIG_ENV_FLAGS_LIST_DEFAULT "test:sr"):

        U-Boot> printenv .flags
        .flags=test:sr
        U-Boot> setenv -f test 1
        U-Boot> printenv test
        test=1
        U-Boot> savee
        Saving Environment to SPI Flash
        ...
        OK
        U-Boot> reset
        ...
        Loading Environment from SPI Flash...
        ## Error: Can't create "test"
        himport_r: can't insert "test=1" into hash table
        ...
        U-Boot> printenv test
        ## Error: "test" not defined

Pass the H_FORCE flag when importing the environment from storage.

Signed-off-by: Yaniv Levinsky <[hidden email]>
---
 env/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/env/common.c b/env/common.c
index 3317cef355..de8dd47e9b 100644
--- a/env/common.c
+++ b/env/common.c
@@ -119,7 +119,7 @@ int env_import(const char *buf, int check)
  }
  }
 
- if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0,
+ if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', H_FORCE, 0,
  0, NULL)) {
  gd->flags |= GD_FLG_ENV_READY;
  return 0;
--
2.18.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] env: common: load read-only variables after reset

Wolfgang Denk
Dear Yaniv,

In message <[hidden email]> you wrote:
> U-Boot fails to load read-only variables from storage after a reset. It
> happens because the environment hash table prevents creating read-only
> variables unless the H_FORCE flag is passed.

This is NOT a good idea.  "env import" should respect read-only
settings in exactly the same way as "env set" does.  Please keep in
minf that the user my set a variable to read-only exactly just to
prevent if from bein (accidentially) overwritten when importing an
(not exactly know) set of environment settings.  Your patch would
kill any such protection.

IMO the correct approach would be to add a "-f" flag to "env import"
and the, and ONLY then, also set the H_FORCE flag.

Naked-by: Wolfgang Denk <[hidden email]>

Best regards,

Wolfgang Denk

--
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: [hidden email]
I'm a programmer: I don't buy software, I write it.
                                                  -- Tom Christiansen
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] env: common: load read-only variables after reset

Yaniv Levinsky
Hello Wolfgang,

On 07/28/2018 03:56 PM, Wolfgang Denk wrote:

> Dear Yaniv,
>
> In message <[hidden email]> you wrote:
>> U-Boot fails to load read-only variables from storage after a reset. It
>> happens because the environment hash table prevents creating read-only
>> variables unless the H_FORCE flag is passed.
>
> This is NOT a good idea.  "env import" should respect read-only
> settings in exactly the same way as "env set" does.  Please keep in
> minf that the user my set a variable to read-only exactly just to
> prevent if from bein (accidentially) overwritten when importing an
> (not exactly know) set of environment settings.  Your patch would
> kill any such protection.
>
> IMO the correct approach would be to add a "-f" flag to "env import"
> and the, and ONLY then, also set the H_FORCE flag.
>
> Naked-by: Wolfgang Denk <[hidden email]>
>
> Best regards,
>
> Wolfgang Denk
>
Thank you for reviewing the patch.

The command "env import" should of course respect access restriction flags.

Confusingly, the function env_import() in env/common.c has nothing to do with
the implementation of the "env import" command. This is done entirely by
do_env_import() in cmd/nvedit.c.

To the best of my knowledge, the function env_import() is used only by storage
devices to load the environment on boot. In this scenario, I think we do need to
ignore access restriction flags. Otherwise, read-only variables won't load from
storage with the rest of the environment.

Please correct me if I'm wrong.

Thanks,
Yaniv
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] env: common: load read-only variables after reset

Yaniv Levinsky
In reply to this post by Yaniv Levinsky
On 07/27/2018 06:34 PM, Yaniv Levinsky wrote:

> U-Boot fails to load read-only variables from storage after a reset. It
> happens because the environment hash table prevents creating read-only
> variables unless the H_FORCE flag is passed.
>
> In the following example, the variable "test" is set to read-only in the
> board header file (#define CONFIG_ENV_FLAGS_LIST_DEFAULT "test:sr"):
>
> U-Boot> printenv .flags
> .flags=test:sr
> U-Boot> setenv -f test 1
> U-Boot> printenv test
> test=1
> U-Boot> savee
> Saving Environment to SPI Flash
> ...
> OK
> U-Boot> reset
> ...
> Loading Environment from SPI Flash...
> ## Error: Can't create "test"
> himport_r: can't insert "test=1" into hash table
> ...
> U-Boot> printenv test
> ## Error: "test" not defined
>
> Pass the H_FORCE flag when importing the environment from storage.
>
> Signed-off-by: Yaniv Levinsky <[hidden email]>
> ---
>  env/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/env/common.c b/env/common.c
> index 3317cef355..de8dd47e9b 100644
> --- a/env/common.c
> +++ b/env/common.c
> @@ -119,7 +119,7 @@ int env_import(const char *buf, int check)
>   }
>   }
>  
> - if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0,
> + if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', H_FORCE, 0,
>   0, NULL)) {
>   gd->flags |= GD_FLG_ENV_READY;
>   return 0;
>

Gentle ping.
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot