| Message ID | 5379C2E8.5040000@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Please don't add mindless casts! On Mon, May 19, 2014 at 10:38 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -1190,10 +1190,13 @@ static struct mtd_info * __init open_mtd_by_chdev(const char *mtd_dev) > static struct mtd_info * __init open_mtd_device(const char *mtd_dev) > { > struct mtd_info *mtd; > - int mtd_num; > + int mtd_num, ret; > char *endp; > > - mtd_num = simple_strtoul(mtd_dev, &endp, 0); > + endp = (char *)mtd_dev; > + ret = kstrtoul(endp, 0, (unsigned long *)&mtd_num); On 64-bit, long is 64-bit, hence this will write beyond mtd_num and will corrupt the stack. 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:14, Geert Uytterhoeven wrote: > Please don't add mindless casts! > > On Mon, May 19, 2014 at 10:38 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: >> --- a/drivers/mtd/ubi/build.c >> +++ b/drivers/mtd/ubi/build.c >> @@ -1190,10 +1190,13 @@ static struct mtd_info * __init open_mtd_by_chdev(const char *mtd_dev) >> static struct mtd_info * __init open_mtd_device(const char *mtd_dev) >> { >> struct mtd_info *mtd; >> - int mtd_num; >> + int mtd_num, ret; >> char *endp; >> >> - mtd_num = simple_strtoul(mtd_dev, &endp, 0); >> + endp = (char *)mtd_dev; >> + ret = kstrtoul(endp, 0, (unsigned long *)&mtd_num); > > On 64-bit, long is 64-bit, hence this will write beyond mtd_num 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. > > 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 3:18 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: > On 2014/5/19 17:14, Geert Uytterhoeven wrote: >> Please don't add mindless casts! >> >> On Mon, May 19, 2014 at 10:38 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: >>> --- a/drivers/mtd/ubi/build.c >>> +++ b/drivers/mtd/ubi/build.c >>> @@ -1190,10 +1190,13 @@ static struct mtd_info * __init open_mtd_by_chdev(const char *mtd_dev) >>> static struct mtd_info * __init open_mtd_device(const char *mtd_dev) >>> { >>> struct mtd_info *mtd; >>> - int mtd_num; >>> + int mtd_num, ret; >>> char *endp; >>> >>> - mtd_num = simple_strtoul(mtd_dev, &endp, 0); >>> + endp = (char *)mtd_dev; >>> + ret = kstrtoul(endp, 0, (unsigned long *)&mtd_num); >> >> On 64-bit, long is 64-bit, hence this will write beyond mtd_num 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? 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 3:18 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: >> On 2014/5/19 17:14, Geert Uytterhoeven wrote: >>> Please don't add mindless casts! >>> >>> On Mon, May 19, 2014 at 10:38 AM, Zhang Zhen <zhenzhang.zhang@huawei.com> wrote: >>>> --- a/drivers/mtd/ubi/build.c >>>> +++ b/drivers/mtd/ubi/build.c >>>> @@ -1190,10 +1190,13 @@ static struct mtd_info * __init open_mtd_by_chdev(const char *mtd_dev) >>>> static struct mtd_info * __init open_mtd_device(const char *mtd_dev) >>>> { >>>> struct mtd_info *mtd; >>>> - int mtd_num; >>>> + int mtd_num, ret; >>>> char *endp; >>>> >>>> - mtd_num = simple_strtoul(mtd_dev, &endp, 0); >>>> + endp = (char *)mtd_dev; >>>> + ret = kstrtoul(endp, 0, (unsigned long *)&mtd_num); >>> >>> On 64-bit, long is 64-bit, hence this will write beyond mtd_num 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? > > 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/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index 6e30a3c..80c539c 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -1190,10 +1190,13 @@ static struct mtd_info * __init open_mtd_by_chdev(const char *mtd_dev) static struct mtd_info * __init open_mtd_device(const char *mtd_dev) { struct mtd_info *mtd; - int mtd_num; + int mtd_num, ret; char *endp; - mtd_num = simple_strtoul(mtd_dev, &endp, 0); + endp = (char *)mtd_dev; + ret = kstrtoul(endp, 0, (unsigned long *)&mtd_num); + if (ret) + return ERR_PTR(-EINVAL); if (*endp != '\0' || mtd_dev == endp) { /* * This does not look like an ASCII integer, probably this is @@ -1362,8 +1365,12 @@ static int __init bytes_str_to_int(const char *str) { char *endp; unsigned long result; + int ret; - result = simple_strtoul(str, &endp, 0); + endp = (char *)str; + ret = kstrtoul(endp, 0, &result); + if (ret) + return -EINVAL; if (str == endp || result >= INT_MAX) { ubi_err("incorrect bytes count: \"%s\"\n", str); return -EINVAL;