From: Dave Martin <Dave.Martin@arm.com>
To: Peter Collingbourne <pcc@google.com>
Cc: Parisc List <linux-parisc@vger.kernel.org>,
Catalin Marinas <catalin.marinas@arm.com>,
Kevin Brodsky <kevin.brodsky@arm.com>,
Oleg Nesterov <oleg@redhat.com>,
Evgenii Stepanov <eugenis@google.com>,
"James E.J. Bottomley" <James.Bottomley@hansenpartnership.com>,
Kostya Serebryany <kcc@google.com>,
"Eric W. Biederman" <ebiederm@xmission.com>,
Andrey Konovalov <andreyknvl@google.com>,
David Spickett <david.spickett@linaro.org>,
Vincenzo Frascino <vincenzo.frascino@arm.com>,
Will Deacon <will@kernel.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH v9 6/6] arm64: expose FAR_EL1 tag bits in siginfo
Date: Mon, 24 Aug 2020 15:23:49 +0100 [thread overview]
Message-ID: <20200824142348.GN6642@arm.com> (raw)
In-Reply-To: <CAMn1gO5pFUGDLJEWe1uOetz0ohE1-pdWGvz7_XxOMZROOfO=ag@mail.gmail.com>
On Wed, Aug 19, 2020 at 06:49:01PM -0700, Peter Collingbourne wrote:
> On Wed, Aug 19, 2020 at 8:56 AM Dave Martin <Dave.Martin@arm.com> wrote:
> >
> > On Mon, Aug 17, 2020 at 08:33:51PM -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 new pair of
> > > union fields in siginfo._sigfault, and store the tag bits of FAR_EL1
> > > there, together with a mask specifying which bits are valid.
> > >
> > > A flag is added to si_xflags to allow userspace to determine whether
> > > the values in the fields are valid.
> > >
> > > [1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
> > >
> > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > ---
[...]
> > > diff --git a/kernel/signal.c b/kernel/signal.c
> > > index 72182eed1b8d..1f1e42adc57d 100644
> > > --- a/kernel/signal.c
> > > +++ b/kernel/signal.c
[...]
> > > @@ -1706,7 +1722,9 @@ int send_sig_fault(int sig, int code, void __user *addr
> > > return send_sig_info(info.si_signo, &info, t);
> > > }
> > >
> > > -int force_sig_mceerr(int code, void __user *addr, short lsb)
> > > +int force_sig_mceerr(int code, void __user *addr, short lsb,
> > > + unsigned long addr_ignored_bits,
> > > + unsigned long addr_ignored_bits_mask)
> >
> > Rather than having to pass these extra arguments all over the place, can
> > we just pass the far for addr, and have an arch-specific hook for
> > splitting the ignored from non-ignored bits. For now at least, I expect
> > that ignored bits mask to be constant for a given platform and kernel.
>
> That sounds like a good idea. I think that for MTE we will want to
> make it conditional on the si_code (so SEGV_MTESERR would get 0xf <<
> 56 while everything else would get 0xff << 56) so I can hook that up
> at the same time.
OK, I think that's reasonable.
Mind you, we seem to have 3 kinds of bits here, so I'm starting to
wonder whether this is really sufficient:
1) address bits used in the faulted access
2) attribute/permission bits used in the faulted access
3) unavailable bits.
si_addr contains (1).
si_addr_ignored_bits contains (2).
The tag bits from non-MTE faults seem to fall under case (3). Code that
looks for these bits in si_addr_ignored_bits won't find them.
So perhaps it would make sense to amend the design a bit. We'd have
si_addr = address bits only
si_attr = attribute bits only
si_attr_mask = mask of valid bits in si_addr
Thinking about it, I've just shortened/changed some named and changed
the meaning of the mask, so it's very similar to what you already have.
The mask of valid bits in si_addr is decided by architectural
convention (i.e., ~0xffUL << 56), but we could also expose
si_addr_mask = mask of valid bits in si_addr
This is a bit redundant though. I think people would already assume
that all bits required for classifying the faulting location accurately
must already be provided there.
>
> > > {
> > > struct kernel_siginfo info;
> > >
> > > @@ -1717,7 +1735,9 @@ int force_sig_mceerr(int code, void __user *addr, short lsb)
> > > info.si_code = code;
> > > info.si_addr = addr;
> > > info.si_addr_lsb = lsb;
> > > - info.si_xflags = 0;
> > > + info.si_xflags = SI_XFLAG_IGNORED_BITS;
> >
> > Maybe drop the first _, so that this becomes
> >
> > SIXFLAG_IGNORED_BITS
> >
> > ? This may help to avoid anyone thinking this might be an si_code
> > value. Maybe ..._IGNORED_ADDR_BITS would also help make it clearer what
> > this bit is referring to.
>
> Yes, it should have been spelled the same way as the field name (i.e.
> SIXFLAG_ADDR_IGNORED_BITS), that was my mistake. I don't have a strong
> opinion on whether to keep the underscore in the middle or not.
Fair enough (modulo possible changes above).
[...]
> > > case SIL_FAULT_BNDERR:
> > > to->si_addr = compat_ptr(from->si_addr);
> > > @@ -3399,6 +3431,8 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to,
> > > to->si_lower = compat_ptr(from->si_lower);
> > > to->si_upper = compat_ptr(from->si_upper);
> > > to->si_xflags = from->si_xflags;
> > > + to->si_addr_ignored_bits = from->si_addr_ignored_bits;
> > > + to->si_addr_ignored_bits_mask = from->si_addr_ignored_bits_mask;
> > > break;
> > > case SIL_FAULT_PKUERR:
> > > to->si_addr = compat_ptr(from->si_addr);
> > > @@ -3407,6 +3441,8 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to,
> > > #endif
> > > to->si_pkey = from->si_pkey;
> > > to->si_xflags = from->si_xflags;
> > > + to->si_addr_ignored_bits = from->si_addr_ignored_bits;
> > > + to->si_addr_ignored_bits_mask = from->si_addr_ignored_bits_mask;
> >
> > Again, can you justify why this is exhaustive? If there any way to
> > factor this so as to reduce the number of places we need to hack this
> > in?
>
> Addressed on the other patch. Once I've factored the various
> SIL_FAULT* cases there should be only a handful of places requiring
> changes.
And that looked like a reasonable justification.
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-08-24 14:25 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-18 3:33 [PATCH v9 0/6] arm64: expose FAR_EL1 tag bits in siginfo Peter Collingbourne
2020-08-18 3:33 ` [PATCH v9 1/6] parisc: start using signal-defs.h Peter Collingbourne
2020-08-18 3:33 ` [PATCH v9 2/6] arch: move SA_* definitions to generic headers Peter Collingbourne
2020-08-19 7:13 ` Geert Uytterhoeven
2020-08-19 22:44 ` Peter Collingbourne
2020-08-19 10:30 ` Dave Martin
2020-08-19 21:35 ` Peter Collingbourne
2020-08-18 3:33 ` [PATCH v9 3/6] signal: clear non-uapi flag bits when passing/returning sa_flags Peter Collingbourne
2020-08-19 10:39 ` Dave Martin
2020-08-19 23:39 ` Peter Collingbourne
2020-08-24 13:40 ` Dave Martin
2020-08-25 0:51 ` Peter Collingbourne
2020-08-25 14:25 ` Dave Martin
2020-08-18 3:33 ` [PATCH v9 4/6] signal: define the SA_UNSUPPORTED bit in sa_flags Peter Collingbourne
2020-08-19 14:51 ` Dave Martin
2020-08-20 0:23 ` Peter Collingbourne
2020-08-24 13:41 ` Dave Martin
2020-08-18 3:33 ` [PATCH v9 5/6] signal: define the field siginfo.si_xflags Peter Collingbourne
2020-08-19 15:40 ` Dave Martin
2020-08-20 1:37 ` Peter Collingbourne
2020-08-24 14:03 ` Dave Martin
2020-08-25 1:27 ` Peter Collingbourne
2020-08-25 14:47 ` Dave Martin
2020-08-25 20:08 ` Peter Collingbourne
2020-08-26 16:15 ` Dave Martin
2020-08-18 3:33 ` [PATCH v9 6/6] arm64: expose FAR_EL1 tag bits in siginfo Peter Collingbourne
2020-08-19 15:56 ` Dave Martin
2020-08-20 1:49 ` Peter Collingbourne
2020-08-24 14:23 ` Dave Martin [this message]
2020-08-25 2:18 ` Peter Collingbourne
2020-08-25 15:02 ` Dave Martin
2020-08-25 22:06 ` Peter Collingbourne
2020-08-26 15:32 ` Dave Martin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200824142348.GN6642@arm.com \
--to=dave.martin@arm.com \
--cc=James.Bottomley@hansenpartnership.com \
--cc=andreyknvl@google.com \
--cc=catalin.marinas@arm.com \
--cc=david.spickett@linaro.org \
--cc=ebiederm@xmission.com \
--cc=eugenis@google.com \
--cc=kcc@google.com \
--cc=kevin.brodsky@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-parisc@vger.kernel.org \
--cc=oleg@redhat.com \
--cc=pcc@google.com \
--cc=rth@twiddle.net \
--cc=vincenzo.frascino@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).