Re: [PATCH v3 27/28] tcg: When allocating for !splitwx, begin with PROT_NONE

[Richard Henderson]

On 6/9/21 4:21 AM, Alex Bennée wrote:
> Richard Henderson <richard.henderson@linaro.org> writes:
>
> > There's a change in mprotect() behaviour [1] in the latest macOS
> > on M1 and it's not yet clear if it's going to be fixed by Apple.
> >
> > In this case, instead of changing permissions of N guard pages,
> > we change permissions of N rwx regions.  The same number of
> > syscalls are required either way.
> >
> > [1] https://gist.github.com/hikalium/75ae822466ee4da13cbbe486498a191f
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   tcg/region.c | 19 +++++++++----------
> >   1 file changed, 9 insertions(+), 10 deletions(-)
> >
> > diff --git a/tcg/region.c b/tcg/region.c
> > index 604530b902..5e00db4cfb 100644
> > --- a/tcg/region.c
> > +++ b/tcg/region.c
> > @@ -765,12 +765,15 @@ static int alloc_code_gen_buffer(size_t size, int 
> > splitwx, Error **errp)
> >           error_free_or_abort(errp);
> >       }
> >   
> > -    prot = PROT_READ | PROT_WRITE | PROT_EXEC;
> > +    /*
> > +     * macOS 11.2 has a bug (Apple Feedback FB8994773) in which mprotect
> > +     * rejects a permission change from RWX -> NONE when reserving the
> > +     * guard pages later.  We can go the other way with the same number
> > +     * of syscalls, so always begin with PROT_NONE.
> > +     */
> > +    prot = PROT_NONE;
> >       flags = MAP_PRIVATE | MAP_ANONYMOUS;
> > -#ifdef CONFIG_TCG_INTERPRETER
> > -    /* The tcg interpreter does not need execute permission. */
> > -    prot = PROT_READ | PROT_WRITE;
> > -#elif defined(CONFIG_DARWIN)
> > +#ifdef CONFIG_DARWIN
> >       /* Applicable to both iOS and macOS (Apple Silicon). */
> >       if (!splitwx) {
> >           flags |= MAP_JIT;
> > @@ -901,11 +904,7 @@ void tcg_region_init(size_t tb_size, int splitwx, unsigned 
> > max_cpus)
> >               }
> >           }
>
> I think the comment in tcg_region_init needs to be updated, currently
> reads:
>
>      /*
>       * Set guard pages in the rw buffer, as that's the one into which
>       * buffer overruns could occur.  Do not set guard pages in the rx
>       * buffer -- let that one use hugepages throughout.
>       * Work with the page protections set up with the initial mapping.
>       */
>      need_prot = PAGE_READ | PAGE_WRITE;
>
> but now we start with everything at PROT_NONE and are just setting
> need_prot for the non-guard pages.

The comment *has* been updated: work with the page protections set up with the 
initial mapping.  Which is *not* always PROT_NONE.

See the USE_STATIC_CODE_GEN_BUFFER and _WIN32 copies of alloc_code_gen_buffer.


> >           if (have_prot != 0) {
> > -            /*
> > -             * macOS 11.2 has a bug (Apple Feedback FB8994773) in which 
> > mprotect
> > -             * rejects a permission change from RWX -> NONE.  Guard pages are
> > -             * nice for bug detection but are not essential; ignore any 
> > failure.
> > -             */
> > +            /* Guard pages are nice for bug detection but are not essential. */
> >               (void)qemu_mprotect_none(end, page_size);
>
> Isn't the last page already set as PROT_NONE?

No.


r~