| Message ID | 1316799532-20761-5-git-send-email-sjg@chromium.org |
|---|---|
| State | Deferred, archived |
| Headers | show |
On Friday, September 23, 2011 13:38:52 Simon Glass wrote: > This tidies up network code to use snprintf() in most cases instead of > sprintf(). A few functions remain as they require header file changes. NAK to most of these. we pick local sized buffers that are known to not overflow, or require circumstances that aren't really feasible. 3 examples (which are the first 3 changes in this patch) below ... > --- a/net/eth.c > +++ b/net/eth.c > > char buf[20]; > - sprintf(buf, "%pM", enetaddr); > + snprintf(buf, sizeof(buf), "%pM", enetaddr); a mac address will not take more than 19 bytes. unless the sprintf code is completely busted, but if that's the case, we should fix that instead since it'd be pretty fundamentally screwed. > char enetvar[32]; > - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); > + snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr", > + base_name, index); in order for this to overflow, we have to have 1000+ eth devices (maybe more? i'd have to read the code closer) > char enetvar[15]; > - sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index); > + snprintf(enetvar, sizeof(enetvar), > + index ? "eth%dmacskip" : "ethmacskip", index); in order for this to overflow, we have to have 10000+ eth devices please look at the realistic needs rather than blanket converting to snprintf -mike
Hi Mike, On Fri, Sep 23, 2011 at 11:15 AM, Mike Frysinger <vapier@gentoo.org> wrote: > On Friday, September 23, 2011 13:38:52 Simon Glass wrote: >> This tidies up network code to use snprintf() in most cases instead of >> sprintf(). A few functions remain as they require header file changes. > > NAK to most of these. we pick local sized buffers that are known to not > overflow, or require circumstances that aren't really feasible. Yes that's why I sent this patch on the end, to see how far this should be taken. > > 3 examples (which are the first 3 changes in this patch) below ... > >> --- a/net/eth.c >> +++ b/net/eth.c >> >> char buf[20]; >> - sprintf(buf, "%pM", enetaddr); >> + snprintf(buf, sizeof(buf), "%pM", enetaddr); > > a mac address will not take more than 19 bytes. unless the sprintf code is > completely busted, but if that's the case, we should fix that instead since > it'd be pretty fundamentally screwed. OK (18 bytes?) > >> char enetvar[32]; >> - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); >> + snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr", >> + base_name, index); > > in order for this to overflow, we have to have 1000+ eth devices (maybe more? > i'd have to read the code closer) Or base_name could be longer. > >> char enetvar[15]; >> - sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index); >> + snprintf(enetvar, sizeof(enetvar), >> + index ? "eth%dmacskip" : "ethmacskip", index); > > in order for this to overflow, we have to have 10000+ eth devices > > please look at the realistic needs rather than blanket converting to snprintf OK, this is fair enough. There are a couple of functions like ip_to_string() which don't get passed a length. I would prefer to see this done, so we can use snprintf() or assert() since the code then becomes more robust. But there are no bugs there at present. Thoughts? Regards, Simon > -mike >
On Friday, September 23, 2011 14:30:09 Simon Glass wrote: > On Fri, Sep 23, 2011 at 11:15 AM, Mike Frysinger wrote: > > On Friday, September 23, 2011 13:38:52 Simon Glass wrote: > >> --- a/net/eth.c > >> +++ b/net/eth.c > >> > >> char buf[20]; > >> - sprintf(buf, "%pM", enetaddr); > >> + snprintf(buf, sizeof(buf), "%pM", enetaddr); > > > > a mac address will not take more than 19 bytes. unless the sprintf code > > is completely busted, but if that's the case, we should fix that instead > > since it'd be pretty fundamentally screwed. > > OK (18 bytes?) right ... i meant there's 19 available (since 20 makes NUL), and there's no way it should hit that 19 limit. > >> char enetvar[32]; > >> - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); > >> + snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr", > >> + base_name, index); > > > > in order for this to overflow, we have to have 1000+ eth devices (maybe > > more? i'd have to read the code closer) > > Or base_name could be longer. true, but base_name atm is supposed to be "eth" or "usbeth". i would hope we'd be diligent about device naming conventions rather than letting someone slip in "mysuperawesomeboardeth" (although that still wouldn't overflow ;]). > >> char enetvar[15]; > >> - sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index); > >> + snprintf(enetvar, sizeof(enetvar), > >> + index ? "eth%dmacskip" : "ethmacskip", index); > > > > in order for this to overflow, we have to have 10000+ eth devices > > > > please look at the realistic needs rather than blanket converting to > > snprintf > > OK, this is fair enough. There are a couple of functions like > ip_to_string() which don't get passed a length. I would prefer to see > this done, so we can use snprintf() or assert() since the code then > becomes more robust. But there are no bugs there at present. Thoughts? for funcs that do generally handle arbitrary data, i don't mind including len specs, but when we're using known standards, i'd prefer to stick to the smaller/simpler code. this is a thin boot loader that is meant to be fast, and there are plenty of ways to bring down the system otherwise (and i don't think these cases are generally a robustness concern). don't know if wolfgang has an opinion. -mike
Hi Mike, On Fri, Sep 23, 2011 at 1:09 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Friday, September 23, 2011 14:30:09 Simon Glass wrote: >> On Fri, Sep 23, 2011 at 11:15 AM, Mike Frysinger wrote: >> > On Friday, September 23, 2011 13:38:52 Simon Glass wrote: >> >> --- a/net/eth.c >> >> +++ b/net/eth.c >> >> >> >> char buf[20]; >> >> - sprintf(buf, "%pM", enetaddr); >> >> + snprintf(buf, sizeof(buf), "%pM", enetaddr); >> > >> > a mac address will not take more than 19 bytes. unless the sprintf code >> > is completely busted, but if that's the case, we should fix that instead >> > since it'd be pretty fundamentally screwed. >> >> OK (18 bytes?) > > right ... i meant there's 19 available (since 20 makes NUL), and there's no > way it should hit that 19 limit. > >> >> char enetvar[32]; >> >> - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); >> >> + snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr", >> >> + base_name, index); >> > >> > in order for this to overflow, we have to have 1000+ eth devices (maybe >> > more? i'd have to read the code closer) >> >> Or base_name could be longer. > > true, but base_name atm is supposed to be "eth" or "usbeth". i would hope > we'd be diligent about device naming conventions rather than letting someone > slip in "mysuperawesomeboardeth" (although that still wouldn't overflow ;]). Well here I feel that it is pushing it a bit to have a function that builds a string based on arguments over which it has no control. I suppose an assert(strlen(base_name) < 23) or whatever would do it. But isn't snprintf() better? > >> >> char enetvar[15]; >> >> - sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index); >> >> + snprintf(enetvar, sizeof(enetvar), >> >> + index ? "eth%dmacskip" : "ethmacskip", index); >> > >> > in order for this to overflow, we have to have 10000+ eth devices >> > >> > please look at the realistic needs rather than blanket converting to >> > snprintf >> >> OK, this is fair enough. There are a couple of functions like >> ip_to_string() which don't get passed a length. I would prefer to see >> this done, so we can use snprintf() or assert() since the code then >> becomes more robust. But there are no bugs there at present. Thoughts? > > for funcs that do generally handle arbitrary data, i don't mind including len > specs, but when we're using known standards, i'd prefer to stick to the > smaller/simpler code. this is a thin boot loader that is meant to be fast, > and there are plenty of ways to bring down the system otherwise (and i don't > think these cases are generally a robustness concern). Well that argues towards using assert() in these cases rather than just ignoring them. I'm keen to try to make U-Boot more testable. I do understand it is just a 'thin' boot loader, but it has quite a few features now, and it if breaks, the system won't boot. I do agree there is a difference between telling devs about a bug / overflow at build/test time and bloating the code in production. Either/or is good with me, but I'm not entirely comfortable with just ignoring these issues and hoping they don't bite us. It's not like we have a test suite to check it. > > don't know if wolfgang has an opinion. <insert here> Regards, Simon > -mike >
diff --git a/net/eth.c b/net/eth.c index 5911b1c..30711d4 100644 --- a/net/eth.c +++ b/net/eth.c @@ -49,7 +49,7 @@ int eth_setenv_enetaddr(char *name, const uchar *enetaddr) { char buf[20]; - sprintf(buf, "%pM", enetaddr); + snprintf(buf, sizeof(buf), "%pM", enetaddr); return setenv(name, buf); } @@ -58,7 +58,9 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index, uchar *enetaddr) { char enetvar[32]; - sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); + + snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr", + base_name, index); return eth_getenv_enetaddr(enetvar, enetaddr); } @@ -68,7 +70,9 @@ static int eth_mac_skip(int index) { char enetvar[15]; char *skip_state; - sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index); + + snprintf(enetvar, sizeof(enetvar), + index ? "eth%dmacskip" : "ethmacskip", index); return ((skip_state = getenv(enetvar)) != NULL); } diff --git a/net/net.c b/net/net.c index 7a60583..28e45c0 100644 --- a/net/net.c +++ b/net/net.c @@ -573,10 +573,12 @@ restart: printf("Bytes transferred = %ld (%lx hex)\n", NetBootFileXferSize, NetBootFileXferSize); - sprintf(buf, "%lX", NetBootFileXferSize); + snprintf(buf, sizeof(buf), "%lX", + NetBootFileXferSize); setenv("filesize", buf); - sprintf(buf, "%lX", (unsigned long)load_addr); + snprintf(buf, sizeof(buf), "%lX", + (unsigned long)load_addr); setenv("fileaddr", buf); } eth_halt(); @@ -950,7 +952,8 @@ int CDPSendTrigger(void) #ifdef CONFIG_CDP_DEVICE_ID *s++ = htons(CDP_DEVICE_ID_TLV); *s++ = htons(CONFIG_CDP_DEVICE_ID); - sprintf(buf, CONFIG_CDP_DEVICE_ID_PREFIX "%pm", NetOurEther); + snprintf(buf, sizeof(buf), CONFIG_CDP_DEVICE_ID_PREFIX "%pm", + NetOurEther); memcpy((uchar *)s, buf, 16); s += 16 / 2; #endif @@ -958,7 +961,7 @@ int CDPSendTrigger(void) #ifdef CONFIG_CDP_PORT_ID *s++ = htons(CDP_PORT_ID_TLV); memset(buf, 0, sizeof(buf)); - sprintf(buf, CONFIG_CDP_PORT_ID, eth_get_dev_index()); + snprintf(buf, sizeof(buf), CONFIG_CDP_PORT_ID, eth_get_dev_index()); len = strlen(buf); if (len & 1) /* make it even */ len++; @@ -1516,7 +1519,9 @@ NetReceive(volatile uchar *inpkt, int len) #ifdef CONFIG_KEEP_SERVERADDR if (NetServerIP == NetArpWaitPacketIP) { char buf[20]; - sprintf(buf, "%pM", arp->ar_data); + + snprintf(buf, sizeof(buf), "%pM", + arp->ar_data); setenv("serveraddr", buf); } #endif diff --git a/net/nfs.c b/net/nfs.c index f76f60d..d1e894e 100644 --- a/net/nfs.c +++ b/net/nfs.c @@ -688,7 +688,8 @@ NfsStart (void) } if (BootFile[0] == '\0') { - sprintf (default_filename, "/nfsroot/%02lX%02lX%02lX%02lX.img", + snprintf(default_filename, sizeof(default_filename), + "/nfsroot/%02lX%02lX%02lX%02lX.img", NetOurIP & 0xFF, (NetOurIP >> 8) & 0xFF, (NetOurIP >> 16) & 0xFF, diff --git a/net/tftp.c b/net/tftp.c index a893e02..c4f6a59 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -591,7 +591,8 @@ TftpStart(void) TftpRemoteIP = NetServerIP; if (BootFile[0] == '\0') { - sprintf(default_filename, "%02lX%02lX%02lX%02lX.img", + snprintf(default_filename, sizeof(default_filename), + "%02lX%02lX%02lX%02lX.img", NetOurIP & 0xFF, (NetOurIP >> 8) & 0xFF, (NetOurIP >> 16) & 0xFF,
This tidies up network code to use snprintf() in most cases instead of sprintf(). A few functions remain as they require header file changes. Signed-off-by: Simon Glass <sjg@chromium.org> --- net/eth.c | 10 +++++++--- net/net.c | 15 ++++++++++----- net/nfs.c | 3 ++- net/tftp.c | 3 ++- 4 files changed, 21 insertions(+), 10 deletions(-)