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

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

On Fri, Nov 20, 2020 at 11:24 AM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> 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.

Sent out v21 with the generic parts split out.

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

I think that makes sense. I've added comments to those two cases.

Peter

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