From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com ([217.140.101.70]:46206 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755878AbdGKO6s (ORCPT ); Tue, 11 Jul 2017 10:58:48 -0400 Date: Tue, 11 Jul 2017 15:58:49 +0100 From: Will Deacon To: Mark Rutland Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, catalin.marinas@arm.com, james.morse@arm.com, labbott@redhat.com, linux@armlinux.org.uk, stable@vger.kernel.org, steve.capper@arm.com, viro@zeniv.linux.org.uk, peterz@infradead.org, luto@amacapital.net Subject: Re: [PATCH 1/2] arm64: mm: abort uaccess retries upon fatal signal Message-ID: <20170711145849.GE13977@arm.com> References: <1499782763-31418-1-git-send-email-mark.rutland@arm.com> <1499782763-31418-2-git-send-email-mark.rutland@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1499782763-31418-2-git-send-email-mark.rutland@arm.com> Sender: stable-owner@vger.kernel.org List-ID: On Tue, Jul 11, 2017 at 03:19:22PM +0100, Mark Rutland wrote: > When there's a fatal signal pending, arm64's do_page_fault() > implementation returns 0. The intent is that we'll return to the > faulting userspace instruction, delivering the signal on the way. > > However, if we take a fatal signal during fixing up a uaccess, this > results in a return to the faulting kernel instruction, which will be > instantly retried, resulting in the same fault being taken forever. As > the task never reaches userspace, the signal is not delivered, and the > task is left unkillable. While the task is stuck in this state, it can > inhibit the forward progress of the system. > > To avoid this, we must ensure that when a fatal signal is pending, we > apply any necessary fixup for a faulting kernel instruction. Thus we > will return to an error path, and it is up to that code to make forward > progress towards delivering the fatal signal. > > Signed-off-by: Mark Rutland > Reviewed-by: Steve Capper > Tested-by: Steve Capper > Cc: Catalin Marinas > Cc: James Morse > Cc: Laura Abbott > Cc: Will Deacon > Cc: stable@vger.kernel.org > --- > arch/arm64/mm/fault.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index 37b95df..3952d5e 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -397,8 +397,11 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, > * signal first. We do not need to release the mmap_sem because it > * would already be released in __lock_page_or_retry in mm/filemap.c. > */ > - if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current)) > + if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current)) { > + if (!user_mode(regs)) > + goto no_context; > return 0; > + } This will need rebasing at -rc1 (take a look at current HEAD). Also, I think it introduces a weird corner case where we take a page fault when writing the signal frame to the user stack to deliver a SIGSEGV. If we end up with VM_FAULT_RETRY and somebody has sent a SIGKILL to the task, then we'll fail setup_sigframe and force an un-handleable SIGSEGV instead of SIGKILL. The end result (task is killed) is the same, but the fatal signal is wrong. Will