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

[Linus Torvalds]

On Thu, Mar 15, 2018 at 6:18 PM, Sergey Senozhatsky
<sergey.senozhatsky.work@xxxxxxxxx> 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