Richard Henderson <richard.hender...@linaro.org> writes:
> At present we have a potential error in that helper_retaddr contains
> data for handle_cpu_signal, but we have not ensured that those stores
> will be scheduled properly before the operation that may fault.
>
> It might be that these races are not in practice observable, due to
> our use of -fno-strict-aliasing, but better safe than sorry.
>
> Adjust all of the setters of helper_retaddr.
>
> Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
> ---
> include/exec/cpu_ldst.h | 20 +++++++++++
> include/exec/cpu_ldst_useronly_template.h | 12 +++----
> accel/tcg/user-exec.c | 11 +++---
> target/arm/helper-a64.c | 8 ++---
> target/arm/sve_helper.c | 43 +++++++++++------------
> 5 files changed, 57 insertions(+), 37 deletions(-)
>
> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
> index a08b11bd2c..9de8c93303 100644
> --- a/include/exec/cpu_ldst.h
> +++ b/include/exec/cpu_ldst.h
> @@ -89,6 +89,26 @@ typedef target_ulong abi_ptr;
>
> extern __thread uintptr_t helper_retaddr;
>
> +static inline void set_helper_retaddr(uintptr_t ra)
> +{
> + helper_retaddr = ra;
> + /*
> + * Ensure that this write is visible to the SIGSEGV handler that
> + * may be invoked due to a subsequent invalid memory operation.
> + */
> + signal_barrier();
> +}
> +
> +static inline void clear_helper_retaddr(void)
> +{
> + /*
> + * Ensure that previous memory operations have succeeded before
> + * removing the data visible to the signal handler.
> + */
> + signal_barrier();
> + helper_retaddr = 0;
> +}
> +
> /* In user-only mode we provide only the _code and _data accessors. */
>
> #define MEMSUFFIX _data
> diff --git a/include/exec/cpu_ldst_useronly_template.h
> b/include/exec/cpu_ldst_useronly_template.h
> index bc45e2b8d4..e65733f7e2 100644
> --- a/include/exec/cpu_ldst_useronly_template.h
> +++ b/include/exec/cpu_ldst_useronly_template.h
> @@ -78,9 +78,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX),
> _ra)(CPUArchState *env,
> uintptr_t retaddr)
> {
> RES_TYPE ret;
> - helper_retaddr = retaddr;
> + set_helper_retaddr(retaddr);
> ret = glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(env, ptr);
> - helper_retaddr = 0;
> + clear_helper_retaddr();
> return ret;
> }
>
> @@ -102,9 +102,9 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX),
> _ra)(CPUArchState *env,
> uintptr_t retaddr)
> {
> int ret;
> - helper_retaddr = retaddr;
> + set_helper_retaddr(retaddr);
> ret = glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(env, ptr);
> - helper_retaddr = 0;
> + clear_helper_retaddr();
> return ret;
> }
> #endif
> @@ -128,9 +128,9 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX),
> _ra)(CPUArchState *env,
> RES_TYPE v,
> uintptr_t retaddr)
> {
> - helper_retaddr = retaddr;
> + set_helper_retaddr(retaddr);
> glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(env, ptr, v);
> - helper_retaddr = 0;
> + clear_helper_retaddr();
> }
> #endif
>
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index cb5f4b19c5..4384b59a4d 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -134,7 +134,7 @@ static inline int handle_cpu_signal(uintptr_t pc,
> siginfo_t *info,
> * currently executing TB was modified and must be exited
> * immediately. Clear helper_retaddr for next execution.
> */
> - helper_retaddr = 0;
> + clear_helper_retaddr();
> cpu_exit_tb_from_sighandler(cpu, old_set);
> /* NORETURN */
>
> @@ -152,7 +152,7 @@ static inline int handle_cpu_signal(uintptr_t pc,
> siginfo_t *info,
> * an exception. Undo signal and retaddr state prior to longjmp.
> */
> sigprocmask(SIG_SETMASK, old_set, NULL);
> - helper_retaddr = 0;
> + clear_helper_retaddr();
>
> cc = CPU_GET_CLASS(cpu);
> access_type = is_write ? MMU_DATA_STORE : MMU_DATA_LOAD;
> @@ -682,14 +682,15 @@ static void *atomic_mmu_lookup(CPUArchState *env,
> target_ulong addr,
> if (unlikely(addr & (size - 1))) {
> cpu_loop_exit_atomic(env_cpu(env), retaddr);
> }
> - helper_retaddr = retaddr;
> - return g2h(addr);
> + void *ret = g2h(addr);
> + set_helper_retaddr(retaddr);
> + return ret;
> }
>
> /* Macro to call the above, with local variables from the use context. */
> #define ATOMIC_MMU_DECLS do {} while (0)
> #define ATOMIC_MMU_LOOKUP atomic_mmu_lookup(env, addr, DATA_SIZE, GETPC())
> -#define ATOMIC_MMU_CLEANUP do { helper_retaddr = 0; } while (0)
> +#define ATOMIC_MMU_CLEANUP do { clear_helper_retaddr(); } while (0)
>
> #define ATOMIC_NAME(X) HELPER(glue(glue(atomic_ ## X, SUFFIX), END))
> #define EXTRA_ARGS
> diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
> index 44e45a8037..060699b901 100644
> --- a/target/arm/helper-a64.c
> +++ b/target/arm/helper-a64.c
> @@ -554,7 +554,7 @@ uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env,
> uint64_t addr,
> /* ??? Enforce alignment. */
> uint64_t *haddr = g2h(addr);
>
> - helper_retaddr = ra;
> + set_helper_retaddr(ra);
> o0 = ldq_le_p(haddr + 0);
> o1 = ldq_le_p(haddr + 1);
> oldv = int128_make128(o0, o1);
> @@ -564,7 +564,7 @@ uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env,
> uint64_t addr,
> stq_le_p(haddr + 0, int128_getlo(newv));
> stq_le_p(haddr + 1, int128_gethi(newv));
> }
> - helper_retaddr = 0;
> + clear_helper_retaddr();
> #else
> int mem_idx = cpu_mmu_index(env, false);
> TCGMemOpIdx oi0 = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
> @@ -624,7 +624,7 @@ uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env,
> uint64_t addr,
> /* ??? Enforce alignment. */
> uint64_t *haddr = g2h(addr);
>
> - helper_retaddr = ra;
> + set_helper_retaddr(ra);
> o1 = ldq_be_p(haddr + 0);
> o0 = ldq_be_p(haddr + 1);
> oldv = int128_make128(o0, o1);
> @@ -634,7 +634,7 @@ uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env,
> uint64_t addr,
> stq_be_p(haddr + 0, int128_gethi(newv));
> stq_be_p(haddr + 1, int128_getlo(newv));
> }
> - helper_retaddr = 0;
> + clear_helper_retaddr();
> #else
> int mem_idx = cpu_mmu_index(env, false);
> TCGMemOpIdx oi0 = make_memop_idx(MO_BEQ | MO_ALIGN_16, mem_idx);
> diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
> index fd434c66ea..fc0c1755d2 100644
> --- a/target/arm/sve_helper.c
> +++ b/target/arm/sve_helper.c
> @@ -4125,12 +4125,11 @@ static intptr_t max_for_page(target_ulong base,
> intptr_t mem_off,
> return MIN(split, mem_max - mem_off) + mem_off;
> }
>
> -static inline void set_helper_retaddr(uintptr_t ra)
> -{
> -#ifdef CONFIG_USER_ONLY
> - helper_retaddr = ra;
> +#ifndef CONFIG_USER_ONLY
> +/* These are normally defined only for CONFIG_USER_ONLY in <exec/cpu_ldst.h>
> */
> +static inline void set_helper_retaddr(uintptr_t ra) { }
> +static inline void clear_helper_retaddr(void) { }
Why aren't these stubs in the #else leg of cpu_ldst.h?
With that:
Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
--
Alex Bennée
