linux-kernel - Re: [PATCH RFC] auxdisplay: charlcd: Fix and clean up handling of x/y commands

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 27 Feb 2018 22:12:29 +0100
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Willy Tarreau <w@....eu>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Robert Abel <rabel@...ertabel.eu>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] auxdisplay: charlcd: Fix and clean up handling of x/y commands

On Tue, Feb 27, 2018 at 9:52 PM, Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
> On Tue, Feb 27, 2018 at 9:32 PM, Miguel Ojeda
> <miguel.ojeda.sandonis@...il.com> wrote:
>> The current version is not parsing multiple x/y commands as the code
>> originally intended. On top of that, kstrtoul() expects
>> NULL-terminated strings. Finally, the code had to do two passes over
>> the string, while now only one is done.
>>
>> Some explanations about the supported syntax are added as well.
>
> Something like this would work, but see my comments below.
>
>> +/*
>> + * Parses a base 10 number from a string, until a non-digit number is found or
>> + * until PARSE_N_MAX_SIZE digits.
>> + *
>> + * See kstrtoul() for the meaning of the return value.
>> + * It also returns the next character in the string, i.e. the first non-digit.
>> + */
>> +#define PARSE_N_MAX_SIZE (3)
>> +static unsigned long parse_n(const char *s, unsigned long *res,
>> +       const char **next_s)
>
> static int parse_n(const char *s, unsigned long *res, const char **next_s)

Oops, thanks!

>
>> +{
>> +       char tmp[PARSE_N_MAX_SIZE + 1];
>> +       int i;
>> +
>
> First of all you need
>
> unsigned int i = 0;
>
> while (s[i] == '0')
>  i++;

Why?

>
>> +       for (i = 0; ; ++i) {
>> +               if (!isdigit(s[i]))
>> +                       break;
>> +               if (i >= PARSE_N_MAX_SIZE)
>> +                       return -EINVAL;
>> +               tmp[i] = s[i];
>> +       }
>
> And here you partially open coded what simple_strto*() does.
>
>> +
>> +       tmp[i] = 0;
>> +       *next_s = &s[i];
>> +       return kstrtoul(tmp, 10, res);
>> +}
>
> It's over engineered. Just simple use simple_strto*() and that's all.
> Do you really care about overflows? Is those numbers somehow critical?
> What will go wrong?

We shouldn't use simple_strto*() since it is deprecated. So parse_n()
can be either this wrapper to call kstrto*() (which indeed is
overengineered, but easier to understand than Robert's patch that
modified the original sequence, and can be replaced by an inplace
version whenever we have one) or we just put a simple loop for the
moment (like the one Willy coded in 2008). Both are fine with me.

Cheers,
Miguel

>
> --
> With Best Regards,
> Andy Shevchenko