Re: [Qemu-devel] [PATCH] target-arm: check that LSB <= MSB in BFI instruction

[Peter Maydell]

On 30 January 2015 at 12:36, Kirill Batuzov <address@hidden> wrote:
> The documentation states that if LSB > MSB in BFI instruction behaviour
> is unpredictable. Currently QEMU crashes because of assertion failure in
> this case:
>
> tcg/tcg-op.h:2061: tcg_gen_deposit_i32: Assertion `len <= 32' failed.
>
> While assertion failure may meet the "unpredictable" definition this
> behaviour is undesirable because it allows an unprivileged guest program
> to crash the emulator with the OS and other programs.

Thanks for this bug fix. Some minor nits:

> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index bdfcdf1..2821289 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -8739,6 +8739,8 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>                          ARCH(6T2);
>                          shift = (insn >> 7) & 0x1f;
>                          i = (insn >> 16) & 0x1f;
> +                        if (i < shift)
> +                            goto illegal_op;

This needs braces to comply with coding style. checkpatch.pl
should warn you about this.

I also like to comment this kind of check with
 /* UNPREDICTABLE; we choose to UNDEF */

>                          i = i + 1 - shift;
>                          if (rm == 15) {
>                              tmp = tcg_temp_new_i32();

I checked the Thumb decoder, and that code seems to have
this test already, so it's just the ARM decoder that
needs fixing.

thanks
-- PMM