On Thu, May 07, 2020 at 10:55:02AM -0700, Peter Collingbourne wrote:

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

quoted

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.

Apologies for the delay in responding -- I think it does make sense to
reserve space in __reserved[] for the new record, the the location you
suggested for it is sensible.

__reserved[] is a scarce resource, and should only be burned on "small"
records, but far_context is small.


here's another reason too, which is that we don't want to needlessly
block new software from using this field without allocating larger
stacks -- not least because they just won't, and the problem won't
bite them until much later.


Hope that helps clarify things.

Cheers
---Dave

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