Messages in this thread |  | | Subject | Re: [PATCH v4 8/9] vsprintf: Prevent crash when dereferencing invalid pointers | From | Rasmus Villemoes <> | Date | Thu, 5 Apr 2018 16:46:23 +0200 |
| |
On 2018-04-04 10:58, Petr Mladek wrote:
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 3551b7957d9e..1a080a75a825 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -599,12 +599,46 @@ char *__string(char *buf, char *end, const char *s, struct printf_spec spec) > return widen_string(buf, len, end, spec); > } > > + /* > + * This is not a fool-proof test. 99% of the time that this will fault is > + * due to a bad pointer, not one that crosses into bad memory. Just test > + * the address to make sure it doesn't fault due to a poorly added printk > + * during debugging. > + */ > +static const char *check_pointer_access(const void *ptr) > +{ > + char byte; > + > + if (!ptr) > + return "(null)"; > + > + if (probe_kernel_address(ptr, byte)) > + return "(efault)"; > + > + return NULL; > +}
So while I think the WARNings are mostly pointless for the bad format specifiers, I'm wondering why an averted crash is not worth a WARN_ONCE? This means there's an actual bug somewhere, probably even exploitable, but we're just silently producing some innocent string...
Also, I'd still prefer to insist on ptr being a kernel pointer. Sure, for %ph userspace gets to print their own memory, but for a lot of the others, we're chasing pointers another level, so if an attacker can feed a user pointer to one of those, there's a trivial arbitrary read gadget. We have lots of printks in untested error paths, and I find it quite likely that one of those uses a garbage pointer.
I know you're mostly phrasing this in terms of preventing a crash, but it seems silly not to close that when it only costs a pointer comparison.
You're also missing the %pD (struct file*) case, which is one of those double-pointer chasing cases.
Rasmus
|  |