| Message ID | 1521777509-22896-1-git-send-email-onursahin08@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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;
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(-)