Messages in this thread | | | Subject | Re: [PATCH] x86/fault: Fix wrong signal when vsyscall fails with pkey | From | Dave Hansen <> | Date | Wed, 28 Jul 2021 10:57:58 -0700 |
| |
On 7/28/21 4:44 AM, Jiashuo Liang wrote: > The function __bad_area_nosemaphore calls kernelmode_fixup_or_oops with > parameter "signal" being "pkey", which will send a signal numbered "pkey".
Yikes.
> When emulating vsyscall, the kernel may fail to access user-given memory > pages that are protected by pkey. In such a case, the kernel should send a > SIGSEGV signal with si_code=SEGV_PKUERR and si_pkey=pkey.
This could use a bit more context.
First of all this is for user address space faults in the do_user_addr_fault() path. Second, the buggy code is under a !user_mode() check, so this must be a kernel fault in the user address space. Third, the only notice this problem when the page fault handler ends up delivering a signal as a result of the fault. Most cases will simply return an error code to the faulting kernel code which will see -EFAULT come back from copy_to/from_user() and friends.
The *only* condition in which we generate that signal from the fault handler is when current->thread.sig_on_uaccess_err=1, and the only place that gets used is in emulate_vsyscall().
This makes me want to add some code that tickles vsyscall emulation in the pkey selftests, but I think I'll resist the urge for now. :)
Is that all correct?
> So a new parameter "pkey" is added to kernelmode_fixup_or_oops to fix it.
Yeah, I think that's the right fix. You also need this:
Fixes: 5042d40a264c ("x86/fault: Bypass no_context() for implicit kernel faults from usermode")
I believe that's where this issue originated.
How did you find this, by the way?
> arch/x86/mm/fault.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c > index b2eefdefc108..883294282e1e 100644 > --- a/arch/x86/mm/fault.c > +++ b/arch/x86/mm/fault.c > @@ -710,7 +710,8 @@ page_fault_oops(struct pt_regs *regs, unsigned long error_code, > > static noinline void > kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code, > - unsigned long address, int signal, int si_code) > + unsigned long address, int signal, int si_code, > + u32 pkey) > { > WARN_ON_ONCE(user_mode(regs)); > > @@ -735,8 +736,12 @@ kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code, > > set_signal_archinfo(address, error_code); > > - /* XXX: hwpoison faults will set the wrong code. */ > - force_sig_fault(signal, si_code, (void __user *)address); > + if (si_code == SEGV_PKUERR) { > + force_sig_pkuerr((void __user *)address, pkey); > + } else { > + /* XXX: hwpoison faults will set the wrong code. */ > + force_sig_fault(signal, si_code, (void __user *)address); > + } > } > > /* > @@ -798,7 +803,8 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code, > struct task_struct *tsk = current; > > if (!user_mode(regs)) { > - kernelmode_fixup_or_oops(regs, error_code, address, pkey, si_code); > + kernelmode_fixup_or_oops(regs, error_code, address, > + SIGSEGV, si_code, pkey); > return; > } > > @@ -930,7 +936,8 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address, > { > /* Kernel mode? Handle exceptions or die: */ > if (!user_mode(regs)) { > - kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS, BUS_ADRERR); > + kernelmode_fixup_or_oops(regs, error_code, address, > + SIGBUS, BUS_ADRERR, 0); > return; > }
Could we please use ARCH_DEFAULT_PKEY instead of 0's in all these call sites? I just detest seeing mystery functions with lots of 0's and 1's as parameters.
| |