On Wed, May 20, 2020 at 2:26 AM Dave Martin [off-list ref] wrote:

On Wed, May 20, 2020 at 09:55:03AM +0100, Will Deacon wrote:

quoted

On Tue, May 19, 2020 at 03:00:12PM -0700, Peter Collingbourne wrote:

quoted

On Mon, May 18, 2020 at 2:53 AM Dave Martin [off-list ref] wrote:

quoted

On Thu, May 14, 2020 at 05:58:21PM -0700, Peter Collingbourne wrote:

quoted

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.

quoted

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