* [Qemu-devel] [PATCH] tcg: Fix mmap lock assert on translation failure
@ 2019-06-16 21:05 Richard Henderson
2019-06-17 12:41 ` Christophe Lyon
2019-06-17 16:18 ` Alex Bennée
0 siblings, 2 replies; 5+ messages in thread
From: Richard Henderson @ 2019-06-16 21:05 UTC (permalink / raw)
To: qemu-devel; +Cc: christophe.lyon, alex.bennee
Check page flags before letting an invalid pc cause a SIGSEGV.
Prepare for eventially validating PROT_EXEC. The current wrinkle being
that we have a problem with our implementation of signals. We should
be using a vdso like the kernel, but we instead put the trampoline on
the stack. In the meantime, let PROT_READ match PROT_EXEC.
Fixes: https://bugs.launchpad.net/qemu/+bug/1832353
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/exec/cpu-all.h | 1 +
include/exec/cpu_ldst_useronly_template.h | 8 +++++--
accel/tcg/translate-all.c | 29 +++++++++++++++++++++++
3 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 536ea58f81..58b8915617 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -259,6 +259,7 @@ int walk_memory_regions(void *, walk_memory_regions_fn);
int page_get_flags(target_ulong address);
void page_set_flags(target_ulong start, target_ulong end, int flags);
int page_check_range(target_ulong start, target_ulong len, int flags);
+void validate_exec_access(CPUArchState *env, target_ulong s, target_ulong l);
#endif
CPUArchState *cpu_copy(CPUArchState *env);
diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
index bc45e2b8d4..f095415149 100644
--- a/include/exec/cpu_ldst_useronly_template.h
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -64,7 +64,9 @@
static inline RES_TYPE
glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
{
-#if !defined(CODE_ACCESS)
+#ifdef CODE_ACCESS
+ validate_exec_access(env, ptr, DATA_SIZE);
+#else
trace_guest_mem_before_exec(
env_cpu(env), ptr,
trace_mem_build_info(SHIFT, false, MO_TE, false));
@@ -88,7 +90,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
static inline int
glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
{
-#if !defined(CODE_ACCESS)
+#ifdef CODE_ACCESS
+ validate_exec_access(env, ptr, DATA_SIZE);
+#else
trace_guest_mem_before_exec(
env_cpu(env), ptr,
trace_mem_build_info(SHIFT, true, MO_TE, false));
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 5d1e08b169..1d4a8a260f 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2600,10 +2600,39 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
}
}
}
+ /*
+ * FIXME: We place the signal trampoline on the stack,
+ * even when the guest expects that to be in the vdso.
+ * Until we fix that, allow execute on any readable page.
+ */
+ if ((flags & PAGE_EXEC) && !(p->flags & (PAGE_EXEC | PAGE_READ))) {
+ return -1;
+ }
}
return 0;
}
+/*
+ * Called for each code read, longjmp out to issue SIGSEGV if the page(s)
+ * do not have execute access.
+ */
+void validate_exec_access(CPUArchState *env,
+ target_ulong ptr, target_ulong len)
+{
+ if (page_check_range(ptr, len, PAGE_EXEC) < 0) {
+ CPUState *cs = env_cpu(env);
+ CPUClass *cc = CPU_GET_CLASS(cs);
+
+ /* Like tb_gen_code, release the memory lock before cpu_loop_exit. */
+ assert_memory_lock();
+ mmap_unlock();
+
+ /* This is user-only. The target must raise an exception. */
+ cc->tlb_fill(cs, ptr, 0, MMU_INST_FETCH, MMU_USER_IDX, false, 0);
+ g_assert_not_reached();
+ }
+}
+
/* called from signal handler: invalidate the code and unprotect the
* page. Return 0 if the fault was not handled, 1 if it was handled,
* and 2 if it was handled but the caller must cause the TB to be
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Fix mmap lock assert on translation failure
2019-06-16 21:05 [Qemu-devel] [PATCH] tcg: Fix mmap lock assert on translation failure Richard Henderson
@ 2019-06-17 12:41 ` Christophe Lyon
2019-06-17 16:18 ` Alex Bennée
1 sibling, 0 replies; 5+ messages in thread
From: Christophe Lyon @ 2019-06-17 12:41 UTC (permalink / raw)
To: Richard Henderson; +Cc: Alex Bennée, QEMU Developers
On Sun, 16 Jun 2019 at 23:05, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Check page flags before letting an invalid pc cause a SIGSEGV.
>
> Prepare for eventially validating PROT_EXEC. The current wrinkle being
> that we have a problem with our implementation of signals. We should
> be using a vdso like the kernel, but we instead put the trampoline on
> the stack. In the meantime, let PROT_READ match PROT_EXEC.
>
Thanks for the quick fix, I can confirm it works for my testcase.
Christophe
> Fixes: https://bugs.launchpad.net/qemu/+bug/1832353
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> include/exec/cpu-all.h | 1 +
> include/exec/cpu_ldst_useronly_template.h | 8 +++++--
> accel/tcg/translate-all.c | 29 +++++++++++++++++++++++
> 3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 536ea58f81..58b8915617 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -259,6 +259,7 @@ int walk_memory_regions(void *, walk_memory_regions_fn);
> int page_get_flags(target_ulong address);
> void page_set_flags(target_ulong start, target_ulong end, int flags);
> int page_check_range(target_ulong start, target_ulong len, int flags);
> +void validate_exec_access(CPUArchState *env, target_ulong s, target_ulong l);
> #endif
>
> CPUArchState *cpu_copy(CPUArchState *env);
> diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
> index bc45e2b8d4..f095415149 100644
> --- a/include/exec/cpu_ldst_useronly_template.h
> +++ b/include/exec/cpu_ldst_useronly_template.h
> @@ -64,7 +64,9 @@
> static inline RES_TYPE
> glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
> {
> -#if !defined(CODE_ACCESS)
> +#ifdef CODE_ACCESS
> + validate_exec_access(env, ptr, DATA_SIZE);
> +#else
> trace_guest_mem_before_exec(
> env_cpu(env), ptr,
> trace_mem_build_info(SHIFT, false, MO_TE, false));
> @@ -88,7 +90,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
> static inline int
> glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
> {
> -#if !defined(CODE_ACCESS)
> +#ifdef CODE_ACCESS
> + validate_exec_access(env, ptr, DATA_SIZE);
> +#else
> trace_guest_mem_before_exec(
> env_cpu(env), ptr,
> trace_mem_build_info(SHIFT, true, MO_TE, false));
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 5d1e08b169..1d4a8a260f 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -2600,10 +2600,39 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
> }
> }
> }
> + /*
> + * FIXME: We place the signal trampoline on the stack,
> + * even when the guest expects that to be in the vdso.
> + * Until we fix that, allow execute on any readable page.
> + */
> + if ((flags & PAGE_EXEC) && !(p->flags & (PAGE_EXEC | PAGE_READ))) {
> + return -1;
> + }
> }
> return 0;
> }
>
> +/*
> + * Called for each code read, longjmp out to issue SIGSEGV if the page(s)
> + * do not have execute access.
> + */
> +void validate_exec_access(CPUArchState *env,
> + target_ulong ptr, target_ulong len)
> +{
> + if (page_check_range(ptr, len, PAGE_EXEC) < 0) {
> + CPUState *cs = env_cpu(env);
> + CPUClass *cc = CPU_GET_CLASS(cs);
> +
> + /* Like tb_gen_code, release the memory lock before cpu_loop_exit. */
> + assert_memory_lock();
> + mmap_unlock();
> +
> + /* This is user-only. The target must raise an exception. */
> + cc->tlb_fill(cs, ptr, 0, MMU_INST_FETCH, MMU_USER_IDX, false, 0);
> + g_assert_not_reached();
> + }
> +}
> +
> /* called from signal handler: invalidate the code and unprotect the
> * page. Return 0 if the fault was not handled, 1 if it was handled,
> * and 2 if it was handled but the caller must cause the TB to be
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Fix mmap lock assert on translation failure
2019-06-16 21:05 [Qemu-devel] [PATCH] tcg: Fix mmap lock assert on translation failure Richard Henderson
2019-06-17 12:41 ` Christophe Lyon
@ 2019-06-17 16:18 ` Alex Bennée
2019-06-17 16:58 ` Richard Henderson
1 sibling, 1 reply; 5+ messages in thread
From: Alex Bennée @ 2019-06-17 16:18 UTC (permalink / raw)
To: Richard Henderson; +Cc: christophe.lyon, qemu-devel
Richard Henderson <richard.henderson@linaro.org> writes:
> Check page flags before letting an invalid pc cause a SIGSEGV.
>
> Prepare for eventially validating PROT_EXEC. The current wrinkle being
> that we have a problem with our implementation of signals. We should
> be using a vdso like the kernel, but we instead put the trampoline on
> the stack. In the meantime, let PROT_READ match PROT_EXEC.
We can come up with a test case for this right? Would it be triggered by
having:
__attribute__((aligned(PAGE_SIZE)))
void some_func(void) {
/* does something */
}
__attribute__((aligned(PAGE_SIZE)))
... rest of code ...
main () {
mmap(&some_func, PAGE_SIZE, PROT_READ, MAP_ANONYMOUS, 0, 0);
some_func()
/* causes SEGV */
mmap(&some_func, PAGE_SIZE, PROT_READ|PROT_EXEC, MAP_ANONYMOUS, 0, 0);
some_func()
/* works */
}
Or is it trickier to mess with your own mapped memory?
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1832353
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> include/exec/cpu-all.h | 1 +
> include/exec/cpu_ldst_useronly_template.h | 8 +++++--
> accel/tcg/translate-all.c | 29 +++++++++++++++++++++++
> 3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
> index 536ea58f81..58b8915617 100644
> --- a/include/exec/cpu-all.h
> +++ b/include/exec/cpu-all.h
> @@ -259,6 +259,7 @@ int walk_memory_regions(void *, walk_memory_regions_fn);
> int page_get_flags(target_ulong address);
> void page_set_flags(target_ulong start, target_ulong end, int flags);
> int page_check_range(target_ulong start, target_ulong len, int flags);
> +void validate_exec_access(CPUArchState *env, target_ulong s, target_ulong l);
> #endif
>
> CPUArchState *cpu_copy(CPUArchState *env);
> diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
> index bc45e2b8d4..f095415149 100644
> --- a/include/exec/cpu_ldst_useronly_template.h
> +++ b/include/exec/cpu_ldst_useronly_template.h
> @@ -64,7 +64,9 @@
> static inline RES_TYPE
> glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
> {
> -#if !defined(CODE_ACCESS)
> +#ifdef CODE_ACCESS
> + validate_exec_access(env, ptr, DATA_SIZE);
> +#else
> trace_guest_mem_before_exec(
> env_cpu(env), ptr,
> trace_mem_build_info(SHIFT, false, MO_TE, false));
> @@ -88,7 +90,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
> static inline int
> glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
> {
> -#if !defined(CODE_ACCESS)
> +#ifdef CODE_ACCESS
> + validate_exec_access(env, ptr, DATA_SIZE);
> +#else
> trace_guest_mem_before_exec(
> env_cpu(env), ptr,
> trace_mem_build_info(SHIFT, true, MO_TE, false));
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 5d1e08b169..1d4a8a260f 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -2600,10 +2600,39 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
> }
> }
> }
> + /*
> + * FIXME: We place the signal trampoline on the stack,
> + * even when the guest expects that to be in the vdso.
> + * Until we fix that, allow execute on any readable page.
> + */
> + if ((flags & PAGE_EXEC) && !(p->flags & (PAGE_EXEC | PAGE_READ))) {
> + return -1;
> + }
> }
> return 0;
> }
>
> +/*
> + * Called for each code read, longjmp out to issue SIGSEGV if the page(s)
> + * do not have execute access.
> + */
> +void validate_exec_access(CPUArchState *env,
> + target_ulong ptr, target_ulong len)
> +{
> + if (page_check_range(ptr, len, PAGE_EXEC) < 0) {
> + CPUState *cs = env_cpu(env);
> + CPUClass *cc = CPU_GET_CLASS(cs);
> +
> + /* Like tb_gen_code, release the memory lock before cpu_loop_exit. */
> + assert_memory_lock();
> + mmap_unlock();
> +
> + /* This is user-only. The target must raise an exception. */
> + cc->tlb_fill(cs, ptr, 0, MMU_INST_FETCH, MMU_USER_IDX, false, 0);
> + g_assert_not_reached();
> + }
> +}
> +
> /* called from signal handler: invalidate the code and unprotect the
> * page. Return 0 if the fault was not handled, 1 if it was handled,
> * and 2 if it was handled but the caller must cause the TB to be
--
Alex Bennée
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Fix mmap lock assert on translation failure
2019-06-17 16:18 ` Alex Bennée
@ 2019-06-17 16:58 ` Richard Henderson
2019-06-17 18:10 ` Alex Bennée
0 siblings, 1 reply; 5+ messages in thread
From: Richard Henderson @ 2019-06-17 16:58 UTC (permalink / raw)
To: Alex Bennée; +Cc: christophe.lyon, qemu-devel
On 6/17/19 9:18 AM, Alex Bennée wrote:
>
> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> Check page flags before letting an invalid pc cause a SIGSEGV.
>>
>> Prepare for eventially validating PROT_EXEC. The current wrinkle being
>> that we have a problem with our implementation of signals. We should
>> be using a vdso like the kernel, but we instead put the trampoline on
>> the stack. In the meantime, let PROT_READ match PROT_EXEC.
>
> We can come up with a test case for this right? Would it be triggered by
> having:
>
> __attribute__((aligned(PAGE_SIZE)))
> void some_func(void) {
> /* does something */
> }
>
> __attribute__((aligned(PAGE_SIZE)))
> ... rest of code ...
>
> main () {
> mmap(&some_func, PAGE_SIZE, PROT_READ, MAP_ANONYMOUS, 0, 0);
> some_func()
> /* causes SEGV */
> mmap(&some_func, PAGE_SIZE, PROT_READ|PROT_EXEC, MAP_ANONYMOUS, 0, 0);
> some_func()
> /* works */
> }
>
> Or is it trickier to mess with your own mapped memory?
It's trickier than that, but I do have a simple test case.
https://bugs.launchpad.net/qemu/+bug/1832916
But fixing that, as I mention above, makes signal trampolines fail.
Or did you mean for Christophe's failure? That's easier -- just make a NULL
function call.
r~
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Fix mmap lock assert on translation failure
2019-06-17 16:58 ` Richard Henderson
@ 2019-06-17 18:10 ` Alex Bennée
0 siblings, 0 replies; 5+ messages in thread
From: Alex Bennée @ 2019-06-17 18:10 UTC (permalink / raw)
To: Richard Henderson; +Cc: christophe.lyon, qemu-devel
Richard Henderson <richard.henderson@linaro.org> writes:
> On 6/17/19 9:18 AM, Alex Bennée wrote:
>>
>> Richard Henderson <richard.henderson@linaro.org> writes:
>>
>>> Check page flags before letting an invalid pc cause a SIGSEGV.
>>>
>>> Prepare for eventially validating PROT_EXEC. The current wrinkle being
>>> that we have a problem with our implementation of signals. We should
>>> be using a vdso like the kernel, but we instead put the trampoline on
>>> the stack. In the meantime, let PROT_READ match PROT_EXEC.
>>
>> We can come up with a test case for this right? Would it be triggered by
>> having:
>>
>> __attribute__((aligned(PAGE_SIZE)))
>> void some_func(void) {
>> /* does something */
>> }
>>
>> __attribute__((aligned(PAGE_SIZE)))
>> ... rest of code ...
>>
>> main () {
>> mmap(&some_func, PAGE_SIZE, PROT_READ, MAP_ANONYMOUS, 0, 0);
>> some_func()
>> /* causes SEGV */
>> mmap(&some_func, PAGE_SIZE, PROT_READ|PROT_EXEC, MAP_ANONYMOUS, 0, 0);
>> some_func()
>> /* works */
>> }
>>
>> Or is it trickier to mess with your own mapped memory?
>
> It's trickier than that, but I do have a simple test case.
>
> https://bugs.launchpad.net/qemu/+bug/1832916
>
> But fixing that, as I mention above, makes signal trampolines fail.
Ahh I missed that. I guess we add it once we have the full solution.
>
> Or did you mean for Christophe's failure? That's easier -- just make a NULL
> function call.
>
>
> r~
--
Alex Bennée
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-17 18:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-16 21:05 [Qemu-devel] [PATCH] tcg: Fix mmap lock assert on translation failure Richard Henderson
2019-06-17 12:41 ` Christophe Lyon
2019-06-17 16:18 ` Alex Bennée
2019-06-17 16:58 ` Richard Henderson
2019-06-17 18:10 ` Alex Bennée
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.