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

[Evgenii Stepanov <eugenis@google.com>]

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?

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.

>
> > >
> > > Fixes: ce18d171cb73 ("mm: untag user pointers in mmap/munmap/mremap/brk")
> > > Cc: <stable@vger.kernel.org> # 5.4.x-
> > > Cc: Andrey Konovalov <andreyknvl@google.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Florian Weimer <fweimer@redhat.com>
> > > Reported-by: Victor Stinner <vstinner@redhat.com>
> > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> > > ---
> > >  Documentation/arm64/tagged-address-abi.rst | 7 +++++--
> > >  mm/mmap.c                                  | 4 ----
> > >  mm/mremap.c                                | 1 -
> > >  3 files changed, 5 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/Documentation/arm64/tagged-address-abi.rst b/Documentation/arm64/tagged-address-abi.rst
> > > index d4a85d535bf9..1771a8b5712e 100644
> > > --- a/Documentation/arm64/tagged-address-abi.rst
> > > +++ b/Documentation/arm64/tagged-address-abi.rst
> > > @@ -44,8 +44,11 @@ The AArch64 Tagged Address ABI has two stages of relaxation depending
> > >  how the user addresses are used by the kernel:
> > >
> > >  1. User addresses not accessed by the kernel but used for address space
> > > -   management (e.g. ``mmap()``, ``mprotect()``, ``madvise()``). The use
> > > -   of valid tagged pointers in this context is always allowed.
> > > +   management (e.g. ``mprotect()``, ``madvise()``). The use of valid
> > > +   tagged pointers in this context is allowed with the exception of
> > > +   ``brk()``, ``mmap()`` and the ``new_address`` argument to
> > > +   ``mremap()`` as these have the potential of aliasing with existing
> > > +   user addresses.
> >
> > Given that we're backporting this to stable kernels, perhaps it's worth
> > a note here along the lines of:
> >
> > NOTE: This behaviour changed in v5.6 and so some earlier kernels may
> > incorrectly accept valid tagged pointers for these system calls.
> >
> > With that:
> >
> > Acked-by: Will Deacon <will@kernel.org>
> >
> > Happy to take this as an arm64 fix for 5.6, unless Andrew would prefer
> > to route it via his tree.
> >
> > Will