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

[Brad Smith]

On 10/19/2018 7:55 AM, Peter Maydell wrote:

> On 18 October 2018 at 23:10, Brad Smith <address@hidden> 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 <address@hidden>
> > Reviewed-by: Kamil Rytarowski <address@hidden>
> >
> > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > index fbd0dc8c57..7814e61114 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,15 @@ 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;
> > +#if defined(MAP_STACK) && defined(__OpenBSD__)
> > +    /* Only enable MAP_STACK on OpenBSD. Other OS's such
> > +       as Linux/FreeBSD/NetBSD have a flag with the same
> > +       name but have differing functionality. */
> > +    flags |= MAP_STACK;
> > +#endif
> I think it would be useful to also add "OpenBSD will SEGV
> if it spots execution with a stack pointer pointing at
> memory that was not allocated with MAP_STACK." (summarising
> what the interesting bit of OpenBSD behaviour is here to save
> having to go back and read the commit message in future).
>
> Also our multiline comment format is linux-kernel style
>   /*
>    * line 1
>    * line 2
>    */
> (as noted in CODING_STYLE).
>
> Otherwise
> Reviewed-by: Peter Maydell <address@hidden>
Added the comment and adjusted the comment format.

Thanks.