Re: [PATCH 2/3] arm64: signal: sigreturn() and rt_sigreturn() sometime returns the wrong signals

From: Liam Howlett <liam.howlett@oracle.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Julien Grall <julien.grall@arm.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>
Subject: Re: [PATCH 2/3] arm64: signal: sigreturn() and rt_sigreturn() sometime returns the wrong signals
Date: Fri, 30 Apr 2021 20:31:42 +0000	[thread overview]
Message-ID: <20210430203136.lnzqt7vuninh5v4t@revolver> (raw)
In-Reply-To: <m1y2cztuiw.fsf@fess.ebiederm.org>

* Eric W. Biederman <ebiederm@xmission.com> [210430 15:57]:
> Liam Howlett <liam.howlett@oracle.com> writes:
> 
> > This is way out of scope for what I'm doing.  I'm trying to fix a call
> > to the wrong mm API.  I was trying to clean up any obvious errors in
> > calling functions which were exposed by fixing that error.  If you want
> > this fixed differently, then please go ahead and tackle the problems you
> > see.
> 
> I was asked by the arm maintainers to describe what the code should be
> doing here.  I hope I have done that.
> 
> What is very interesting is that the code in __do_page_fault does not
> use find_vma_intersection it uses find_vma.  Which suggests that
> find_vma_intersection may not be the proper mm api.
> 
> The logic is:
> 
> From __do_page_fault:
> 	struct vm_area_struct *vma = find_vma(mm, addr);
> 
> 	if (unlikely(!vma))
> 		return VM_FAULT_BADMAP;
> 
> 	/*
> 	 * Ok, we have a good vm_area for this memory access, so we can handle
> 	 * it.
> 	 */
> 	if (unlikely(vma->vm_start > addr)) {
> 		if (!(vma->vm_flags & VM_GROWSDOWN))
> 			return VM_FAULT_BADMAP;
> 		if (expand_stack(vma, addr))
> 			return VM_FAULT_BADMAP;
> 	}
> 
> 	/*
> 	 * Check that the permissions on the VMA allow for the fault which
> 	 * occurred.
> 	 */
> 	if (!(vma->vm_flags & vm_flags))
> 		return VM_FAULT_BADACCESS;
> 
> From do_page_fault:
> 
> 	arm64_force_sig_fault(SIGSEGV,
> 			      fault == VM_FAULT_BADACCESS ? SEGV_ACCERR : SEGV_MAPERR,
> 			      far, inf->name);
> 
> 
> Hmm.  If the expand_stack step is skipped. Does is the logic equivalent
> to find_vma_intersection?
> 
> 	static inline struct vm_area_struct *find_vma_intersection(
>         	struct mm_struct * mm,
>                 unsigned long start_addr,
>                 unsigned long end_addr)
> 	{
> 		struct vm_area_struct * vma = find_vma(mm,start_addr);
> 	
> 		if (vma && end_addr <= vma->vm_start)
> 			vma = NULL;
> 		return vma;
> 	}
> 
> Yes. It does look that way.  VM_FAULT_BADMAP is returned when a vma
> covering the specified address is not found.  And VM_FAULT_BADACCESS is
> returned when there is a vma and there is a permission problem.
> 
> There are also two SIGBUS cases that arm64_notify_segfault does not
> handle.
> 
> So it appears changing arm64_notify_segfault to use
> find_vma_intersection instead of find_vma would be a correct but
> incomplete fix.

The reason VM_GROWSDOWN is checked here is to see if the stack should be
attempted to be expanded, which happens above.  If VM_GROWSDOWN is true,
then wouldn't the do_page_fault() and __do_page_fault() calls already be
done and be handling this case?

> 
> I don't see a point in changing sigerturn or rt_sigreturn.

Both functions call arm64_notify_segfault() on !access_ok() which I've
increased the potential for returning SEGV_MAPERR.  It is also not
necessary to walk the VMAs when !access_ok(), so it seemed a decent
thing to do.

Thanks,
Liam