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

Attachment: signature.asc
Description: PGP signature

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to