Re: [PATCH] target/arm: Fix accidental write to TCG constant

[Gustavo Romero <gustavo.romero@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 2574 bytes --]

Hi folks,

On 11/6/25 15:49, Richard Henderson wrote:
> Currently an unpredictable movw such as
> 
>    movw pc, 0x123

bah, how did you get this insn.? Are you using any fuzzer? :P


> results in the tinycode
> 
>     and_i32 $0x123,$0x123,$0xfffffffc
>     mov_i32 pc,$0x123
>     exit_tb $0x0
> 
> which is clearly a bug, writing to a constant is incorrect and discards
> the result of the mask.  Fix this by adding a temporary in store_reg().
> 
> Signed-off-by: Anton Johansson <anjo@rev.ng>
> [rth: Avoid an extra temp and extra move.]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/tcg/translate.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
> index 5f64fed220..63735d9789 100644
> --- a/target/arm/tcg/translate.c
> +++ b/target/arm/tcg/translate.c
> @@ -303,20 +303,23 @@ TCGv_i32 add_reg_for_lit(DisasContext *s, int reg, int ofs)
>      marked as dead.  */
>   void store_reg(DisasContext *s, int reg, TCGv_i32 var)
>   {
> +    uint32_t mask = 0;
> +
>       if (reg == 15) {
> -        /* In Thumb mode, we must ignore bit 0.
> +        /*
> +         * In Thumb mode, we must ignore bit 0.
>            * In ARM mode, for ARMv4 and ARMv5, it is UNPREDICTABLE if bits [1:0]
>            * are not 0b00, but for ARMv6 and above, we must ignore bits [1:0].
>            * We choose to ignore [1:0] in ARM mode for all architecture versions.
>            */
> -        tcg_gen_andi_i32(var, var, s->thumb ? ~1 : ~3);
> +        mask = s->thumb ? 1 : 3;
>           s->base.is_jmp = DISAS_JUMP;
>           s->pc_save = -1;
>       } else if (reg == 13 && arm_dc_feature(s, ARM_FEATURE_M)) {
>           /* For M-profile SP bits [1:0] are always zero */
> -        tcg_gen_andi_i32(var, var, ~3);
> +        mask = 3;
>       }
> -    tcg_gen_mov_i32(cpu_R[reg], var);
> +    tcg_gen_andi_i32(cpu_R[reg], var, ~mask);
>   }

The difference between v1 and v2 is:

v1:
  mov_i32 tmp3,$0x123
  and_i32 tmp3,tmp3,$0xfffffffc
  mov_i32 pc,tmp3

v2 (this version)
and_i32 pc,$0x123,$0xfffffffc


I think we need only a v3 that updates the commit message since we
are not adding a temporary anymore.

Interestingly, I was not able to crash the host when native code
was generated from:

and_i32 $0x123,$0x123,$0xfffffffc

I'm sending the binary I used to test it attached for convenience.

Anyways:

Tested-by: Gustavo Romero <gustavo.romero@linaro.org>

and with the commit message updated:

Reviewed-by: <gustavo.romero@linaro.org>

[-- Attachment #2: movw_pc.bin --]
[-- Type: application/octet-stream, Size: 4 bytes --]