From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 References: <20200515171612.1020-1-catalin.marinas@arm.com> <20200515171612.1020-12-catalin.marinas@arm.com> <20200528091445.GA2961@gaia> <20200528110509.GA18623@arm.com> <20200528163412.GC2961@gaia> In-Reply-To: <20200528163412.GC2961@gaia> From: Evgenii Stepanov Date: Thu, 28 May 2020 11:35:50 -0700 Message-ID: Subject: Re: [PATCH v4 11/26] arm64: mte: Add PROT_MTE support to mmap() and mprotect() Content-Type: multipart/alternative; boundary="000000000000d9931405a6b99917" Sender: owner-linux-mm@kvack.org To: Catalin Marinas Cc: Szabolcs Nagy , Peter Collingbourne , Linux ARM , Linux Memory Management List , linux-arch , Will Deacon , Dave P Martin , Vincenzo Frascino , Kevin Brodsky , Andrey Konovalov , nd List-ID: --000000000000d9931405a6b99917 Content-Type: text/plain; charset="UTF-8" On Thu, May 28, 2020 at 9:34 AM Catalin Marinas wrote: > On Thu, May 28, 2020 at 12:05:09PM +0100, Szabolcs Nagy wrote: > > The 05/28/2020 10:14, Catalin Marinas wrote: > > > On Wed, May 27, 2020 at 11:57:39AM -0700, Peter Collingbourne wrote: > > > > On Fri, May 15, 2020 at 10:16 AM Catalin Marinas > > > > wrote: > > > > > To enable tagging on a memory range, the user must explicitly opt > in via > > > > > a new PROT_MTE flag passed to mmap() or mprotect(). Since this is > a new > > > > > memory type in the AttrIndx field of a pte, simplify the or'ing of > these > > > > > bits over the protection_map[] attributes by making MT_NORMAL > index 0. > > > > > > > > Should the userspace stack always be mapped as if with PROT_MTE if > the > > > > hardware supports it? Such a change would be invisible to non-MTE > > > > aware userspace since it would already need to opt in to tag checking > > > > via prctl. This would let userspace avoid a complex stack > > > > initialization sequence when running with stack tagging enabled on > the > > > > main thread. > > > > > > I don't think the stack initialisation is that difficult. On program > > > startup (can be the dynamic loader). Something like (untested): > > > > > > register unsigned long stack asm ("sp"); > > > unsigned long page_sz = sysconf(_SC_PAGESIZE); > > > > > > mprotect((void *)(stack & ~(page_sz - 1)), page_sz, > > > PROT_READ | PROT_WRITE | PROT_MTE | PROT_GROWSDOWN); > > > > > > (the essential part it PROT_GROWSDOWN so that you don't have to specify > > > a stack lower limit) > > > > does this work even if the currently mapped stack is more than page_sz? > > determining the mapped main stack area is i think non-trivial to do in > > userspace (requires parsing /proc/self/maps or similar). > > Because of PROT_GROWSDOWN, the kernel adjusts the start of the range > down automatically. It is potentially problematic if the top of the > stack is more than a page away and you want the whole stack coloured. I > haven't run a test but my reading of the kernel code is that the stack > vma would be split in this scenario, so the range beyond sp+page_sz > won't have PROT_MTE set. > > My assumption is that if you do this during program start, the stack is > smaller than a page. Alternatively, could we use argv or envp to > determine the top of the user stack (the bottom is taken care of by the > kernel)? > PROT_GROWSDOWN seems to work fine in our case, and the extra tag maintenance overhead sounds like a valid argument against setting PROT_MTE unconditionally. On the other hand, we may end up doing this in the userspace in every process. The reason is, PROT_MTE can not be set on a page that contains a live frame with stack tagging because of mismatching tags (IRG is not affected by PROT_MTE but STG is). So ideally, this should be done at (or near) the program entry point, while the stack is mostly empty. > > > > I'm fine, however, with enabling PROT_MTE on the main stack based on > > > some ELF note. > > > > note that would likely mean an elf note on the dynamic linker > > (because a dynamic linked executable may not be loaded by the > > kernel and ctors in loaded libs run before the executable entry > > code anyway, so the executable alone cannot be in charge of this > > decision) i.e. one global switch for all dynamic linked binaries. > > I guess parsing such note in the kernel is only useful for static > binaries. > > > i think a dynamic linker can map a new stack and switch to it > > if it needs to control the properties of the stack at runtime > > (it's wasteful though). > > There is already user code to check for HWCAP2_MTE and the prctl(), so > adding an mprotect() doesn't look like a significant overhead. > > > and i think there should be a runtime mechanism for the brk area: > > it should be possible to request that future brk expansions are > > mapped as PROT_MTE so an mte aware malloc implementation can use > > brk. i think this is not important in the initial design, but if > > a prctl flag can do it that may be useful to add (may be at a > > later time). > > Looking at the kernel code briefly, I think this would work. We do end > up with two vmas for the brk, only the expansion having PROT_MTE, and > I'd to find a way to store the extra flag. > > From a coding perspective, it's easier to just set PROT_MTE by default > on both brk and initial stack ;) (VM_DATA_DEFAULT_FLAGS). > > > (and eventually there should be a way to use PROT_MTE on > > writable global data and appropriate code generation that > > takes colors into account when globals are accessed, but > > that requires significant ELF, ld.so and compiler changes, > > that need not be part of the initial mte design). > > The .data section needs to be driven by the ELF information. It's also a > file mapping and we don't support PROT_MTE on them even if MAP_PRIVATE. > There are complications like DAX where the file you mmap for CoW may be > hosted on memory that does not support MTE (copied to RAM on write). > > Is there a use-case for global data to be tagged? > Yes, catching global buffer overflow bugs. They are not nearly as common as heap-based issues though. > > -- > Catalin > --000000000000d9931405a6b99917 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, May 28, 2020 at 9:34 AM Catal= in Marinas <catalin.marinas@a= rm.com> wrote:
On Thu, May 28, 2020 at 12:05:09PM +0100, Szabolcs Nagy wrote:
> The 05/28/2020 10:14, Catalin Marinas wrote:
> > On Wed, May 27, 2020 at 11:57:39AM -0700, Peter Collingbourne wro= te:
> > > On Fri, May 15, 2020 at 10:16 AM Catalin Marinas
> > > <catalin.marinas@arm.com> wrote:
> > > > To enable tagging on a memory range, the user must expl= icitly opt in via
> > > > a new PROT_MTE flag passed to mmap() or mprotect(). Sin= ce this is a new
> > > > memory type in the AttrIndx field of a pte, simplify th= e or'ing of these
> > > > bits over the protection_map[] attributes by making MT_= NORMAL index 0.
> > >
> > > Should the userspace stack always be mapped as if with PROT_= MTE if the
> > > hardware supports it? Such a change would be invisible to no= n-MTE
> > > aware userspace since it would already need to opt in to tag= checking
> > > via prctl. This would let userspace avoid a complex stack > > > initialization sequence when running with stack tagging enab= led on the
> > > main thread.
> >
> > I don't think the stack initialisation is that difficult. On = program
> > startup (can be the dynamic loader). Something like (untested): > >
> >=C2=A0 =C2=A0 =C2=A0register unsigned long stack asm ("sp&quo= t;);
> >=C2=A0 =C2=A0 =C2=A0unsigned long page_sz =3D sysconf(_SC_PAGESIZE= );
> >
> >=C2=A0 =C2=A0 =C2=A0mprotect((void *)(stack & ~(page_sz - 1)),= page_sz,
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 PROT_READ | PROT_= WRITE | PROT_MTE | PROT_GROWSDOWN);
> >
> > (the essential part it PROT_GROWSDOWN so that you don't have = to specify
> > a stack lower limit)
>
> does this work even if the currently mapped stack is more than page_sz= ?
> determining the mapped main stack area is i think non-trivial to do in=
> userspace (requires parsing /proc/self/maps or similar).

Because of PROT_GROWSDOWN, the kernel adjusts the start of the range
down automatically. It is potentially problematic if the top of the
stack is more than a page away and you want the whole stack coloured. I
haven't run a test but my reading of the kernel code is that the stack<= br> vma would be split in this scenario, so the range beyond sp+page_sz
won't have PROT_MTE set.

My assumption is that if you do this during program start, the stack is
smaller than a page. Alternatively, could we use argv or envp to
determine the top of the user stack (the bottom is taken care of by the
kernel)?

PROT_GROWSDOWN seems to work fine in our case, and the extra tag maint= enance overhead sounds like a valid argument against setting PROT_MTE uncon= ditionally.

On the other hand, we may= end up doing this in the userspace in every process. The reason is, PROT_M= TE can not be set on a page that contains a live frame with stack tagging b= ecause of mismatching tags (IRG is not affected by PROT_MTE but STG is). So= ideally, this should be done at (or near) the program entry point, while t= he stack is mostly empty.
=C2=A0

> > I'm fine, however, with enabling PROT_MTE on the main stack b= ased on
> > some ELF note.
>
> note that would likely mean an elf note on the dynamic linker
> (because a dynamic linked executable may not be loaded by the
> kernel and ctors in loaded libs run before the executable entry
> code anyway, so the executable alone cannot be in charge of this
> decision) i.e. one global switch for all dynamic linked binaries.

I guess parsing such note in the kernel is only useful for static
binaries.

> i think a dynamic linker can map a new stack and switch to it
> if it needs to control the properties of the stack at runtime
> (it's wasteful though).

There is already user code to check for HWCAP2_MTE and the prctl(), so
adding an mprotect() doesn't look like a significant overhead.

> and i think there should be a runtime mechanism for the brk area:
> it should be possible to request that future brk expansions are
> mapped as PROT_MTE so an mte aware malloc implementation can use
> brk. i think this is not important in the initial design, but if
> a prctl flag can do it that may be useful to add (may be at a
> later time).

Looking at the kernel code briefly, I think this would work. We do end
up with two vmas for the brk, only the expansion having PROT_MTE, and
I'd to find a way to store the extra flag.

>From a coding perspective, it's easier to just set PROT_MTE by default<= br> on both brk and initial stack ;) (VM_DATA_DEFAULT_FLAGS).

> (and eventually there should be a way to use PROT_MTE on
> writable global data and appropriate code generation that
> takes colors into account when globals are accessed, but
> that requires significant ELF, ld.so and compiler changes,
> that need not be part of the initial mte design).

The .data section needs to be driven by the ELF information. It's also = a
file mapping and we don't support PROT_MTE on them even if MAP_PRIVATE.=
There are complications like DAX where the file you mmap for CoW may be
hosted on memory that does not support MTE (copied to RAM on write).

Is there a use-case for global data to be tagged?

=
Yes, catching global buffer overflow bugs. They are not nearly a= s common as heap-based issues though.
=C2=A0

--
Catalin
--000000000000d9931405a6b99917--