On Wed, Aug 19, 2020 at 06:49:01PM -0700, Peter Collingbourne wrote:

On Wed, Aug 19, 2020 at 8:56 AM Dave Martin [off-list ref] wrote:

quoted

On Mon, Aug 17, 2020 at 08:33:51PM -0700, Peter Collingbourne wrote:

quoted

The kernel currently clears the tag bits (i.e. bits 56-63) in the fault
address exposed via siginfo.si_addr and sigcontext.fault_address. However,
the tag bits may be needed by tools in order to accurately diagnose
memory errors, such as HWASan [1] or future tools based on the Memory
Tagging Extension (MTE).

We should not stop clearing these bits in the existing fault address
fields, because there may be existing userspace applications that are
expecting the tag bits to be cleared. Instead, create a new pair of
union fields in siginfo._sigfault, and store the tag bits of FAR_EL1
there, together with a mask specifying which bits are valid.

A flag is added to si_xflags to allow userspace to determine whether
the values in the fields are valid.

[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html

Signed-off-by: Peter Collingbourne <redacted>
---

[...]

quoted

quoted

diff --git a/kernel/signal.c b/kernel/signal.c
index 72182eed1b8d..1f1e42adc57d 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c

[...]

quoted

quoted

@@ -1706,7 +1722,9 @@ int send_sig_fault(int sig, int code, void __user *addr
      return send_sig_info(info.si_signo, &info, t);
 }

-int force_sig_mceerr(int code, void __user *addr, short lsb)
+int force_sig_mceerr(int code, void __user *addr, short lsb,
+                  unsigned long addr_ignored_bits,
+                  unsigned long addr_ignored_bits_mask)

Rather than having to pass these extra arguments all over the place, can
we just pass the far for addr, and have an arch-specific hook for
splitting the ignored from non-ignored bits.  For now at least, I expect
that ignored bits mask to be constant for a given platform and kernel.

That sounds like a good idea. I think that for MTE we will want to
make it conditional on the si_code (so SEGV_MTESERR would get 0xf <<
56 while everything else would get 0xff << 56) so I can hook that up
at the same time.

OK, I think that's reasonable.

Mind you, we seem to have 3 kinds of bits here, so I'm starting to
wonder whether this is really sufficient:

	1) address bits used in the faulted access
	2) attribute/permission bits used in the faulted access
	3) unavailable bits.

si_addr contains (1).

si_addr_ignored_bits contains (2).

The tag bits from non-MTE faults seem to fall under case (3).  Code that
looks for these bits in si_addr_ignored_bits won't find them.


So perhaps it would make sense to amend the design a bit.  We'd have

	si_addr = address bits only
	si_attr = attribute bits only
	si_attr_mask = mask of valid bits in si_addr

Thinking about it, I've just shortened/changed some named and changed
the meaning of the mask, so it's very similar to what you already have.

The mask of valid bits in si_addr is decided by architectural
convention (i.e., ~0xffUL << 56), but we could also expose

	si_addr_mask = mask of valid bits in si_addr

This is a bit redundant though.  I think people would already assume
that all bits required for classifying the faulting location accurately
must already be provided there.

quoted

quoted

 {
      struct kernel_siginfo info;

@@ -1717,7 +1735,9 @@ int force_sig_mceerr(int code, void __user *addr, short lsb)
      info.si_code = code;
      info.si_addr = addr;
      info.si_addr_lsb = lsb;
-     info.si_xflags = 0;
+     info.si_xflags = SI_XFLAG_IGNORED_BITS;

Maybe drop the first _, so that this becomes

        SIXFLAG_IGNORED_BITS

?  This may help to avoid anyone thinking this might be an si_code
value.  Maybe ..._IGNORED_ADDR_BITS would also help make it clearer what
this bit is referring to.

Yes, it should have been spelled the same way as the field name (i.e.
SIXFLAG_ADDR_IGNORED_BITS), that was my mistake. I don't have a strong
opinion on whether to keep the underscore in the middle or not.

Fair enough (modulo possible changes above).

[...]

quoted

quoted

      case SIL_FAULT_BNDERR:
              to->si_addr = compat_ptr(from->si_addr);

@@ -3399,6 +3431,8 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to,
              to->si_lower = compat_ptr(from->si_lower);
              to->si_upper = compat_ptr(from->si_upper);
              to->si_xflags = from->si_xflags;
+             to->si_addr_ignored_bits = from->si_addr_ignored_bits;
+             to->si_addr_ignored_bits_mask = from->si_addr_ignored_bits_mask;
              break;
      case SIL_FAULT_PKUERR:
              to->si_addr = compat_ptr(from->si_addr);

@@ -3407,6 +3441,8 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to,
 #endif
              to->si_pkey = from->si_pkey;
              to->si_xflags = from->si_xflags;
+             to->si_addr_ignored_bits = from->si_addr_ignored_bits;
+             to->si_addr_ignored_bits_mask = from->si_addr_ignored_bits_mask;

Again, can you justify why this is exhaustive?  If there any way to
factor this so as to reduce the number of places we need to hack this
in?

Addressed on the other patch. Once I've factored the various
SIL_FAULT* cases there should be only a handful of places requiring
changes.

And that looked like a reasonable justification.

Cheers
---Dave

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