Re: [PATCH v6] arm64: Expose FAR_EL1 tag bits in sigcontext

From: Dave Martin <Dave.Martin@arm•com>
To: Will Deacon <will@kernel•org>
Cc: Andrey Konovalov <andreyknvl@google•com>,
	Kevin Brodsky <kevin.brodsky@arm•com>,
	Kostya Serebryany <kcc@google•com>,
	Linux ARM <linux-arm-kernel@lists•infradead.org>,
	Catalin Marinas <catalin.marinas@arm•com>,
	Vincenzo Frascino <vincenzo.frascino@arm•com>,
	Peter Collingbourne <pcc@google•com>,
	Evgenii Stepanov <eugenis@google•com>,
	Richard Henderson <rth@twiddle•net>
Subject: Re: [PATCH v6] arm64: Expose FAR_EL1 tag bits in sigcontext
Date: Wed, 20 May 2020 10:26:00 +0100	[thread overview]
Message-ID: <20200520092558.GI5031@arm.com> (raw)
In-Reply-To: <20200520085502.GC24293@willie-the-truck>

On Wed, May 20, 2020 at 09:55:03AM +0100, Will Deacon wrote:
> On Tue, May 19, 2020 at 03:00:12PM -0700, Peter Collingbourne wrote:
> > On Mon, May 18, 2020 at 2:53 AM Dave Martin <Dave.Martin@arm•com> wrote:
> > > On Thu, May 14, 2020 at 05:58:21PM -0700, Peter Collingbourne wrote:
> > > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> > > > index baa88dc02e5c..5867f2fdbe64 100644
> > > > --- a/arch/arm64/kernel/signal.c
> > > > +++ b/arch/arm64/kernel/signal.c
> > > > @@ -648,6 +648,7 @@ static int setup_sigframe(struct
> > > > rt_sigframe_user_layout *user,
> > > >                 __put_user_error(ESR_MAGIC, &esr_ctx->head.magic, err);
> > > >                 __put_user_error(sizeof(*esr_ctx), &esr_ctx->head.size, err);
> > > >                 __put_user_error(current->thread.fault_code,
> > > > &esr_ctx->esr, err);
> > > > +               current->thread.fault_code = 0;
> > >
> > > Perhaps, but we'd need to be careful.  For example, can we run out of
> > > user stack before this and deliver a SIGSEGV, but with the old
> > > fault_code still set?  Then we'd emit the old fault code with the
> > > new "can't deliver signal" signal, which doesn't make sense.
> > >
> > > Stuff may also go wrong with signal prioritisation.
> > >
> > > If a higher-priority signal (say SIGINT) comes in after a data abort
> > > enters the kernel but before the resulting SIGSEGV is dequeued for
> > > delivery, wouldn't we deliver SIGINT first, with the bogus fault code?
> > > With your change we'd then have cleared the fault code by the time we
> > > deliver the SIGSEGV it actually relates to, if I've understood right.
> > >
> > > Today, I think we just attach that fault code to every signal that's
> > > delivered until something overwrites or resets it, which means that
> > > a signal that needs fault_code gets it, at the expense of attaching
> > > it to a bunch of other random signals too.
> > >
> > >
> > > Checking the signal number and si_code might help us to know what we
> > > should be doing with fault_code.  We need to have sure userspace can't
> > > trick us with a non kernel generated signal here.  It would also be
> > > necessary to check how PTRACE_SETSIGINFO interacts with this.
> > 
> > With these possible interactions in mind I think we should store the
> > fault code and fault address in kernel_siginfo instead of
> > thread_struct (and clear these fields when we receive a siginfo from
> > userspace, i.e. in copy_siginfo_from_user which is used by
> > ptrace(PTRACE_SETSIGINFO) among other places). That way, the
> > information is clearly associated with the signal itself and not the
> > thread, so we don't need to worry about our signal being delivered out
> > of order.
> 
> Hmm, I can't see a way to do that that isn't horribly invasive in the core
> signal code. Can you?
> 
> But generally, I agree: the per-thread handling of fault_address and
> fault_code appears to be quite broken in the face of signal prioritisation
> and signals that don't correspond directly to hardware trap. It would be
> nice to have some tests for this...
> 
> If we want to pile on more bodges, perhaps we could stash the signal number
> to which the fault_{address,code} relate, and then check that at delivery
> and clear on a match. I hate it.

I agree with Daniel's suggestion in principle, but I was also concerned
about whether it would be too invasive elsewhere.

Question though: does the core code take special care to make sure that
a force_sig cannot be outprioritised by a regular signal?  If so,
perhaps we get away with it.  I ask this, because the same same issue
may be hitting other arches otherwise.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel