Re: [PATCH] x86/mm: Fix fault error path using unsafe vma pointer

[Laurent Dufour]

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 <dave.hansen@xxxxxxxxxxxxxxx>
> Signed-off-by: Laurent Dufour <ldufour@xxxxxxxxxxxxxxxxxx>

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;
>  	}
>  
>