linux-kernel - Re: [PATCH 1/2] dyndbg: dont panic over bad input

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJfuBxz7sz7816zqZURDcubdJT6Wz056eVyMWWL46UD58bxyvw@mail.gmail.com>
Date:   Tue, 22 Sep 2020 08:23:48 -0600
From:   jim.cromie@...il.com
To:     Petr Mladek <pmladek@...e.com>
Cc:     Jason Baron <jbaron@...mai.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] dyndbg: dont panic over bad input

On Tue, Sep 22, 2020 at 2:08 AM Petr Mladek <pmladek@...e.com> wrote:
>
> On Mon 2020-09-21 13:04:32, Jim Cromie wrote:
> > This BUG_ON, from 2009, caught the impossible case of a word-char both
> > starting and ending a string (loosely speaking).  A bad (reverted)
> > patch finally hit this case, but even "impossibly bad input" is no
> > reason to panic the kernel.  Instead pr_err and return -EINVAL.
> >
> > Reported-by: Petr Mladek <pmladek@...e.com>
> > Signed-off-by: Jim Cromie <jim.cromie@...il.com>
> > ---
> >  lib/dynamic_debug.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 2d4dfd44b0fa5..90ddf07ce34fe 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -259,7 +259,10 @@ static int ddebug_tokenize(char *buf, char *words[], int maxwords)
> >               } else {
> >                       for (end = buf; *end && !isspace(*end); end++)
> >                               ;
> > -                     BUG_ON(end == buf);
> > +                     if (end == buf) {
> > +                             pr_err("expected non-empty bareword");
> > +                             return -EINVAL;
>
> My understanding is that the BUG_ON() was there to catch eventual bugs
> in the algorithm.
>
> IMHO, it was never reachable in the original code:
>
>    1. The following lines will skip all spaces and bail out
>       when we reached the trailing '\0':
>
>                 buf = skip_spaces(buf);
>                 if (!*buf)
>                         break;  /* oh, it was trailing whitespace */
>
>
>    2. The following code will find the end of a quoted text:
>
>                 if (*buf == '"' || *buf == '\'') {
>                         int quote = *buf++;
>                         for (end = buf; *end && *end != quote; end++)
>
>
>     3. The else part will find end of non-quoted word:
>
>                 } else {
>                         for (end = buf; *end && !isspace(*end); end++)
>
>     4. The BUG_ON() will trigger when the above cycle reached the
>        trailing '\0' or space.
>
>        This will never happen because this situation was caught
>        in the 1st step.
>
>
> Your patch triggered the BUG_ON() because it wanted to handle
> '=' as a special character and was incomplete.
>
> If you want to handle '=' special way. You need to do it the same way
> as with the space. You need to skip it in 1st step. And it must mark
> the end of the word in 4th step.
>
> But it will be more complicated. You must be able to handle
> mix of spaces and '='. I mean the following situations:
>
>     word=word
>     word =word
>     word= word
>     word = word
>     word = = word   /* failure ? */
>
>
> Back to the BUG_ON(). It might be changed to something like:
>
>         pr_err("Internal error when parsing dynamic debug query\n");
>         return -EINVAL;
>
>
> Best Regards,
> Petr


yes, the original patch was ill conceived

Im blaming Transient Acute Myopia,
where I couldnt see to the end of the line, where "=flags"
is a proper flag setting.

That "interferes" with using '=' to separate keyword=value.

As you outlined, its possible to implement something that handles both,
but I decided that handling keyword=value is just a convenience feature,
and isnt worth the added corner-cases and explanatory burden.