[PULL,09/20] target/tricore: Fix out-of-bounds index in imask instruction

Commit Message

Bastian Koppelmann June 21, 2023, 4:14 p.m. UTC

From: Siqi Chen <[e-mail]>

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 <[e-mail]>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1698
Reported-by: Siqi Chen <[e-mail]>
Signed-off-by: Siqi Chen <[e-mail]>
Signed-off-by: Bastian Koppelmann <[e-mail]>
Message-Id: <[e-mail]>
Message-Id: <[e-mail]>
---
 target/tricore/translate.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Michael Tokarev June 22, 2023, 7:43 a.m. UTC | #1

21.06.2023 19:14, Bastian Koppelmann wrote:
> From: Siqi Chen <[e-mail]>
> 
> 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 <[e-mail]>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1698
> Reported-by: Siqi Chen <[e-mail]>
> Signed-off-by: Siqi Chen <[e-mail]>
> Signed-off-by: Bastian Koppelmann <[e-mail]>
> Message-Id: <[e-mail]>
> Message-Id: <[e-mail]>
> ---
>   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

Bastian Koppelmann June 22, 2023, 2:51 p.m. UTC | #2

On Thu, Jun 22, 2023 at 10:43:16AM +0300, Michael Tokarev wrote:
> 21.06.2023 19:14, Bastian Koppelmann wrote:
> > From: Siqi Chen <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1698
> > Reported-by: Siqi Chen <[e-mail]>
> > Signed-off-by: Siqi Chen <[e-mail]>
> > Signed-off-by: Bastian Koppelmann <[e-mail]>
> > Message-Id: <[e-mail]>
> > Message-Id: <[e-mail]>
> > ---
> >   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/[e-mail]/T/#md18391dd165c4fc2e60ddefb886f3522e715f487
which applies the same fix to other instructions.

Cheers,
Bastian

Michael Tokarev June 23, 2023, 6:54 a.m. UTC | #3

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/[e-mail]/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: [e-mail] for patches
worth to have in -stable.

Thanks!

/mjt

Bastian Koppelmann June 23, 2023, 9:51 a.m. UTC | #4

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/[e-mail]/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: [e-mail] 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: [e-mail] for.

Thanks,
Bastian

Michael Tokarev June 23, 2023, 10:29 a.m. UTC | #5

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: [e-mail] 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: [e-mail] 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

Bastian Koppelmann June 23, 2023, 11:09 a.m. UTC | #6

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

Patch

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);