linux-kernel - Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

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]

Date:   Thu, 15 Mar 2018 18:35:10 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        "Tobin C . Harding" <me@...in.cc>, Joe Perches <joe@...ches.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...e.cz>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [PATCH v3] vsprintf: Prevent crash when dereferencing invalid pointers

On Thu, Mar 15, 2018 at 6:18 PM, Sergey Senozhatsky
<sergey.senozhatsky.work@...il.com> wrote:
>
> Hm, may be sizeof(ptr) still won't suffice. It would be great if we
> could always look at spec.field_width, which can be up to 2 * sizeof(void *),
> and then just probe_kernel_read(spec.field_width). E.g., %b/%bl prints out a
> bitmap, accessing max_t(int, spec.field_width, 0) bits, which is good. But,
> for instance, %U (uuid printout) doesn't look at spec.field_width, and reads
> in 16 bytes from the given memory address. Then we have ipv4/ipv6, mac, etc.
> So I think that checking just 1 byte or sizeof(ptr) is not really enough if
> we want to fix vsprintf. What do you think?

Honestly, I think it would be better to move the whole logic to the
functions that actually do the printout.

Then you can do it right, and you don't need to have the strchr() either.

There really isn't any commonality between the different versions.
field_width is meaningless, since it's about the size of the _printed_
field, not the size in memory.

Would it be a few more lines? Yes. But it would also clarify the code
and get all the cases right. Look at hex_string() for example, and
imagine fetching a byte at a time and just getting the corner cases
automatically right.

And if you don't care, just do a

       const char *err = check_pointer_access(ptr);
       if (err)
                return string(buf, end, err, spec);

at the top of each function that dereferences things. Some of them
already have the equivalent of that (the afore-mentioned hex_string:

        if (ZERO_OR_NULL_PTR(addr))
                /* NULL pointer */
                return string(buf, end, NULL, spec);

and it certainly is neither a lot of extra code, nor subtle or complex
to just do that (except using "err = check_pointer_access(ptr);").

               Linus