Re: [PATCH v9 29/29] arm64: mte: Add Memory Tagging Extension documentation

[Peter Collingbourne <pcc@google.com>]

On Fri, Sep 18, 2020 at 1:30 AM Will Deacon <will@kernel.org> wrote:
>
> On Thu, Sep 17, 2020 at 05:15:53PM +0100, Dave Martin wrote:
> > On Thu, Sep 17, 2020 at 10:02:30AM +0100, Catalin Marinas wrote:
> > > On Thu, Sep 17, 2020 at 09:11:08AM +0100, Will Deacon wrote:
> > > > On Fri, Sep 04, 2020 at 11:30:29AM +0100, Catalin Marinas wrote:
> > > > > From: Vincenzo Frascino <vincenzo.frascino@arm.com>
> > > > >
> > > > > Memory Tagging Extension (part of the ARMv8.5 Extensions) provides
> > > > > a mechanism to detect the sources of memory related errors which
> > > > > may be vulnerable to exploitation, including bounds violations,
> > > > > use-after-free, use-after-return, use-out-of-scope and use before
> > > > > initialization errors.
> > > > >
> > > > > Add Memory Tagging Extension documentation for the arm64 linux
> > > > > kernel support.
> > > > >
> > > > > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> > > > > Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Acked-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
> > > >
> > > > I'm taking this to mean that Szabolcs is happy with the proposed ABI --
> > > > please shout if that's not the case!
> > >
> > > I think Szabolcs is still on holiday. To summarise the past threads,
> > > AFAICT he's happy with this per-thread control ABI but the discussion
> > > went on whether to expand it in the future (with a new bit) to
> > > synchronise the tag checking mode across all threads of a process. This
> > > adds some complications for the kernel as it needs an IPI to the other
> > > CPUs to set SCTLR_EL1 and it's also racy with multiple threads
> > > requesting different modes.
> > >
> > > Now, in the glibc land, if the tag check mode is controlled via
> > > environment variables, the dynamic loader can set this at process start
> > > while still in single-threaded mode and not touch it at run-time. The
> > > MTE checking can still be enabled at run-time, per mapped memory range
> > > via the PROT_MTE flag. This approach doesn't require any additional
> > > changes to the current patches. But it's for Szabolcs to confirm once
> > > he's back.
> > >
> > > > Wasn't there a man page kicking around too? Would be good to see that
> > > > go upstream (to the manpages project, of course).
> > >
> > > Dave started writing one for the tagged address ABI, not sure where that
> > > is. For the MTE additions, we are waiting for the ABI to be upstreamed.
> >
> > The tagged address ABI control stuff is upstream in the man-pages-5.08
> > release.
> >
> > I don't think anyone drafted anything for MTE yet.  Do we consider the
> > MTE ABI to be sufficiently stable now for it to be worth starting
> > drafting something?
>
> I think so, yes. I'm hoping to queue it for 5.10, once I have an Ack from
> the Android tools side on the per-thread ABI.

Our main requirement on the Android side is to provide an API for
changing the tag checking mode in all threads in a process while
multiple threads are running. I think we've been able to accomplish
this [1] by using a libc private real-time signal which is sent to all
threads. The implementation has been tested on FVP via the included
unit tests. The code has also been tested on real hardware in a
multi-threaded app process (of course we don't have MTE-enabled
hardware, so the implementation was tested on hardware by hacking it
to disable the tagged address ABI instead of changing the tag checking
mode, and then verifying via ptrace(PTRACE_GETREGSET) that the tagged
address ABI was disabled in all threads).

That being said, as with any code at the nexus of concurrency and
POSIX signals, the implementation is quite tricky so I would say it
falls more into the category of "no obvious problems" than "obviously
no problems". It also relies on changes to the implementations of
pthread APIs so it wouldn't catch threads created directly via clone()
rather than via pthread_create(). I think we would be able to ignore
such threads on Android without causing compatibility issues because
we can require the process to not create threads via clone() before
calling the function. I imagine this may not necessarily work for
other libcs like glibc, though, but as I understand it glibc has no
plan to offer such an API.

I feel confident enough in the kernel API though that I think that
it's reasonable as a starting point at least, and that if a problem
with the API is discovered I would expect it to be fixable by adding
new APIs, so:

Acked-by: Peter Collingbourne <pcc@google.com>

Peter

[1] https://android-review.googlesource.com/c/platform/bionic/+/1427377