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

[Alex Bennée]

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.

>          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?

>          }
>      }


-- 
Alex Bennée