Re: [PATCH] mm: Avoid creating virtual address aliases in brk()/mmap()/mremap()

[Will Deacon <will@kernel.org>]

On Tue, Feb 18, 2020 at 01:05:14PM -0800, Evgenii Stepanov wrote:
> On Tue, Feb 18, 2020 at 5:07 AM Andrey Konovalov <andreyknvl@google.com> wrote:
> >
> > On Tue, Feb 18, 2020 at 1:34 PM Will Deacon <will@kernel.org> wrote:
> > >
> > > On Tue, Feb 18, 2020 at 12:23:10PM +0000, Catalin Marinas wrote:
> > > > Currently the arm64 kernel ignores the top address byte passed to brk(),
> > > > mmap() and mremap(). When the user is not aware of the 56-bit address
> > > > limit or relies on the kernel to return an error, untagging such
> > > > pointers has the potential to create address aliases in user-space.
> > > > Passing a tagged address to munmap(), madvise() is permitted since the
> > > > tagged pointer is expected to be inside an existing mapping.
> > >
> > > Might be worth mentioning that this is causing real issues for existing
> > > userspace:
> > >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1797052
> > >
> > > and so should be merged as a fix.
> > >
> > > > Remove untagging in the above functions by partially reverting commit
> > > > ce18d171cb73 ("mm: untag user pointers in mmap/munmap/mremap/brk"). In
> > > > addition, update the arm64 tagged-address-abi.rst document accordingly.
> >
> > Evgenii, do you know if this will cause any issues for HWASAN?
> 
> Is it possible to preserve the untagging behavior when a process has
> opted in TBI?

It's /possible/, but without a concrete need I'm not keen to special case
mmap() like this. "Avoid aliasing user addresses" is an enforceable rule
across all system calls and is easily documented as such, so I'd prefer
to start from that position and only add exceptions where they are really
needed. That clearly doesn't preclude adding them later on with an extension
to the current prctl() controls.

> I have not seen an actual issue with a tagged pointer in mmap yet
> (I've seen two with mprotect, but not mmap or sbrk), so we should be
> fine either way.

Right, and we're leaving mprotect() and madvise() as they were.

Will