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

[Peter Collingbourne <pcc@google.com>]

On Wed, May 20, 2020 at 2:26 AM Dave Martin <Dave.Martin@arm.com> wrote:
>
> 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?

I think I've come up with a way that doesn't seem to be too invasive.
See patch #1 of the series that I'm about to send out.

> > 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.

Not as far as I can tell. There does appear to be prioritisation for
synchronous signals [1] but as far as I can tell nothing to
distinguish one of these signals from one with the same signal number
sent from userspace (e.g. via kill(2)).

Peter

[1] https://github.com/torvalds/linux/blob/b85051e755b0e9d6dd8f17ef1da083851b83287d/kernel/signal.c#L222

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