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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210430203136.lnzqt7vuninh5v4t@revolver>
Date:   Fri, 30 Apr 2021 20:31:42 +0000
From:   Liam Howlett <liam.howlett@...cle.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
CC:     Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Julien Grall <julien.grall@....com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>
Subject: Re: [PATCH 2/3] arm64: signal: sigreturn() and rt_sigreturn()
 sometime returns the wrong signals

* Eric W. Biederman <ebiederm@...ssion.com> [210430 15:57]:
> Liam Howlett <liam.howlett@...cle.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