target/arm/tcg/translate.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
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.0Hi 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>#� �
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
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~
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.
© 2016 - 2025 Red Hat, Inc.