Messages in this thread | | | Date | Tue, 11 Dec 2018 20:04:58 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH 2/2] vsprintf: Stop using obsolete simple_strtoul() |
| |
On Tue, Dec 11, 2018 at 09:22:22AM -0800, Linus Torvalds wrote: > On Tue, Dec 11, 2018 at 7:21 AM Thomas Preston > <thomas.preston@codethink.co.uk> wrote: > > > > Stop using the obsolete functions simple_strtoul() and > > simple_strtoull(). Instead, we should use the improved kstrtol() and > > kstrtoll() functions. To do this, we must copy the current field into a > > null-terminated tmpstr and advance the variable `next` manually. > > I see what you're trying to do, but this fix is much much worse than > the bug was. > > > + if (field_width > 0) { > > + char tmpstr[INT_BUF_LEN]; > > + int ret; > > + > > + strscpy(tmpstr, str, field_width+1); > > If field_width is larger than INT_BUF_LEN, you are now corrupting kernel stack. > > And no, you can't fix it by limiting field_width, since a large > field_width is quite possible and might even be valid - and still fit > in an int. Maybe the number is > > 000000000000000000000001 > > or something? > > A fix might be to skip leading zeroes. > > Honestly, just do it by hand. Don't use kstrol and friends at all. > Just do something like > > unsigned long long val = 0; > p = str; > for (;;) { > int c; > if (field_width > 0 && p - str >= field_width) > break; > c = hexval(*p++); > if (c < 0 || c > base) > break; > val = val * base + c; > // check for overflow
I think it's slightly more complicated, I run the following test case on glibc:
uint32_t hi, lo, t;
sscanf("00fafafafa0d0b0b0b0c000000", "%8x%8x%x", &hi, &lo, &t);
64-bit: HI: 00fafafa LO: fa0d0b0b (c000000) 32-bit: HI: 00fafafa LO: fa0d0b0b (ffffffff)
> } > /* Now do "sign" and range checking on val */ > /* Ta-daa, all done */ > > or similar. Treat the above as pseudo-code, I didn't fill in all the details. > > Linus
-- With Best Regards, Andy Shevchenko
| |