Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751434AbdILJ2N (ORCPT ); Tue, 12 Sep 2017 05:28:13 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:45537 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751214AbdILJ2L (ORCPT ); Tue, 12 Sep 2017 05:28:11 -0400 Subject: Re: [PATCH] x86/mm: Fix fault error path using unsafe vma pointer From: Laurent Dufour To: Thomas Gleixner , Ingo Molnar , x86@kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org Cc: Dave Hansen References: <1504513935-12742-1-git-send-email-ldufour@linux.vnet.ibm.com> Date: Tue, 12 Sep 2017 11:28:05 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1504513935-12742-1-git-send-email-ldufour@linux.vnet.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17091209-0012-0000-0000-000005785427 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17091209-0013-0000-0000-000018F16A5B Message-Id: <4fe0c94c-855f-d45f-0d43-560761db8c89@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-09-12_04:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1709120131 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7598 Lines: 206 On 04/09/2017 10:32, Laurent Dufour wrote: > The commit 7b2d0dbac489 ("x86/mm/pkeys: Pass VMA down in to fault signal > generation code") pass down a vma pointer to the error path, but that is > done once the mmap_sem is released when calling mm_fault_error() from > __do_page_fault(). > > This is dangerous as the pointed vma structure is no more safe to be used > once the mmap_sem has been released. As only the protection key value is > required in the error processing, we could just pass down this value. > > This patch fixes this by passing a pointer to a protection key value down > to the fault signal generation code. The use of a pointer allows to keep > the check generating a warning message in fill_sig_info_pkey() when the vma > was not known. If the pointer is valid, the protection value can be > accessed by deferencing the pointer. > > Fixes: 7b2d0dbac489 ("x86/mm/pkeys: Pass VMA down in to fault signal > generation code") > Cc: Dave Hansen > Signed-off-by: Laurent Dufour Hi all, any interest to this ? Cheers, Laurent. > --- > arch/x86/mm/fault.c | 47 ++++++++++++++++++++++++----------------------- > 1 file changed, 24 insertions(+), 23 deletions(-) > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c > index 2a1fa10c6a98..c18e737c5f9b 100644 > --- a/arch/x86/mm/fault.c > +++ b/arch/x86/mm/fault.c > @@ -192,8 +192,7 @@ is_prefetch(struct pt_regs *regs, unsigned long error_code, unsigned long addr) > * 6. T1 : reaches here, sees vma_pkey(vma)=5, when we really > * faulted on a pte with its pkey=4. > */ > -static void fill_sig_info_pkey(int si_code, siginfo_t *info, > - struct vm_area_struct *vma) > +static void fill_sig_info_pkey(int si_code, siginfo_t *info, int *pkey) > { > /* This is effectively an #ifdef */ > if (!boot_cpu_has(X86_FEATURE_OSPKE)) > @@ -209,7 +208,7 @@ static void fill_sig_info_pkey(int si_code, siginfo_t *info, > * valid VMA, so we should never reach this without a > * valid VMA. > */ > - if (!vma) { > + if (!pkey) { > WARN_ONCE(1, "PKU fault with no VMA passed in"); > info->si_pkey = 0; > return; > @@ -219,13 +218,12 @@ static void fill_sig_info_pkey(int si_code, siginfo_t *info, > * absolutely guranteed to be 100% accurate because of > * the race explained above. > */ > - info->si_pkey = vma_pkey(vma); > + info->si_pkey = *pkey; > } > > static void > force_sig_info_fault(int si_signo, int si_code, unsigned long address, > - struct task_struct *tsk, struct vm_area_struct *vma, > - int fault) > + struct task_struct *tsk, int *pkey, int fault) > { > unsigned lsb = 0; > siginfo_t info; > @@ -240,7 +238,7 @@ force_sig_info_fault(int si_signo, int si_code, unsigned long address, > lsb = PAGE_SHIFT; > info.si_addr_lsb = lsb; > > - fill_sig_info_pkey(si_code, &info, vma); > + fill_sig_info_pkey(si_code, &info, pkey); > > force_sig_info(si_signo, &info, tsk); > } > @@ -758,8 +756,6 @@ no_context(struct pt_regs *regs, unsigned long error_code, > struct task_struct *tsk = current; > unsigned long flags; > int sig; > - /* No context means no VMA to pass down */ > - struct vm_area_struct *vma = NULL; > > /* Are we prepared to handle this kernel fault? */ > if (fixup_exception(regs, X86_TRAP_PF)) { > @@ -784,7 +780,7 @@ no_context(struct pt_regs *regs, unsigned long error_code, > > /* XXX: hwpoison faults will set the wrong code. */ > force_sig_info_fault(signal, si_code, address, > - tsk, vma, 0); > + tsk, NULL, 0); > } > > /* > @@ -893,8 +889,7 @@ show_signal_msg(struct pt_regs *regs, unsigned long error_code, > > static void > __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code, > - unsigned long address, struct vm_area_struct *vma, > - int si_code) > + unsigned long address, int *pkey, int si_code) > { > struct task_struct *tsk = current; > > @@ -942,7 +937,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code, > tsk->thread.error_code = error_code; > tsk->thread.trap_nr = X86_TRAP_PF; > > - force_sig_info_fault(SIGSEGV, si_code, address, tsk, vma, 0); > + force_sig_info_fault(SIGSEGV, si_code, address, tsk, pkey, 0); > > return; > } > @@ -955,9 +950,9 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code, > > static noinline void > bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code, > - unsigned long address, struct vm_area_struct *vma) > + unsigned long address, int *pkey) > { > - __bad_area_nosemaphore(regs, error_code, address, vma, SEGV_MAPERR); > + __bad_area_nosemaphore(regs, error_code, address, pkey, SEGV_MAPERR); > } > > static void > @@ -965,6 +960,10 @@ __bad_area(struct pt_regs *regs, unsigned long error_code, > unsigned long address, struct vm_area_struct *vma, int si_code) > { > struct mm_struct *mm = current->mm; > + int pkey; > + > + if (vma) > + pkey = vma_pkey(vma); > > /* > * Something tried to access memory that isn't in our memory map.. > @@ -972,7 +971,8 @@ __bad_area(struct pt_regs *regs, unsigned long error_code, > */ > up_read(&mm->mmap_sem); > > - __bad_area_nosemaphore(regs, error_code, address, vma, si_code); > + __bad_area_nosemaphore(regs, error_code, address, > + (vma) ? &pkey : NULL, si_code); > } > > static noinline void > @@ -1015,7 +1015,7 @@ bad_area_access_error(struct pt_regs *regs, unsigned long error_code, > > static void > do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address, > - struct vm_area_struct *vma, unsigned int fault) > + int *pkey, unsigned int fault) > { > struct task_struct *tsk = current; > int code = BUS_ADRERR; > @@ -1042,13 +1042,12 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address, > code = BUS_MCEERR_AR; > } > #endif > - force_sig_info_fault(SIGBUS, code, address, tsk, vma, fault); > + force_sig_info_fault(SIGBUS, code, address, tsk, pkey, fault); > } > > static noinline void > mm_fault_error(struct pt_regs *regs, unsigned long error_code, > - unsigned long address, struct vm_area_struct *vma, > - unsigned int fault) > + unsigned long address, int *pkey, unsigned int fault) > { > if (fatal_signal_pending(current) && !(error_code & PF_USER)) { > no_context(regs, error_code, address, 0, 0); > @@ -1072,9 +1071,9 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code, > } else { > if (fault & (VM_FAULT_SIGBUS|VM_FAULT_HWPOISON| > VM_FAULT_HWPOISON_LARGE)) > - do_sigbus(regs, error_code, address, vma, fault); > + do_sigbus(regs, error_code, address, pkey, fault); > else if (fault & VM_FAULT_SIGSEGV) > - bad_area_nosemaphore(regs, error_code, address, vma); > + bad_area_nosemaphore(regs, error_code, address, pkey); > else > BUG(); > } > @@ -1268,6 +1267,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code, > struct mm_struct *mm; > int fault, major = 0; > unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; > + int pkey; > > tsk = current; > mm = tsk->mm; > @@ -1468,9 +1468,10 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code, > return; > } > > + pkey = vma_pkey(vma); > up_read(&mm->mmap_sem); > if (unlikely(fault & VM_FAULT_ERROR)) { > - mm_fault_error(regs, error_code, address, vma, fault); > + mm_fault_error(regs, error_code, address, &pkey, fault); > return; > } > >