Re: [PATCH v19] arm64: expose FAR_EL1 tag bits in siginfo

Peter Collingbourne [off-list ref] writes:

Ok.  Seeing that this code compiles out I don't have any concerns about
it's impact on other architectures.  And I like having it always
present as that makes all of the concerns the code has to deal with
easier to discover.

There is one small issue.  The test should be:
	if (!(ksig->ka.sa.sa_flags & SA_EXPOSE_TAGBITS))
		hide_si_addr_tag_bits(ksig);

Outside of sighand->siglock it is racy to access sighand->action and we
already have an atomic snapshot of the values for exactly this reason.

Is there some reason you didn't use ksig->ka?  Am I missing something?

I agree that our consumption of SA bits is slow enough that my other
concerns are a non-issue.

Eric

On Thu, Nov 19, 2020 at 5:10 AM Eric W. Biederman [off-list ref] wrote:

Peter Collingbourne [off-list ref] writes:

quoted

diff --git a/kernel/signal.c b/kernel/signal.c
index 8f34819e80de..678cdeb235ae 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c

@@ -2524,6 +2524,26 @@ static int ptrace_signal(int signr, kernel_siginfo_t *info)
      return signr;
 }

+static void hide_si_addr_tag_bits(struct ksignal *ksig)
+{
+     switch (siginfo_layout(ksig->sig, ksig->info.si_code)) {
+     case SIL_FAULT:
+     case SIL_FAULT_MCEERR:
+     case SIL_FAULT_BNDERR:
+     case SIL_FAULT_PKUERR:
+             ksig->info.si_addr = arch_untagged_si_addr(
+                     ksig->info.si_addr, ksig->sig, ksig->info.si_code);
+             break;
+     case SIL_KILL:
+     case SIL_TIMER:
+     case SIL_POLL:
+     case SIL_CHLD:
+     case SIL_RT:
+     case SIL_SYS:
+             break;
+     }
+}
+
 bool get_signal(struct ksignal *ksig)
 {
      struct sighand_struct *sighand = current->sighand;

@@ -2761,6 +2781,10 @@ bool get_signal(struct ksignal *ksig)
      spin_unlock_irq(&sighand->siglock);

      ksig->sig = signr;
+
+     if (!(sighand->action[signr - 1].sa.sa_flags & SA_EXPOSE_TAGBITS))
+             hide_si_addr_tag_bits(ksig);
+
      return ksig->sig > 0;
 }

Ok.  Seeing that this code compiles out I don't have any concerns about
it's impact on other architectures.  And I like having it always
present as that makes all of the concerns the code has to deal with
easier to discover.

Ack.

There is one small issue.  The test should be:
        if (!(ksig->ka.sa.sa_flags & SA_EXPOSE_TAGBITS))
                hide_si_addr_tag_bits(ksig);

Outside of sighand->siglock it is racy to access sighand->action and we
already have an atomic snapshot of the values for exactly this reason.

Is there some reason you didn't use ksig->ka?  Am I missing something?

No, I missed that we were copying the sigaction for this reason. I've
changed the code to use the copy as suggested in v20.

I agree that our consumption of SA bits is slow enough that my other
concerns are a non-issue.

Ack.

Peter