Messages in this thread | | | Subject | Re: [PATCH] x86/fault: Fix wrong signal when vsyscall fails with pkey | From | Jiashuo Liang <> | Date | Thu, 29 Jul 2021 14:24:45 +0800 |
| |
On Wed, 2021-07-28 at 10:57 -0700, Dave Hansen wrote: >> 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?
Right.
>> 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.
Yeah, we need to add it.
> How did you find this, by the way?
I was learning about memory protection key. So I read the related code in kernel and spotted this.
>> 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.
I agree that using ARCH_DEFAULT_PKEY is better. I think I am supposed to send a patch v2 for the update?
By the way, the magic pkey number 0 also appears when bad_area_nosemaphore calls __bad_area_nosemaphore and bad_area calls __bad_area. Do they need to be changed to ARCH_DEFAULT_PKEY as well?
Thanks!
Jiashuo Liang
| |