linux-kernel - Re: [PATCH v3 0/4] Make sscanf() stricter

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]

Message-ID: <ZJJKr0xYvcO7P8QY@itl-email>
Date:   Tue, 20 Jun 2023 20:56:11 -0400
From:   Demi Marie Obenour <demi@...isiblethingslab.com>
To:     Petr Mladek <pmladek@...e.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     David Laight <David.Laight@...lab.com>,
        Alexey Dobriyan <adobriyan@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Hans de Goede <hdegoede@...hat.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Juergen Gross <jgross@...e.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
        Lee Jones <lee@...nel.org>, Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCH v3 0/4] Make sscanf() stricter

On Tue, Jun 20, 2023 at 03:34:09PM +0200, Petr Mladek wrote:
> On Thu 2023-06-15 14:23:59, Andy Shevchenko wrote:
> > On Thu, Jun 15, 2023 at 08:06:46AM +0000, David Laight wrote:
> > > From: Demi Marie Obenour
> > > > Sent: 14 June 2023 21:09
> > 
> > ...
> > 
> > > > > What sort of formats and data are being used?
> > > > 
> > > > Base-10 or base-16 integers, with whitespace never being valid.
> > > 
> > > In which case sscanf() really isn't what you are looking for.
> > > 
> > > > > The "%s" format terminates on whitespace.
> > > > > Even stroul() (and friends) will skip leading whitespace.
> > > > 
> > > > Yes, which is a reason that strto*l() are just broken IMO.
> > >
> > > They are not 'broken', that is what is useful most of the time.
> > > The usual problem is that "020" is treated as octal.
> 
> I do not know how many users depend on this behavior. But I believe
> that there are such users. And breaking compatibility with userspace
> implementation would make more harm then good in this case.
> 
> > > > I’m trying to replace their uses in Xen with custom parsing code.
> > > 
> > > Then write a custom parser :-)
> 
> Honestly, I dislike any sscanf() modification which have been suggested
> so far:
> 
>   + %!d is not acceptable because it produces compiler errors
> 
>   + %d! is not acceptable because "use 64!" is a realistic string.
>     We could not be sure that "<number>!" will never be parsed
>     in kernel.
> 
>   + %d%[!] produces compiler error either. It is hard to parse by eyes.
>     Also the meaning of such a format would be far from obvious.
> 
>   + %pj or another %p modifiers would be hard to understand either.
> 
>     Yes, we have %pe but I think that only few people really use it.
>     And it is kind of self-explanatory because it is typically
>     used together with ERR_PTR() and with variables called
>     "err" or "ret".
> 
> 
> > Hmm... Usually we are against zillion implementations of the same with zillion
> > bugs hidden (each buggy implementation with its own bugs).
> 
> I would really like to see the code depending on it. The cover letter
> suggests that there already is a patch with such a custom parser.
> I am sorry if it has already been mentioned. There were so many threads.
> 
> Sure, we do not want two full featured sscanf() implementations. But a
> wrapper checking for leading whitespace and using kstrto<foo>
> family does not sound too complex.
> 
> There should always be a good reason to introduce an incompatibility
> between the kernel and the userspace implementation of a commonly
> used API.
> 
> Best Regards,
> Petr

I strongly believe that overflow should be forbidden by default, but it
turns out that I do not have time to advance this patch further.  My
understanding is that Xen never wants to allow spaces in Xenstore
entries, but that is easy to ensure via an explicit check prior to
calling vsscanf().
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)