| Message ID | 20241021211451.GB835676@lichtman.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Replace the use of simple_strtol/ul functions with kstrto | expand |
Hi, On Mon, Oct 21, 2024 at 2:14 PM Nir Lichtman <nir@lichtman.org> wrote: > > The simple_str* family of functions perform no error checking in > scenarios where the input value overflows the intended output variable. > This results in these functions successfully returning even when the > output does not match the input string. > > Or as it was mentioned [1], "...simple_strtol(), simple_strtoll(), > simple_strtoul(), and simple_strtoull() functions explicitly ignore > overflows, which may lead to unexpected results in callers." > Hence, the use of those functions is discouraged. > > This patch replaces all uses of the simple_strto* series of functions > with their safer kstrto* alternatives. > > Side effects of this patch: > - Every string to long or long long conversion using kstrto* is now > checked for failure. > - kstrto* errors are handled with appropriate `KDB_BADINT` wherever > applicable. > - A good side effect is that we end up saving a few lines of code > since unlike in simple_strto* functions, kstrto functions do not > need an additional "end pointer" variable, and the return values > of the latter can be directly checked in an "if" statement without > the need to define additional `ret` or `err` variables. > This, of course, results in cleaner, yet still easy to understand > code. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull > > Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com> Since this was Yuran's patch originally and is still substantially the same, keeping their sign-off first was good. ...but: * You should have kept Yuran as the "author". This should have happened automatically in your local "git" repo when you picked Yuran's patch and then made changes. If it didn't you could manually set the author with "git commit --amend --author=blah". Even if it's right in your local repository, though, it won't be right when someone applies your patch. This is because when someone else is the author the first line of the commit message is supposed to be "From: Original Author <their-email@address>". Your message doesn't have that so when I grab it it looks like it came from you. If you use a tool like "patman" or "b4" to post your patches (or even git-send-email) this is handled for you. * When you take someone else's patch and make a small change it's nice to mention what you changed in the commit message. If you want to see an example of where I took someone else's patch and posted it, you can see: http://lore.kernel.org/r/20241004171340.v2.1.I938c91d10e454e841fdf5d64499a8ae8514dc004@changeid ...though maybe ignore the fact that I managed to accidentally add a Signed-off-by for both of my email addresses--oops. You can see that it has a "From: " line that patman/git-send-email added for me and you can also see my mention of what I did: [dianders: rebased to modern kernels] > @@ -402,42 +402,15 @@ static void kdb_printenv(void) > */ > int kdbgetularg(const char *arg, unsigned long *value) > { > - char *endp; > - unsigned long val; > - > - val = simple_strtoul(arg, &endp, 0); > - > - if (endp == arg) { > - /* > - * Also try base 16, for us folks too lazy to type the > - * leading 0x... > - */ In my original response [1] I suggested that the removal of the base 16 fallback should probably have been done in a separate patch (in other words change this from a 2-patch series to a 3-patch series). That keeps the "cleanup" patch separate from the one that's changing functionality. It's not a huge deal but it would be nice if you could split it out. If, for some reason, you didn't split it out then you'd certainly have wanted to mention it in the commit message of the patch since it's a change in functionality. [1] https://lore.kernel.org/r/CAD=FV=VZ61XFb1Ks79BHr1jL1jwf_36wYXryy0ZXOz1xTQ9zOg@mail.gmail.com/
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index f5f7d7fb5936..5f4be507d79f 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -306,8 +306,8 @@ static int kdbgetulenv(const char *match, unsigned long *value) return KDB_NOTENV; if (strlen(ep) == 0) return KDB_NOENVVALUE; - - *value = simple_strtoul(ep, NULL, 0); + if (kstrtoul(ep, 0, value)) + return KDB_BADINT; return 0; } @@ -402,42 +402,15 @@ static void kdb_printenv(void) */ int kdbgetularg(const char *arg, unsigned long *value) { - char *endp; - unsigned long val; - - val = simple_strtoul(arg, &endp, 0); - - if (endp == arg) { - /* - * Also try base 16, for us folks too lazy to type the - * leading 0x... - */ - val = simple_strtoul(arg, &endp, 16); - if (endp == arg) - return KDB_BADINT; - } - - *value = val; - + if (kstrtoul(arg, 0, value)) + return KDB_BADINT; return 0; } int kdbgetu64arg(const char *arg, u64 *value) { - char *endp; - u64 val; - - val = simple_strtoull(arg, &endp, 0); - - if (endp == arg) { - - val = simple_strtoull(arg, &endp, 16); - if (endp == arg) - return KDB_BADINT; - } - - *value = val; - + if (kstrtou64(arg, 0, value)) + return KDB_BADINT; return 0; } @@ -473,10 +446,10 @@ int kdb_set(int argc, const char **argv) */ if (strcmp(argv[1], "KDBDEBUG") == 0) { unsigned int debugflags; - char *cp; + int ret; - debugflags = simple_strtoul(argv[2], &cp, 0); - if (cp == argv[2] || debugflags & ~KDB_DEBUG_FLAG_MASK) { + ret = kstrtouint(argv[2], 0, &debugflags); + if (ret || debugflags & ~KDB_DEBUG_FLAG_MASK) { kdb_printf("kdb: illegal debug flags '%s'\n", argv[2]); return 0; @@ -1619,10 +1592,10 @@ static int kdb_md(int argc, const char **argv) if (!argv[0][3]) valid = 1; else if (argv[0][3] == 'c' && argv[0][4]) { - char *p; - repeat = simple_strtoul(argv[0] + 4, &p, 10); + if (kstrtouint(argv[0] + 4, 10, &repeat)) + return KDB_BADINT; mdcount = ((repeat * bytesperword) + 15) / 16; - valid = !*p; + valid = 1; } last_repeat = repeat; } else if (strcmp(argv[0], "md") == 0) @@ -2083,15 +2056,10 @@ static int kdb_dmesg(int argc, const char **argv) if (argc > 2) return KDB_ARGCOUNT; if (argc) { - char *cp; - lines = simple_strtol(argv[1], &cp, 0); - if (*cp) + if (kstrtoint(argv[1], 0, &lines)) lines = 0; - if (argc > 1) { - adjust = simple_strtoul(argv[2], &cp, 0); - if (*cp || adjust < 0) - adjust = 0; - } + if (argc > 1 && (kstrtoint(argv[2], 0, &adjust) || adjust < 0)) + adjust = 0; } /* disable LOGGING if set */ @@ -2428,14 +2396,12 @@ static int kdb_help(int argc, const char **argv) static int kdb_kill(int argc, const char **argv) { long sig, pid; - char *endp; struct task_struct *p; if (argc != 2) return KDB_ARGCOUNT; - sig = simple_strtol(argv[1], &endp, 0); - if (*endp) + if (kstrtol(argv[1], 0, &sig)) return KDB_BADINT; if ((sig >= 0) || !valid_signal(-sig)) { kdb_printf("Invalid signal parameter.<-signal>\n"); @@ -2443,8 +2409,7 @@ static int kdb_kill(int argc, const char **argv) } sig = -sig; - pid = simple_strtol(argv[2], &endp, 0); - if (*endp) + if (kstrtol(argv[2], 0, &pid)) return KDB_BADINT; if (pid <= 0) { kdb_printf("Process ID must be large than 0.\n");