linux-kernel - Re: [PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

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: <20180302125118.bjd3tbuu72vgfczo@pathway.suse.cz>
Date:   Fri, 2 Mar 2018 13:51:18 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     "Tobin C . Harding" <me@...in.cc>, linux@...musvillemoes.dk,
        Joe Perches <joe@...ches.com>, linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...e.cz>
Subject: Re: [PATCH v2 8/9] lib/vsprintf: Remove useless NULL checks

On Wed 2018-02-28 12:42:24, Andy Shevchenko wrote:
> On Wed, 2018-02-28 at 11:04 +0100, Petr Mladek wrote:
> > On Tue 2018-02-27 19:35:50, Andy Shevchenko wrote:
> > > On Tue, 2018-02-27 at 16:50 +0100, Petr Mladek wrote:
> > Note that it will most likely crash in vprintk_emit() on the line
> > 
> >    text_len = vscnprintf(text, sizeof(textbuf), fmt, args);
> > 
> > It will be with logbug_lock() taken. The nested printk() messages
> > will be stored in per-CPU buffer thanks to printk_safe code.
> 
> Yeah, that's bad.
> 
> > IMHO, it would make sense to hanve this check also pointers that are
> > being deferred.
> 
> Send a patch to discuss!

I thought about this more. IMHO, the check for PAGE_SIZE in pointer()
makes perfect sense. It helps to avoid crash and actually see
the message. I am going to send the patch in a minute.

BTW: I am not sure who is going to pass this patchset to Linus.
If nobody is against, I could eventually do so via printk.git.


> > > > To be honest, I do not feel experienced enough to decide
> > > > about the preferred behavior. On one hand, it is bad when
> > > > printk() would crash the kernel. On the other hand, hiding wide
> > > > range of values under "(null)" string might confuse people.
> > > > Would it make sense to survive and write different strings for
> > > > difference intervals? For example?
> > > > 
> > > >     "(null)"     for ptr == 0
> > > >     "(null-16)"  for ptr > 0 && ptr <= 16
> > > >     "(null-pg)"  for prt > 16 && ptr <= PAGE_SIZE
> > > > 
> > > > In each case, this patch changes the behavior and it should
> > > > be documented in the commit message.
> > > 
> > > Personally I strongly disagree with blowing code up in such places
> > > for little or none benefit.
> 
> Send a patch to discuss!

I am not going to do so unless there is an evidence that people are
confused or that the above idea is desired.

Best Regards,
Petr