[PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE

[Tom Rini]

On Fri, Jan 29, 2021 at 12:03:52AM +0100, Marek Vasut wrote:
> On 1/28/21 8:26 PM, Tom Rini wrote:
> > On Thu, Jan 28, 2021 at 08:07:54PM +0100, Marek Vasut wrote:
> > > On 1/11/21 11:27 AM, Martin Fuzzey wrote:
> > > > Since commit 0f036bf4b87e ("env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set")
> > > > a warning message is displayed when setenv -f is used WITHOUT
> > > > CONFIG_ENV_ACCESS_IGNORE_FORCE, but the variable is set anyway, resulting
> > > > in lots of log pollution.
> > > > 
> > > > env_flags_validate() returns 0 if the access is accepted, or non zero
> > > > if it is refused.
> > > > 
> > > > So the original code
> > > > 	#ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
> > > > 		if (flag & H_FORCE)
> > > > 			return 0;
> > > > 	#endif
> > > > 
> > > > was correct, it returns 0 (accepts the modification) if forced UNLESS
> > > > IGNORE_FORCE is set (in which case access checks in the following code
> > > > are applied). The broken patch just added a printf to the force accepted
> > > > case.
> > > > 
> > > > To obtain the intent of the patch we need this:
> > > > 	if (flag & H_FORCE) {
> > > > 	#ifdef CONFIG_ENV_ACCESS_IGNORE_FORCE
> > > > 		printf("## Error: Can't force access to \"%s\"\n", name);
> > > > 	#else
> > > > 		return 0;
> > > > 	#endif
> > > > 	}
> > > > 
> > > > Fixes: 0f036bf4b87e ("env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set")
> > > > 
> > > > Signed-off-by: Martin Fuzzey <martin.fuzzey at flowbird.group>
> > > > ---
> > > >    env/flags.c | 5 +++--
> > > >    1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/env/flags.c b/env/flags.c
> > > > index df4aed2..e3e833c 100644
> > > > --- a/env/flags.c
> > > > +++ b/env/flags.c
> > > > @@ -563,12 +563,13 @@ int env_flags_validate(const struct env_entry *item, const char *newval,
> > > >    		return 1;
> > > >    #endif
> > > > -#ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
> > > >    	if (flag & H_FORCE) {
> > > > +#ifdef CONFIG_ENV_ACCESS_IGNORE_FORCE
> > > >    		printf("## Error: Can't force access to \"%s\"\n", name);
> > > > +#else
> > > >    		return 0;
> > > > -	}
> > > >    #endif
> > > 
> > > Based on env/Kconfig  description of this option:
> > > 
> > > config ENV_ACCESS_IGNORE_FORCE
> > >          bool "Block forced environment operations"
> > >          default n
> > >          help
> > >            If defined, don't allow the -f switch to env set override variable
> > >            access flags.
> > > 
> > > I would think the code should look like this:
> > > 
> > > #ifdef CONFIG_ENV_ACCESS_IGNORE_FORCE
> > >          if (flag & H_FORCE) {
> > >                  printf("## Error: Can't force access to \"%s\"\n", name);
> > >                  return 1;
> > >          }
> > > #else
> > >          if (flag & H_FORCE)
> > > 		return 0;
> > > #endif
> > 
> > So, prior to 0f036bf4b87e we had what you're suggesting, and that lead
> > to 8a5cdf601f8d (which is the commit I was trying to think of) which
> > Heinrich did not like, but was what was needed to get things to function
> > again.  Wouldn't what you're proposing break the use case you had in the
> > first place?
> 
> No, the idea is to completely block the -f flag if
> CONFIG_ENV_ACCESS_IGNORE_FORCE is set from setting anything in the
> environment. That's how I understand the Kconfig entry help text.

So was this all a "by inspection" bug then and not something you ran in
to in use?  I'm a bit worried that "how it's implemented" is relied upon
more than "how it's documented in the help", esp since the former is
probably older than the latter.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210201/a4b49527/attachment.sig>