On Mon, May 4, 2020 at 3:19 AM Dave Martin [off-list ref] wrote:

On Fri, Mar 27, 2020 at 12:19:15PM -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 far_context in
sigcontext (similar to the existing esr_context), and store the original
value of FAR_EL1 (including the tag bits) there.

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

Signed-off-by: Peter Collingbourne <redacted>
---
v3:
- add documentation to tagged-pointers.rst
- update comments in sigcontext.h

v2:
- revert changes to hw_breakpoint.c
- rename set_thread_esr to set_thread_far_esr

 Documentation/arm64/tagged-pointers.rst  | 17 +++++----
 arch/arm64/include/asm/exception.h       |  2 +-
 arch/arm64/include/asm/processor.h       |  2 +-
 arch/arm64/include/uapi/asm/sigcontext.h | 21 +++++++----
 arch/arm64/kernel/entry-common.c         |  2 --
 arch/arm64/kernel/signal.c               | 20 ++++++++++-
 arch/arm64/mm/fault.c                    | 45 ++++++++++++++----------
 7 files changed, 74 insertions(+), 35 deletions(-)

[...]

quoted

diff --git a/arch/arm64/include/uapi/asm/sigcontext.h b/arch/arm64/include/uapi/asm/sigcontext.h
index 8b0ebce92427..6782394633cb 100644
--- a/arch/arm64/include/uapi/asm/sigcontext.h
+++ b/arch/arm64/include/uapi/asm/sigcontext.h

@@ -44,11 +44,12 @@ struct sigcontext {
  *
  *   0x210           fpsimd_context
  *    0x10           esr_context
+ *    0x10           far_context
  *   0x8a0           sve_context (vl <= 64) (optional)
  *    0x20           extra_context (optional)
  *    0x10           terminator (null _aarch64_ctx)
  *
- *   0x510           (reserved for future allocation)
+ *   0x500           (reserved for future allocation)
  *
  * New records that can exceed this space need to be opt-in for userspace, so
  * that an expanded signal frame is not generated unexpectedly.  The mechanism

@@ -94,17 +95,25 @@ struct esr_context {
      __u64 esr;
 };

+/* FAR_EL1 context */
+#define FAR_MAGIC    0x46415201
+
+struct far_context {
+     struct _aarch64_ctx head;
+     __u64 far;
+};
+
 /*
  * extra_context: describes extra space in the signal frame for
  * additional structures that don't fit in sigcontext.__reserved[].
  *
  * Note:
  *
- * 1) fpsimd_context, esr_context and extra_context must be placed in
- * sigcontext.__reserved[] if present.  They cannot be placed in the
- * extra space.  Any other record can be placed either in the extra
- * space or in sigcontext.__reserved[], unless otherwise specified in
- * this file.
+ * 1) fpsimd_context, esr_context, far_context and extra_context must be
+ * placed in sigcontext.__reserved[] if present.  They cannot be placed
+ * in the extra space.  Any other record can be placed either in the
+ * extra space or in sigcontext.__reserved[], unless otherwise specified
+ * in this file.

This is for backwards compatibility only.  We don't need this constraint
for any new field, so you can probably leave the paragraph as-is.

Removing this would mean constraint would mean that userspace must be
prepared to traverse extra_context when looking for far_context.  But
really we want modern userspace to do this anyway, since it reduces
backwards compatibilty worries when adding more new records in the
future.

My original reason for updating this comment was that I figured that
this record was small enough that we could just always include it in
__reserved.

But thinking about this a bit more, it doesn't seem that just wanting
userspace to read extra_context will guarantee that it will do so. In
practice, it would be easy to write userspace code that works right
now but doesn't read extra_context correctly (either because
extra_context wasn't considered at all, or because the code purporting
to read the record from extra_context contains a latent bug because it
wasn't exercised). Since we may be practically constrained from moving
the record anyway, we might as well document it and allow the
userspace code to be a little simpler.

I guess one alternative is that we always place this record in
extra_context, which would force userspace to read it correctly. That
has something of the opposite problem (userspace code could be written
to only expect the record in extra_context), but at least we're less
constrained there, and it's more likely that the code would be parsing
__reserved correctly since it would need to do so in order to find
extra_context.

Anyway, I've reverted the comment change for now in v4, but let me
know what you think.


Peter

The nasty loop in parse_user_sigframe() allows some flexibility
regarding the order of records, but prior to this patch there is no
record that can be actually be moved, due to other backwards
compatibility constraints -- so the flexibility isn't used today.  I'd
like to avoid reorderability creeping in, so that we can get rid of the
loop.

So, mandating that records must be in a consistent order to sigcontext.h
could be helpful.  inserting new records in the middle should be fine,
so long as there is no shuffling.

I'm not sure this patch needs to do anything extra for that: perhaps we
can leave this no-shuffling rule implicit for now (?)

People already get shouted at for needslessly noisy diffs, so there is a
strong disincentive to shuffle existing headers in any case...

[...]

Cheers
---Dave

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