Re: [PATCH 1/2] kdb: Replace the use of simple_strto with safer kstrto in kdb_main

[Doug Anderson]

Hi,

On Sun, Nov 19, 2023 at 4:07 PM Yuran Pereira <yuran.pereira@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
>  kernel/debug/kdb/kdb_main.c | 70 +++++++++++--------------------------
>  1 file changed, 21 insertions(+), 49 deletions(-)

Sorry for taking so long to review this--it arrived in my inbox at a
bad time. A few minor nits below that I think should be fixed before
landing but overall I think it's a nice cleanup. Thanks!


> @@ -412,42 +412,21 @@ 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)
> +       /*
> +        * If the first fails, also try base 16, for us
> +        * folks too lazy to type the leading 0x...
> +        */
> +       if (kstrtoul(arg, 0, value))
> +               if (kstrtoul(arg, 16, value))

Not new to your patch, but the above seems like a terrible idea to me.
What that means is that:

kdbgetularg("18", &value) => value is 18
kdbgetularg("19", &value) => value is 19
kdbgetularg("1a", &value) => value is 26

Bleh! If someone wants hex then they should put the 0x first.

I'd suggest a followup patch that removes the fallback for the lazy
folks. Here and in the next function...


> @@ -2095,15 +2074,11 @@ 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)
> +               if (argc > 1)
> +                       if (kstrtouint(argv[2], 0, &adjust) || adjust < 0)

My gut reaction is that some sort of build bot is going to come and
yell at you about the above line. Even if it doesn't, it's a bit
confusing. You're passing a pointer to an int into a function that
expects a pointer to an unsigned int. Most things don't really care
about signed/unsigned, but I could swear that some compilers get mad
when you start working with pointers to those types...

In any case, I think everything would work fine if you just change it
to kstrtoint(), right? I guess the other option would be to change the
variable to unsigned, but I guess that doesn't make sense since it's a
modifier to "lines" which is an int.

Side note: I didn't even know about the "adjust" argument, since it's
not in the help text in the command table below. I guess that could be
fixed in a separate patch.

nit: IMO if you have nested "if" statements then the outer one should
have braces. AKA:

if (a) {
  if (b)
    blah();
}

instead of:

if (a)
  if (b)
    blah();

...or you could do better and just change it to:

if (a && b)
  blah();