On Tue, Aug 7, 2018 at 4:55 AM Andy Lutomirski <[email protected]> wrote: > > On Aug 6, 2018, at 6:22 PM, Jann Horn <[email protected]> wrote: > > > > There have been multiple kernel vulnerabilities that permitted userspace to > > pass completely unchecked pointers through to userspace accessors: > > > > - the waitid() bug - commit 96ca579a1ecc ("waitid(): Add missing > > access_ok() checks") > > - the sg/bsg read/write APIs > > - the infiniband read/write APIs > > > > These don't happen all that often, but when they do happen, it is hard to > > test for them properly; and it is probably also hard to discover them with > > fuzzing. Even when an unmapped kernel address is supplied to such buggy > > code, it just returns -EFAULT instead of doing a proper BUG() or at least > > WARN(). > > > > This patch attempts to make such misbehaving code a bit more visible by > > WARN()ing in the pagefault handler code when a userspace accessor causes > > #PF on a kernel address and the current context isn't whitelisted. > > I like this a lot, and, in fact, I once wrote a patch to do something > similar. It was before the fancy extable code, though, so it was a mess. > Here are some thoughts: > > - It should be three patches. One patch to add the _UA annotations, one to > improve the info passes to the handlers, and one to change behavior. > > - You should pass the vector, the error code, and the address to the handler. > > - The uaccess handler should IMO WARN if the vector is anything other than > #PF (which mainly means warning if it’s #GP). I think it should pr_emerg() > and return false if the vector is #PF and the address is too high.
What about #MC? do_machine_check() sometimes invokes fixup handlers. It looks like fixup_exception() is basically reached for anything that can't be restarted (either MCG_STATUS_RIPV isn't set or the worst severity is MCE_AR_SEVERITY), but doesn't reach MCE_PANIC_SEVERITY? In particular for "Action required: data load in error recoverable area of kernel", if I'm reading the code correctly. It seems like the code is intentionally preventing memory errors during user access from being treated as kernel memory errors? So perhaps #MC in user access should not WARN()? > - Arguably most non-uaccess fixups should at least warn for anything other > than #GP and #UD.
