Re: [Qemu-devel] [PATCH] oslib-posix: Use MAP_STACK in qemu_alloc_stack() on OpenBSD

[Brad Smith <brad@comstyle.com>]

On Tue, Oct 09, 2018 at 03:52:30PM +0200, Kamil Rytarowski wrote:
> On 07.10.2018 17:37, Brad Smith wrote:
> > Use MAP_STACK in qemu_alloc_stack() on OpenBSD.
> > 
> > Added to -current and will be in our soon to be 6.4 release.
> > 
> > MAP_STACK      Indicate that the mapping is used as a stack.  This
> >                flag must be used in combination with MAP_ANON and
> >                MAP_PRIVATE.
> > 
> > Implement MAP_STACK option for mmap().  Synchronous faults (pagefault and
> > syscall) confirm the stack register points at MAP_STACK memory, otherwise
> > SIGSEGV is delivered. sigaltstack() and pthread_attr_setstack() are modified
> > to create a MAP_STACK sub-region which satisfies alignment requirements.
> > Observe that MAP_STACK can only be set/cleared by mmap(), which zeroes the
> > contents of the region -- there is no mprotect() equivalent operation, so
> > there is no MAP_STACK-adding gadget.
> > 
> > 
> > Signed-off-by: Brad Smith <brad@comstyle.com>
> > 
> > 
> > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > index fbd0dc8c57..51e9a012c2 100644
> > --- a/util/oslib-posix.c
> > +++ b/util/oslib-posix.c
> > @@ -611,7 +611,11 @@ void *qemu_alloc_stack(size_t *sz)
> >      *sz += pagesz;
> >  
> >      ptr = mmap(NULL, *sz, PROT_READ | PROT_WRITE,
> > -               MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > +               MAP_PRIVATE | MAP_ANONYMOUS
> > +#ifdef MAP_STACK
> > +               | MAP_STACK
> > +#endif
> > +               , -1, 0);
> >      if (ptr == MAP_FAILED) {
> >          perror("failed to allocate memory for stack");
> >          abort();
> > 
> 
> Can we handle it differently, storing MAP_* flags in a variable:
> 
> int flags = MAP_PRIVATE | MAP_ANONYMOUS;
> #ifdef MAP_STACK
> flags |= MAP_STACK;
> #endif
> 
> ptr = mmap(NULL, *sz, PROT_READ | PROT_WRITE, flags, -1, 0);
> 
> This way it will look nicer as we won't ifdef the middle of a function call.

How about the following?


diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index fbd0dc8c57..f8ee349c9c 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -596,6 +596,7 @@ pid_t qemu_fork(Error **errp)
 void *qemu_alloc_stack(size_t *sz)
 {
     void *ptr, *guardpage;
+    int flags;
 #ifdef CONFIG_DEBUG_STACK_USAGE
     void *ptr2;
 #endif
@@ -610,8 +611,12 @@ void *qemu_alloc_stack(size_t *sz)
     /* allocate one extra page for the guard page */
     *sz += pagesz;
 
-    ptr = mmap(NULL, *sz, PROT_READ | PROT_WRITE,
-               MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    flags = MAP_PRIVATE | MAP_ANONYMOUS;
+#ifdef MAP_STACK
+    flags |= MAP_STACK;
+#endif
+
+    ptr = mmap(NULL, *sz, PROT_READ | PROT_WRITE, flags, -1, 0);
     if (ptr == MAP_FAILED) {
         perror("failed to allocate memory for stack");
         abort();