On Tue, Aug 25, 2020 at 03:06:39PM -0700, Peter Collingbourne wrote:

On Tue, Aug 25, 2020 at 8:02 AM Dave Martin [off-list ref] wrote:

quoted

On Mon, Aug 24, 2020 at 07:18:19PM -0700, Peter Collingbourne wrote:

quoted

On Mon, Aug 24, 2020 at 7:23 AM Dave Martin [off-list ref] wrote:

quoted

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

quoted

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

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

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.

I'm reasonably sure that the tag bits are available for non-MTE
faults. From https://developer.arm.com/docs/ddi0595/h/aarch64-system-registers/far_el1
:
"For a Data Abort or Watchpoint exception, if address tagging is
enabled for the address accessed by the data access that caused the
exception, then this field includes the tag."

Right, but I wonder whether it would still be good idea to have a way to
tell userspace which bits are valid.

I'm a bit confused by this. si_addr_ignored_bits_mask is exactly the
mechanism for telling userspace which bits are valid. Or maybe you're
arguing that we should consider *not* having the mask of valid bits in
siginfo?

quoted

Collecting and synchronising all the correct information for reporting a
fault is notoriously easy to mess up in the implementation, and
misreporting of the tag bits might be regarded as a tolerable fail.

It really depends. Imagine that a future change to the architecture
exposes bits 60-63 in FAR_EL1 in tag fault errors (we have a number of
ideas for how to use these bits to distinguish between different
use-after-frees in error reports). It would be nice for userspace to
be able to distinguish between the situation where bits 60-63 are 0
and the situation where the bits are unknown, in order to avoid
producing an incorrect/misleading report.

quoted

We also don't get tag bits for prefetch aborts (which may be reported
via SIGSEGV).  Arguably the architecture doesn't allow a nonzero tag
(BR etc. likely just throw those bits on the floor).  But it might be
nice to be explicit about this.

If we view the PC as being a 64-bit value where the architecture does
not allow setting bits 56-63, I think it would be correct to claim
that addresses derived from the PC have bits 56-63 clear.

quoted

Other architectures may also have other reasons why the additional bits
are sometimes available, sometimes not.

If this is the case for an architecture, it can always report the bits
to be unavailable until it can figure out in which cases the bits are
available.

quoted

quoted

This language applies to non-tag-check-fault data aborts but is
superseded by the following paragraph for tag check faults:
"For a synchronous Tag Check Fault abort, bits[63:60] are UNKNOWN."

Right, so in this case we should squash those bits and not report them
in the mask.  Currently are you implying that these are address bits,
because you exclude them from si_addr_ignored_mask?

My intent was that these are implied to be unavailable bits, as they
are not set in the architecturally-defined si_addr mask ~(0xff << 56)
nor in si_addr_ignored_mask.

OK, I think part of my confusion here is coming from the "ignored_bits"
naming.

These really aren't ignored, but rather they are meaningful -- that's
why you're implementing this extension.  True, they're ignored for
addressing purposes (i.e., these bits can never distinguish a memory
location from a second, distinct, memory location).  So for backwards
compatibility we mask them out from si_addr.

In the interests of moving on to reviewing the actual code and avoiding
the discussion from getting too fragmented, can I suggest that you
don't reply in detail to this: I'll reflect, and then reiterate my
comments on the v10/v11 thread if I still have concerns.  I may not get
to it this week -- apologies for that -- but if I can start looking at
the updated series today I will.

Cheers
---Dave

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