[U-Boot] [PATCH] cmd: nvedit: env_get_f must check for env_get_char error codes

[Maxime Ripard]

Hi Simon,

On Thu, Feb 01, 2018 at 10:16:46AM +0100, Simon Goldschmidt wrote:
> On 01.02.2018 00:00, York Sun wrote:
> > On 01/30/2018 10:57 PM, Simon Goldschmidt wrote:
> > > env_get_f calls env_get_char to load single characters from the
> > > environment. However, the return value of env_get_char was not
> > > checked for errors. Now if the env driver does not support the
> > > .get_char call, env_get_f did not notice this and looped over the
> > > whole size of the environment, calling env_get_char over 8000
> > > times with the default settings, just to return an error in the
> > > end.
> > > 
> > > Fix this by checking if env_get_char returns < 0.
> > > 
> > > Signed-off-by: Simon Goldschmidt <sgoldschmidt at de.pepperl-fuchs.com>
> > > ---
> > > 
> > >   cmd/nvedit.c | 11 ++++++++---
> > >   1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > > index a690d743cd..4cb25b8248 100644
> > > --- a/cmd/nvedit.c
> > > +++ b/cmd/nvedit.c
> > > @@ -650,12 +650,14 @@ char *env_get(const char *name)
> > >    */
> > >   int env_get_f(const char *name, char *buf, unsigned len)
> > >   {
> > > -	int i, nxt;
> > > +	int i, nxt, c;
> > >   	for (i = 0; env_get_char(i) != '\0'; i = nxt + 1) {
> > >   		int val, n;
> > > -		for (nxt = i; env_get_char(nxt) != '\0'; ++nxt) {
> > > +		for (nxt = i; (c = env_get_char(nxt)) != '\0'; ++nxt) {
> > > +			if (c < 0)
> > > +				return c;
> > >   			if (nxt >= CONFIG_ENV_SIZE)
> > >   				return -1;
> > >   		}
> > > @@ -666,7 +668,10 @@ int env_get_f(const char *name, char *buf, unsigned len)
> > >   		/* found; copy out */
> > >   		for (n = 0; n < len; ++n, ++buf) {
> > > -			*buf = env_get_char(val++);
> > > +			c = env_get_char(val++);
> > > +			if (c < 0)
> > > +				return c;
> > > +			*buf = c;
> > >   			if (*buf == '\0')
> > >   				return n;
> > >   		}
> > > 
> > Simon,
> > 
> > This patch looks correct. But it doesn't fix NOR flash. Do you have plan
> > to add .get_char function to other drivers? Without that function, we
> > cannot get env variables before relocation.
> 
> Ehrm, sorry  I don't plan to do that, no: my target seems to run fine
> without this.
> 
> Given that only the eeprom and nvram env drivers support the get_char
> method, I don't know if this is widely used at all. Maybe a better fallback
> would be to just remove that get_char code path totally and always load from
> the internal (default) environment until the full environment is available
> (after relocation).
> 
> After all, the environment variables loaded via get_char are not CRC checked
> at all. To me, this is another indication that this code is not really
> useful and should probably be removed.

To be honest, I'm not really sure what get_char was here for in the
first place, so getting rid of it sounds like a good idea :)

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180202/a5f79941/attachment.sig>