Re: [PATCH v3] arm64: Expose original FAR_EL1 value in sigcontext

From: Dave Martin <Dave.Martin@arm.com>
To: Peter Collingbourne <pcc@google.com>
Cc: Andrey Konovalov <andreyknvl@google.com>,
	Kevin Brodsky <kevin.brodsky@arm.com>,
	Kostya Serebryany <kcc@google.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Will Deacon <will@kernel.org>,
	Evgenii Stepanov <eugenis@google.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH v3] arm64: Expose original FAR_EL1 value in sigcontext
Date: Mon, 4 May 2020 11:19:30 +0100	[thread overview]
Message-ID: <20200504101930.GG30377@arm.com> (raw)
In-Reply-To: <20200327191915.257116-1-pcc@google.com>

On Fri, Mar 27, 2020 at 12:19:15PM -0700, Peter Collingbourne wrote:
> 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 <pcc@google.com>
> ---
> 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(-)

[...]

> 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.

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