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

[Kamil Rytarowski]

On 09.10.2018 16:12, Peter Maydell wrote:
> On 9 October 2018 at 14:52, Kamil Rytarowski <address@hidden> 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 <address@hidden>
>>>
>>>
>>> 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.
> 
> The other nice way to handle that is to have osdep.h do
> #ifndef MAP_STACK
> #define MAP_STACK 0
> #endif
> and then you can just unconditionally use MAP_STACK in your
> expression for the mmap flags.
> 

I assume that this is a cleaner solution.

> 
> I note that Linux also defines a MAP_STACK, about which the
> manpage says:
>        MAP_STACK (since Linux 2.6.27)
>               Allocate the mapping at an address suitable for a
> process or thread  stack.
>               This  flag  is currently a no-op, but is used in the
> glibc threading imple‐
>               mentation so that if some architectures require special
> treatment for stack
>               allocations, support can later be transparently
> implemented for glibc.
> 
> So this patch would be opting Linux QEMU builds into whatever that
> potential future behaviour change is. That sounds I guess like
> it's more likely to be the right thing than the wrong thing,
> but it would be useful if some Linux expert could confirm...
> 
> thanks
> -- PMM
> 

There is a similar description on NetBSD:

MAP_STACK

Allocate a memory segment that can be used
either for a process or thread stack.  This
currently has no effect, but its use is
reserved for architectures that might require
special treatment of that address space.
Unimplemented.

Apparently OpenBSD started to overload it for some hardening.

signature.asc
Description: OpenPGP digital signature