Messages in this thread | | | Date | Wed, 19 Oct 2022 23:00:09 +0200 | Subject | Re: [PATCH v3 1/1] vsprintf: protect kernel from panic due to non-canonical pointer dereference | From | Rasmus Villemoes <> |
| |
On 19/10/2022 21.41, Jane Chu wrote: > Having stepped on a local kernel bug where reading sysfs has led to > out-of-bound pointer dereference by vsprintf() which led to GPF panic.
Just to be completely clear, the out-of-bounds dereference did not happen in vsprintf if I understand your description right. Essentially you have an array of char* pointers, and you accessed beyond that array, where of course some random memory contents then turned out not to be a real pointer, and that bogus pointer value was passed into vsprintf() as a %s argument.
> And the reason for GPF is that the OOB pointer was turned to a > non-canonical address such as 0x7665645f63616465.
That's ved_cade , or more properly edac_dev ...
> > vsprintf() already has this line of defense > if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr)) > return "(efault)"; > Since a non-canonical pointer can be detected by kern_addr_valid() > on architectures that present VM holes as well as meaningful > implementation of kern_addr_valid() that detects the non-canonical > addresses, this patch adds a check on non-canonical string pointer by > kern_addr_valid() and "(efault)" to alert user that something > is wrong instead of unecessarily panic the server. > > On the other hand, if the non-canonical string pointer is dereferenced > else where in the kernel, by virtue of being non-canonical, a crash > is expected to be immediate.
I'm with Andy on this one, we don't add random checks like this in the kernel, not in vsprintf or elsewhere.
check_pointer_msg is/was actually more about checking the various %p<foo> extensions, where it is (more) expected that somebody does
struct foo *f = get_a_foo(); pr_debug("got %pfoo\n", f); if (IS_ERR(f)) { ... }
[possibly in a not so obvious path], and the PAGE_SIZE check is similarly for cases where the "base" pointer is actually NULL but what is passed is &f->member.
Rasmus
| |