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

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

[Onur Sahin]

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 <onursahin08@gmail.com>
---
 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)) {
                         /* SWP instruction */
                         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 {
+                        goto illegal_op;
+                    }
                 }
             } else {
                 int address_offset;
-- 
1.8.3.1

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 <onursahin08@gmail.com> 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 <onursahin08@gmail.com>

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

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

[Onur Sahin]

Thanks for the feedback Peter. Removing the redundant check on bit
23 and adding checks for the "should be" bits as well (bits [11:8]).

The following patch should make sure we are not treating
architecturally Undefined instructions as a SWP, by verifying
the opcodes as per section A8.8.229 of ARMv7-A specification.

Best,
Onur

Signed-off-by: Onur Sahin <onursahin08@gmail.com>
---
 target/arm/translate.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index ba6ab7d..1fb0b8f 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9227,11 +9227,14 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
                             }
                         }
                         tcg_temp_free_i32(addr);
-                    } else {
+                    } else if ((insn & 0x00300f00) == 0) {
+                        /* 0bcccc_0001_0x00_xxxx_xxxx_0000_1001_xxxx
+                        *  - SWP, SWPB
+                        */
+
                         TCGv taddr;
                         TCGMemOp opc = s->be_data;
 
-                        /* SWP instruction */
                         rm = (insn) & 0xf;
 
                         if (insn & (1 << 22)) {
@@ -9249,6 +9252,8 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
                                                 get_mem_index(s), opc);
                         tcg_temp_free(taddr);
                         store_reg(s, rd, tmp);
+                    } else {
+                        goto illegal_op;
                     }
                 }
             } else {
-- 
1.8.3.1

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

[Peter Maydell]

On 23 March 2018 at 21:43, Onur Sahin <onursahin08@gmail.com> wrote:
> Thanks for the feedback Peter. Removing the redundant check on bit
> 23 and adding checks for the "should be" bits as well (bits [11:8]).
>
> The following patch should make sure we are not treating
> architecturally Undefined instructions as a SWP, by verifying
> the opcodes as per section A8.8.229 of ARMv7-A specification.
>
> Best,
> Onur
>
> Signed-off-by: Onur Sahin <onursahin08@gmail.com>
> ---

Thanks; I've applied this patch to target-arm.next for 2.12,
with some minor tweaks to the commit message.

-- PMM