Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Check undefined opcodes for SWP in A32 decoder

[Peter Maydell]

On 23 March 2018 at 03:58, Onur Sahin <address@hidden> wrote:
> Hi all,
>
> I have noticed that the decoding part in ARM/A32 does not verify the
> opcodes for SWP instructions. The opcode field ([23:20]) for SWP
> instructions should be 0 or 4, and QEMU does not check against these
> values.
>
> Other opcode values less than 8 are Undefined within the encoding
> space of sychronization primitives (e.g., SWP, LDREX*). See section
> A5.2.10 of ARMv7-A manual for reference. Because of the missing opcode
> check, QEMU happily executes these Undefined cases as a SWP instruction.
>
> The following fix adds proper opcode checks before assuming a valid SWP.
>
> Best,
> Onur
>
> Signed-off-by: Onur Sahin <address@hidden>

Yes, we've been historically a bit lax with our arm decoding.
It's good to tighten it up, I think. Thanks for sending this patch;
I have some review comments below.

> ---
>  target-arm/translate.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index bd5d5cb..fb31c12 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -8831,7 +8831,7 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>                              }
>                          }
>                          tcg_temp_free_i32(addr);
> -                    } else {
> +                    } else if (!(insn & 0x00B00000)) {

I think we have already checked bit 23, so we don't need to check it again
(this is the else case of the "if (insn & (1 << 23))" condition).
I think we should also be checking bits [11:8] are zeroes. These are
"(0)" in the instruction description, which means that the instruction
is UNPREDICTABLE if those are not zero (see A5.1.2 "UNDEFINED and
UNPREDICTABLE instruction set space"). It's architecturally permitted
to not check those bits, but elsewhere in the decoder we generally
prefer to UNDEF on the UNPREDICTABLE cases.

So I would make this condition
    } else if ((insn & 0x00300f00) == 0) {

I have been adding comments to the decoder as I modify it which
indicate what bits have been decoded at various points. In this case
I think we should add
       /* 0bcccc_0001_0x00_xxxx_xxxx_0000_1001_xxxx
        *  - SWP, SWPB
        */

just inside this else if () { ...} block.

(and then you can delete the "SWP instruction" comment.)

>                          /* SWP instruction */

What version of QEMU did you make this patch against? The current
git master has some other lines between the '} else {' and this
comment. Generally patches should be against git master, as that's
where all our development happens.

>                          rm = (insn) & 0xf;
>
> @@ -8852,6 +8852,9 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>                          tcg_temp_free_i32(addr);
>                          store_reg(s, rd, tmp2);
>                      }
> +                    else {

Minor style nit: the "else {" should go on the same line as the
preceding "}".

> +                        goto illegal_op;
> +                    }
>                  }
>              } else {
>                  int address_offset;
> --
> 1.8.3.1

thanks
-- PMM