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

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

[Richard Henderson]

Currently an unpredictable movw such as

  movw pc, 0x123

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);
 }
 
 /*
-- 
2.43.0

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

[Gustavo Romero]

[-- 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 --]

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

[Peter Maydell]

On Thu, 6 Nov 2025 at 15:48, Gustavo Romero <gustavo.romero@linaro.org> wrote:
>
> 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().

> 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

The commit message doesn't say this crashes, it says it
discards the result of the mask. (That is, we intended to
clear the low bits of the guest PC but don't.)

Should there be a TCG debug assert for "TCGv for the
result of an operation is a constant" ?

thanks
-- PMM

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

[Richard Henderson]

On 11/6/25 16:57, Peter Maydell wrote:
> On Thu, 6 Nov 2025 at 15:48, Gustavo Romero <gustavo.romero@linaro.org> wrote:
>>
>> 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().
> 
>> 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
> 
> The commit message doesn't say this crashes, it says it
> discards the result of the mask. (That is, we intended to
> clear the low bits of the guest PC but don't.)
> 
> Should there be a TCG debug assert for "TCGv for the
> result of an operation is a constant" ?

There is, at least with --enable-debug-tcg.
I assumed there was a crash from the description,
but I haven't yet tried the test case Gustavo put together.


r~

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

[Anton Johansson via]

On 06/11/25, Richard Henderson wrote:
> On 11/6/25 16:57, Peter Maydell wrote:
> > On Thu, 6 Nov 2025 at 15:48, Gustavo Romero <gustavo.romero@linaro.org> wrote:
> > > 
> > > 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

Not the most familiar with arm myself, but I noticed assemblers aren't
happy with producing this instruction.  We use QEMU primarily for
lifting code for decompilation, and found this instruction when lifting
some android binary.  Looking back at the instructions it might be
incorrectly identified thumb code on our end, so I doubt you'd encounter
this instruction in the wild.  Still I think the code transformation
from Richard makes sense since store_reg() is used with TCG constants.

> > > 
> > > 
> > > > 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().
> > 
> > > 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
> > 
> > The commit message doesn't say this crashes, it says it
> > discards the result of the mask. (That is, we intended to
> > clear the low bits of the guest PC but don't.)
> > 
> > Should there be a TCG debug assert for "TCGv for the
> > result of an operation is a constant" ?
> 
> There is, at least with --enable-debug-tcg.
> I assumed there was a crash from the description,
> but I haven't yet tried the test case Gustavo put together.
> 
> 
> r~
> 

-- 
Anton Johansson
rev.ng Labs Srl.

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

[Peter Maydell]

On Thu, 6 Nov 2025 at 14:49, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Currently an unpredictable movw such as
>
>   movw pc, 0x123
>
> 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>
> ---


Applied to target-arm.next, thanks; tweaked the commit
message to match the code:

    which is clearly a bug: writing to a constant is incorrect and
    discards the result of the mask.  Fix this by always doing an and_i32
    and trusting the optimizer to turn this into a simple move when the
    mask is zero.


-- PMM

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

[Anton Johansson via]

Currently an unpredictable movw such as

  movw pc, 0x123

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>
---
 target/arm/tcg/translate.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
index 5f64fed220..aeac27bbe4 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)
 {
+    TCGv_i32 masked_var = tcg_temp_new_i32();
+    tcg_gen_mov_i32(masked_var, var);
     if (reg == 15) {
         /* 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);
+        tcg_gen_andi_i32(masked_var, masked_var, 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);
+        tcg_gen_andi_i32(masked_var, masked_var, ~3);
     }
-    tcg_gen_mov_i32(cpu_R[reg], var);
+    tcg_gen_mov_i32(cpu_R[reg], masked_var);
+    tcg_gen_discard_i32(masked_var);
 }
 
 /*
-- 
2.51.0