| Message ID | 537979E9.3060209@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Please don't add mindless casts! On Mon, May 19, 2014 at 5:26 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: > --- a/fs/ubifs/super.c > +++ b/fs/ubifs/super.c > @@ -1905,6 +1905,7 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) > struct ubi_volume_desc *ubi; > int dev, vol; dev is int > char *endptr; > + int ret; > > /* First, try to open using the device node path method */ > ubi = ubi_open_volume_path(name, mode); > @@ -1922,7 +1923,10 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) > if (!isdigit(name[3])) > return ERR_PTR(-EINVAL); > > - dev = simple_strtoul(name + 3, &endptr, 0); > + endptr = (char *)name + 3; > + ret = kstrtoul(endptr, 0, (unsigned long *)&dev); On 64-bit, long is 64-bit, hence this will write beyond dev and will corrupt the stack. > + if (ret) > + return ERR_PTR(-EINVAL); > > /* ubiY method */ > if (*endptr == '\0') Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 2014/5/19 17:13, Geert Uytterhoeven wrote: > Please don't add mindless casts! > > On Mon, May 19, 2014 at 5:26 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: >> --- a/fs/ubifs/super.c >> +++ b/fs/ubifs/super.c >> @@ -1905,6 +1905,7 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) >> struct ubi_volume_desc *ubi; >> int dev, vol; > > dev is int > >> char *endptr; >> + int ret; >> >> /* First, try to open using the device node path method */ >> ubi = ubi_open_volume_path(name, mode); >> @@ -1922,7 +1923,10 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) >> if (!isdigit(name[3])) >> return ERR_PTR(-EINVAL); >> >> - dev = simple_strtoul(name + 3, &endptr, 0); >> + endptr = (char *)name + 3; >> + ret = kstrtoul(endptr, 0, (unsigned long *)&dev); > > On 64-bit, long is 64-bit, hence this will write beyond dev and will corrupt > the stack. > Yeah, you are right. This really may write beyond dev. The kstrtoul(const char *s, unsigned int base, unsigned long *res) only accept unsigned long pointer as the third parameter. And the original function simple_strtoul() returns unsigned long type value. It is also cast. So this may not corrupt the stack. Or do you have any better suggestion about this? Thanks. >> + if (ret) >> + return ERR_PTR(-EINVAL); >> >> /* ubiY method */ >> if (*endptr == '\0') > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > >
Hi Zhang, On Mon, May 19, 2014 at 11:49 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: > On 2014/5/19 17:13, Geert Uytterhoeven wrote: >> Please don't add mindless casts! >> >> On Mon, May 19, 2014 at 5:26 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: >>> --- a/fs/ubifs/super.c >>> +++ b/fs/ubifs/super.c >>> @@ -1905,6 +1905,7 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) >>> struct ubi_volume_desc *ubi; >>> int dev, vol; >> >> dev is int >> >>> char *endptr; >>> + int ret; >>> >>> /* First, try to open using the device node path method */ >>> ubi = ubi_open_volume_path(name, mode); >>> @@ -1922,7 +1923,10 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) >>> if (!isdigit(name[3])) >>> return ERR_PTR(-EINVAL); >>> >>> - dev = simple_strtoul(name + 3, &endptr, 0); >>> + endptr = (char *)name + 3; >>> + ret = kstrtoul(endptr, 0, (unsigned long *)&dev); >> >> On 64-bit, long is 64-bit, hence this will write beyond dev and will corrupt >> the stack. >> > Yeah, you are right. This really may write beyond dev. > > The kstrtoul(const char *s, unsigned int base, unsigned long *res) only accept unsigned long > pointer as the third parameter. > And the original function simple_strtoul() returns unsigned long type value. > It is also cast. So this may not corrupt the stack. That's not a cast, but an implicit conversion (which may truncate the value from 64-bit to 32-bit). > Or do you have any better suggestion about this? Change dev and vol to long? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, May 20, 2014 at 8:53 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, May 19, 2014 at 11:49 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: >> On 2014/5/19 17:13, Geert Uytterhoeven wrote: >>> Please don't add mindless casts! >>> >>> On Mon, May 19, 2014 at 5:26 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: >>>> --- a/fs/ubifs/super.c >>>> +++ b/fs/ubifs/super.c >>>> @@ -1905,6 +1905,7 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) >>>> struct ubi_volume_desc *ubi; >>>> int dev, vol; >>> >>> dev is int >>> >>>> char *endptr; >>>> + int ret; >>>> >>>> /* First, try to open using the device node path method */ >>>> ubi = ubi_open_volume_path(name, mode); >>>> @@ -1922,7 +1923,10 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) >>>> if (!isdigit(name[3])) >>>> return ERR_PTR(-EINVAL); >>>> >>>> - dev = simple_strtoul(name + 3, &endptr, 0); >>>> + endptr = (char *)name + 3; >>>> + ret = kstrtoul(endptr, 0, (unsigned long *)&dev); >>> >>> On 64-bit, long is 64-bit, hence this will write beyond dev and will corrupt >>> the stack. >>> >> Yeah, you are right. This really may write beyond dev. >> >> The kstrtoul(const char *s, unsigned int base, unsigned long *res) only accept unsigned long >> pointer as the third parameter. >> And the original function simple_strtoul() returns unsigned long type value. >> It is also cast. So this may not corrupt the stack. > > That's not a cast, but an implicit conversion (which may truncate the > value from 64-bit to 32-bit). > >> Or do you have any better suggestion about this? > > Change dev and vol to long? Oh, there exists a whole range of kstrto*() functions. So you can just use kstrtoint(). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 2014/5/20 14:57, Geert Uytterhoeven wrote: > On Tue, May 20, 2014 at 8:53 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Mon, May 19, 2014 at 11:49 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: >>> On 2014/5/19 17:13, Geert Uytterhoeven wrote: >>>> Please don't add mindless casts! >>>> >>>> On Mon, May 19, 2014 at 5:26 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: >>>>> --- a/fs/ubifs/super.c >>>>> +++ b/fs/ubifs/super.c >>>>> @@ -1905,6 +1905,7 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) >>>>> struct ubi_volume_desc *ubi; >>>>> int dev, vol; >>>> >>>> dev is int >>>> >>>>> char *endptr; >>>>> + int ret; >>>>> >>>>> /* First, try to open using the device node path method */ >>>>> ubi = ubi_open_volume_path(name, mode); >>>>> @@ -1922,7 +1923,10 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) >>>>> if (!isdigit(name[3])) >>>>> return ERR_PTR(-EINVAL); >>>>> >>>>> - dev = simple_strtoul(name + 3, &endptr, 0); >>>>> + endptr = (char *)name + 3; >>>>> + ret = kstrtoul(endptr, 0, (unsigned long *)&dev); >>>> >>>> On 64-bit, long is 64-bit, hence this will write beyond dev and will corrupt >>>> the stack. >>>> >>> Yeah, you are right. This really may write beyond dev. >>> >>> The kstrtoul(const char *s, unsigned int base, unsigned long *res) only accept unsigned long >>> pointer as the third parameter. >>> And the original function simple_strtoul() returns unsigned long type value. >>> It is also cast. So this may not corrupt the stack. >> >> That's not a cast, but an implicit conversion (which may truncate the >> value from 64-bit to 32-bit). >> >>> Or do you have any better suggestion about this? >> >> Change dev and vol to long? > > Oh, there exists a whole range of kstrto*() functions. > > So you can just use kstrtoint(). > Hi Geert, Thanks for your suggestion, i will send the version2 out. > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > >
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index a81c7b5..a30f297 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -1905,6 +1905,7 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) struct ubi_volume_desc *ubi; int dev, vol; char *endptr; + int ret; /* First, try to open using the device node path method */ ubi = ubi_open_volume_path(name, mode); @@ -1922,7 +1923,10 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) if (!isdigit(name[3])) return ERR_PTR(-EINVAL); - dev = simple_strtoul(name + 3, &endptr, 0); + endptr = (char *)name + 3; + ret = kstrtoul(endptr, 0, (unsigned long *)&dev); + if (ret) + return ERR_PTR(-EINVAL); /* ubiY method */ if (*endptr == '\0') @@ -1930,7 +1934,10 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode) /* ubiX_Y method */ if (*endptr == '_' && isdigit(endptr[1])) { - vol = simple_strtoul(endptr + 1, &endptr, 0); + endptr = endptr + 1; + ret = kstrtoul(endptr, 0, (unsigned long *)&vol); + if (ret) + return ERR_PTR(-EINVAL); if (*endptr != '\0') return ERR_PTR(-EINVAL); return ubi_open_volume(dev, vol, mode);
use the newer and more pleasant kstrtoul() to replace simple_strtoul(), because simple_strtoul() is marked for obsoletion. Signed-off-by: Zhang Zhen <zhenzhang.zhang@huawei.com> --- fs/ubifs/super.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)