ubi: replace simple_strtoul() with kstrtoul()

PATCH In review Zhang Zhen 2 participants 5 messages May 19, 2014 – May 20, 2014 1 message not in the archive

Subsystems (2)

Memory technology devices (MTD) Unsorted block images (UBI)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Zhang Zhen

author

2014-05-19 08:39 UTC #1 lore raw

PATCH 1/1 ubi: replace simple_strtoul() with kstrtoul()

use the newer and more pleasant kstrtoul() to replace simple_strtoul(), because simple_strtoul() is marked for obsoletion.

Signed-off-by Zhang Zhen <[email protected]>

Signed-off-by hujianyang <[email protected]>

---
 drivers/mtd/ubi/build.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

drivers/mtd/ubi/build.c +10 -3

index 6e30a3c..80c539c 100644
@@ -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;

Signature · 4 lines

1.8.1.2


.

Geert Uytterhoeven

2014-05-19 09:15 UTC #2 lore raw

Please don't add mindless casts!

drivers/mtd/ubi/build.c static struct mtd_info * __init open_mtd_by_chdev(const char *mtd_dev) · line 1197

Quoted 13 lines · Zhang Zhen

drivers/mtd/ubi/build.c +3 -2

@@ -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 -- [email protected]

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

Zhang Zhen

author

2014-05-20 01:19 UTC #3 lore raw

Quoted 13 lines · Geert Uytterhoeven

Please don't add mindless casts!

Quoted 10 lines · Zhang Zhen

drivers/mtd/ubi/build.c +1 -1

@@ -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;

Quoted 6 lines

drivers/mtd/ubi/build.c static struct mtd_info * __init open_mtd_by_chdev(const char *mtd_dev) · line 1197
Quoted 3 lines
drivers/mtd/ubi/build.c +2 -1
-       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.

Quoted 13 lines

Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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

Geert Uytterhoeven

2014-05-20 06:57 UTC #4 lore raw

Quoted 21 lines · Zhang Zhen

Quoted 13 lines · Geert Uytterhoeven
Please don't add mindless casts!
Quoted 10 lines · Zhang Zhen
drivers/mtd/ubi/build.c +1 -1
@@ -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;
 
Quoted 6 lines
drivers/mtd/ubi/build.c static struct mtd_info * __init open_mtd_by_chdev(const char *mtd_dev) · line 1197
Quoted 3 lines
drivers/mtd/ubi/build.c +2 -1
-       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.

Quoted 8 lines

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 -- [email protected]

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

Zhang Zhen

author

2014-05-20 08:15 UTC #5 lore raw

Quoted 24 lines · Geert Uytterhoeven

Quoted 23 lines · Zhang Zhen
Quoted 13 lines · Geert Uytterhoeven
Please don't add mindless casts!
Quoted 10 lines · Zhang Zhen
drivers/mtd/ubi/build.c +1 -1
@@ -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;
 
Quoted 6 lines
drivers/mtd/ubi/build.c static struct mtd_info * __init open_mtd_by_chdev(const char *mtd_dev) · line 1197
Quoted 3 lines
drivers/mtd/ubi/build.c +2 -1
-       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.

Quoted 9 lines

Quoted 6 lines
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.

Quoted 12 lines

Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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