| Message ID | 20230621161422.1652151-10-kbastian@mail.uni-paderborn.de |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/20] target/tricore: Introduce ISA 1.6.2 feature | expand |
21.06.2023 19:14, Bastian Koppelmann wrote: > From: Siqi Chen <coc.cyqh@gmail.com> > > When translating "imask" instruction of Tricore architecture, QEMU did not check whether the register index was out of bounds, resulting in a global-buffer-overflow. > > Reviewed-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1698 > Reported-by: Siqi Chen <coc.cyqh@gmail.com> > Signed-off-by: Siqi Chen <coc.cyqh@gmail.com> > Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> > Message-Id: <20230612065633.149152-1-coc.cyqh@gmail.com> > Message-Id: <20230612113245.56667-2-kbastian@mail.uni-paderborn.de> > --- > target/tricore/translate.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/target/tricore/translate.c b/target/tricore/translate.c > index 6712d98f6e..74faad4794 100644 > --- a/target/tricore/translate.c > +++ b/target/tricore/translate.c > @@ -5339,6 +5339,7 @@ static void decode_rcrw_insert(DisasContext *ctx) > > switch (op2) { > case OPC2_32_RCRW_IMASK: > + CHECK_REG_PAIR(r4); > tcg_gen_andi_tl(temp, cpu_gpr_d[r3], 0x1f); > tcg_gen_movi_tl(temp2, (1 << width) - 1); > tcg_gen_shl_tl(cpu_gpr_d[r4 + 1], temp2, temp); Is it a -stable material? Thanks, /mjt
On Thu, Jun 22, 2023 at 10:43:16AM +0300, Michael Tokarev wrote: > 21.06.2023 19:14, Bastian Koppelmann wrote: > > From: Siqi Chen <coc.cyqh@gmail.com> > > > > When translating "imask" instruction of Tricore architecture, QEMU did not check whether the register index was out of bounds, resulting in a global-buffer-overflow. > > > > Reviewed-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1698 > > Reported-by: Siqi Chen <coc.cyqh@gmail.com> > > Signed-off-by: Siqi Chen <coc.cyqh@gmail.com> > > Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> > > Message-Id: <20230612065633.149152-1-coc.cyqh@gmail.com> > > Message-Id: <20230612113245.56667-2-kbastian@mail.uni-paderborn.de> > > --- > > target/tricore/translate.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/target/tricore/translate.c b/target/tricore/translate.c > > index 6712d98f6e..74faad4794 100644 > > --- a/target/tricore/translate.c > > +++ b/target/tricore/translate.c > > @@ -5339,6 +5339,7 @@ static void decode_rcrw_insert(DisasContext *ctx) > > switch (op2) { > > case OPC2_32_RCRW_IMASK: > > + CHECK_REG_PAIR(r4); > > tcg_gen_andi_tl(temp, cpu_gpr_d[r3], 0x1f); > > tcg_gen_movi_tl(temp2, (1 << width) - 1); > > tcg_gen_shl_tl(cpu_gpr_d[r4 + 1], temp2, temp); > > Is it a -stable material? Yes. If you pick this up, make sure you also pick up https://lore.kernel.org/qemu-devel/20230621161422.1652151-1-kbastian@mail.uni-paderborn.de/T/#md18391dd165c4fc2e60ddefb886f3522e715f487 which applies the same fix to other instructions. Cheers, Bastian
22.06.2023 17:51, Bastian Koppelmann wrote: .. >> Is it a -stable material? > > Yes. If you pick this up, make sure you also pick up https://lore.kernel.org/qemu-devel/20230621161422.1652151-1-kbastian@mail.uni-paderborn.de/T/#md18391dd165c4fc2e60ddefb886f3522e715f487 > which applies the same fix to other instructions. Aha. "Add CHECK_REG_PAIR() for insn accessing 64 bit regs". This subject suggests the patch's adding this macro, instead of using it. If it were worded like "Use CHECK.. for.." instead, I'd notice this one too. Picked up both, thank you! Is there anything else in this series worth picking up for stable, eg: Fix helper_ret() not correctly restoring PSW Fix RR_JLI clobbering reg A[11] or maybe others? Please, in the future, add Cc: qemu-stable@nongnu.org for patches worth to have in -stable. Thanks! /mjt
Hi Michael, On Fri, Jun 23, 2023 at 09:54:54AM +0300, Michael Tokarev wrote: > 22.06.2023 17:51, Bastian Koppelmann wrote: > .. > > > Is it a -stable material? > > > > Yes. If you pick this up, make sure you also pick up https://lore.kernel.org/qemu-devel/20230621161422.1652151-1-kbastian@mail.uni-paderborn.de/T/#md18391dd165c4fc2e60ddefb886f3522e715f487 > > which applies the same fix to other instructions. > > Aha. "Add CHECK_REG_PAIR() for insn accessing 64 bit regs". > This subject suggests the patch's adding this macro, instead > of using it. If it were worded like "Use CHECK.. for.." instead, I'd > notice this one too. > > Picked up both, thank you! > > Is there anything else in this series worth picking up for stable, eg: > > Fix helper_ret() not correctly restoring PSW > Fix RR_JLI clobbering reg A[11] These are rare cases where the guest does something wrong. It will not lead to a crash of QEMU. > > or maybe others? > > Please, in the future, add Cc: qemu-stable@nongnu.org for patches > worth to have in -stable. I will do that. I'm not sure what is worth while to pick up for stable. My initial thought was fixes for bugs that can lead to a crash in QEMU. Any pointer would make it easier for me to decide what to CC: qemu-stable@nongnu.org for. Thanks, Bastian
23.06.2023 12:51, Bastian Koppelmann wrote: >> Is there anything else in this series worth picking up for stable, eg: >> >> Fix helper_ret() not correctly restoring PSW >> Fix RR_JLI clobbering reg A[11] > > These are rare cases where the guest does something wrong. It will not lead to a > crash of QEMU. Ok, makes sense. >> Please, in the future, add Cc: qemu-stable@nongnu.org for patches >> worth to have in -stable. > > I will do that. I'm not sure what is worth while to pick up for stable. My > initial thought was fixes for bugs that can lead to a crash in QEMU. Any pointer > would make it easier for me to decide what to CC: qemu-stable@nongnu.org for. Here we go: https://www.qemu.org/docs/master/devel/stable-process.html Basically, any bugfix you, as a subsystem maintainer, think is good for stable, is good for stable :) Usual tradeoff applies: more complex stuff with potential to break something vs seriousness of an issue. Thank you! /mjt
On Fri, Jun 23, 2023 at 01:29:23PM +0300, Michael Tokarev wrote: > 23.06.2023 12:51, Bastian Koppelmann wrote: > > Here we go: > https://www.qemu.org/docs/master/devel/stable-process.html > > Basically, any bugfix you, as a subsystem maintainer, think is good for stable, > is good for stable :) Usual tradeoff applies: more complex stuff with potential > to break something vs seriousness of an issue. That helps a lot! Thanks, Bastian
diff --git a/target/tricore/translate.c b/target/tricore/translate.c index 6712d98f6e..74faad4794 100644 --- a/target/tricore/translate.c +++ b/target/tricore/translate.c @@ -5339,6 +5339,7 @@ static void decode_rcrw_insert(DisasContext *ctx) switch (op2) { case OPC2_32_RCRW_IMASK: + CHECK_REG_PAIR(r4); tcg_gen_andi_tl(temp, cpu_gpr_d[r3], 0x1f); tcg_gen_movi_tl(temp2, (1 << width) - 1); tcg_gen_shl_tl(cpu_gpr_d[r4 + 1], temp2, temp);