Re: [PATCH v20] arm64: expose FAR_EL1 tag bits in siginfo

From: ebiederm@xmission.com (Eric W. Biederman)
To: Peter Collingbourne <pcc@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Evgenii Stepanov <eugenis@google.com>,
	Kostya Serebryany <kcc@google.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Dave Martin <Dave.Martin@arm.com>, Will Deacon <will@kernel.org>,
	Oleg Nesterov <oleg@redhat.com>,
	"James E.J. Bottomley" <James.Bottomley@hansenpartnership.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Kevin Brodsky <kevin.brodsky@arm.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Linux API <linux-api@vger.kernel.org>,
	Helge Deller <deller@gmx.de>,
	David Spickett <david.spickett@linaro.org>
Subject: Re: [PATCH v20] arm64: expose FAR_EL1 tag bits in siginfo
Date: Fri, 20 Nov 2020 13:24:07 -0600	[thread overview]
Message-ID: <878sav3k88.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <CAMn1gO6mzzpnYbKfn3gXAwX2tejD-g+2S_ShkyWVCLskFWSqvg@mail.gmail.com> (Peter Collingbourne's message of "Fri, 20 Nov 2020 10:22:47 -0800")

Peter Collingbourne <pcc@google.com> writes:

> On Fri, Nov 20, 2020 at 9:44 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> Peter Collingbourne <pcc@google.com> writes:
>>
>> > 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, introduce a flag in
>> > sigaction.sa_flags, SA_EXPOSE_TAGBITS, and only expose the tag bits
>> > there if the signal handler has this flag set.
>> >
>> > [1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
>>
>> For the generic bits:
>> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> Thanks for the review.
>
>> Some of the arm bits look wrong.  There are a couple of cases where
>> it looks like you are deliberately passing an untagged address into
>> functions that normally take tagged addresses.
>>
>> It might be a good idea to have a type distinction between the two.
>> Perhaps "(void __user *)" vs "(unsigned long)" so that accidentally
>> using the wrong one generates a type error.
>>
>> I don't think I am really qualified to review all of the arm details,
>> and I certainly don't want to be in the middle of any arm bugs this
>> code might introduce.  If you will split out the generic bits of this
>> patch I will take it.  The this whole thing can be merged into the arm
>
> Okay, I'll do that.

Thank you.

>> tree and you can ensure the arm bits are correct.
>
> So you want Catalin to merge your signal-for-v5.11 branch with the
> generic patches into the arm64 tree and then add the arm64-specific
> patch on top? Works for me.

I think that is what we discussed.  Unless he has objections I would
prefer that.  I based the branch on -rc3 in hopes that I would
not cause problems for his process.

In the cases where I was confused you probably want comments describing
why the tag bits are being cleared.  It may be obvious when you know the
architecture intimately but it certainly was not for me.  I certainly
don't know the details of arm64 well enough to understand the
architecture specific nuances.

Eric