[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[Ondřej Jirman]

On Sat, Sep 14, 2019 at 02:29:11PM -0400, Tom Rini wrote:
> On Sat, Sep 14, 2019 at 04:05:44PM +0200, Ondřej Jirman wrote:
> > Hi,
> > 
> > On Fri, Sep 13, 2019 at 07:40:22PM -0500, Joe Hershberger wrote:
> > > Part of the env cleanup moved this out of the environment code and into
> > > the net code. However, this helper is sometimes needed even when the net
> > > stack isn't included.
> > > 
> > > Move the helper to lib/net_utils.c like it's similarly-purposed
> > > string_to_ip(). Also rename the moved function to similar naming.
> > > 
> > > Signed-off-by: Joe Hershberger <joe.hershberger at ni.com>
> > > Reported-by: Ondrej Jirman <megous at megous.com>
> > 
> > I've tested the patch and it works, but I'be found other related issue, where
> > u-boot thinks %pM will format a MAC address string, but it does just
> > print out the pointer due to relevant functions being gated by CONFIG_CMD_NET
> > guard in lib/vsprintf.c.
> > 
> > The gating should probably be done so that it panics/halts the u-boot if gated
> > pointer flags are used by u-boot code, because that will clearly be incorrect,
> > without calling code ever knowing. This way the user will know that something
> > is wrong and will have to fix the code.
> 
> I'm not in favor of panic because of calling an unimplemented print
> format character.  I guess we'll need to see what the size increase is
> on un-guarding these formats and go from there.

OTOH, u-boot doesn't use snprintf everywhere, and uses sprintf quite liberally,
so:

  char buf[exepected_size_for_format_X];

  sprintf(buf, "%pX", some_pointer);

may conceivably overflow the buffer, because u-boot will format a generic
pointer there instead of what the developer expected (based on config
option, the dev may never tried disabling), so only some users may be affected
by silent or not so silent stack corruption.

I think this is unlikely atm, because all formats I inspected, seem to
produce longer strings than the 64-bit generic pointer formatting.

That was why I suggested the panic(), because it may not be just a superficial
formatting issue.

Anyway, you definitely want a loud error, rather than silently
passing incorrect/unexpected data somewhere where it matters, like DTB.

regards,
	o.



> -- 
> Tom



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