| Message ID | 20161219220136.28550-1-judge.packham@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Chris, On 20 December 2016 at 11:01, Chris Packham <judge.packham@gmail.com> wrote: > Previously values greater than 255 were implicitly truncated. Add some > stricter checking to reject addresses with components >255. > > With the input "1234192.168.1.1" the old behaviour would truncate the > address to 192.168.1.1. New behaviour rejects the string outright and > returns 0.0.0.0, which for the purposes of IP addresses can be > considered an error. > > Signed-off-by: Chris Packham <judge.packham@gmail.com> > --- > This was part of my long running IPv6 patchset (which I promise I'll get > back to someday). But I feel this stands on it's own merits. > > lib/net_utils.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/lib/net_utils.c b/lib/net_utils.c > index cfae84275241..f148b8a70a7d 100644 > --- a/lib/net_utils.c > +++ b/lib/net_utils.c > @@ -24,11 +24,16 @@ struct in_addr string_to_ip(const char *s) > > for (addr.s_addr = 0, i = 0; i < 4; ++i) { > ulong val = s ? simple_strtoul(s, &e, 10) : 0; > + if (val > 255) { > + addr.s_addr = 0; > + return addr; > + } > addr.s_addr <<= 8; > addr.s_addr |= (val & 0xFF); > - if (s) { > - s = (*e) ? e+1 : e; > - } > + if (*e == '.') > + s = e + 1; > + else > + break; This change seems to be unrelated. Should it be a separate commit? Also, what happens with '192.168.4' with this change? > } > > addr.s_addr = htonl(addr.s_addr); > -- > 2.11.0.24.ge6920cf > Regards, Simon
On Mon, Dec 26, 2016 at 6:23 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Chris, > > On 20 December 2016 at 11:01, Chris Packham <judge.packham@gmail.com> wrote: >> Previously values greater than 255 were implicitly truncated. Add some >> stricter checking to reject addresses with components >255. >> >> With the input "1234192.168.1.1" the old behaviour would truncate the >> address to 192.168.1.1. New behaviour rejects the string outright and >> returns 0.0.0.0, which for the purposes of IP addresses can be >> considered an error. >> >> Signed-off-by: Chris Packham <judge.packham@gmail.com> >> --- >> This was part of my long running IPv6 patchset (which I promise I'll get >> back to someday). But I feel this stands on it's own merits. >> >> lib/net_utils.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/lib/net_utils.c b/lib/net_utils.c >> index cfae84275241..f148b8a70a7d 100644 >> --- a/lib/net_utils.c >> +++ b/lib/net_utils.c >> @@ -24,11 +24,16 @@ struct in_addr string_to_ip(const char *s) >> >> for (addr.s_addr = 0, i = 0; i < 4; ++i) { >> ulong val = s ? simple_strtoul(s, &e, 10) : 0; >> + if (val > 255) { >> + addr.s_addr = 0; >> + return addr; >> + } >> addr.s_addr <<= 8; >> addr.s_addr |= (val & 0xFF); >> - if (s) { >> - s = (*e) ? e+1 : e; >> - } >> + if (*e == '.') >> + s = e + 1; >> + else >> + break; > > This change seems to be unrelated. Should it be a separate commit? I should at least mention it's purpose in the commit message. It ensures that '.' is used as a separator and not some other arbitrary ascii character. > Also, what happens with '192.168.4' with this change? > Good point. I think it would be parsed as 0.192.168.4 which is clearly wrong. The else should probably set s_addr to 0 to flag the error. >> } >> >> addr.s_addr = htonl(addr.s_addr); >> -- >> 2.11.0.24.ge6920cf >> > > Regards, > Simon
diff --git a/lib/net_utils.c b/lib/net_utils.c index cfae84275241..f148b8a70a7d 100644 --- a/lib/net_utils.c +++ b/lib/net_utils.c @@ -24,11 +24,16 @@ struct in_addr string_to_ip(const char *s) for (addr.s_addr = 0, i = 0; i < 4; ++i) { ulong val = s ? simple_strtoul(s, &e, 10) : 0; + if (val > 255) { + addr.s_addr = 0; + return addr; + } addr.s_addr <<= 8; addr.s_addr |= (val & 0xFF); - if (s) { - s = (*e) ? e+1 : e; - } + if (*e == '.') + s = e + 1; + else + break; } addr.s_addr = htonl(addr.s_addr);
Previously values greater than 255 were implicitly truncated. Add some stricter checking to reject addresses with components >255. With the input "1234192.168.1.1" the old behaviour would truncate the address to 192.168.1.1. New behaviour rejects the string outright and returns 0.0.0.0, which for the purposes of IP addresses can be considered an error. Signed-off-by: Chris Packham <judge.packham@gmail.com> --- This was part of my long running IPv6 patchset (which I promise I'll get back to someday). But I feel this stands on it's own merits. lib/net_utils.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)