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

2015-01-30 Thread Peter Maydell 
On 30 January 2015 at 12:36, Kirill Batuzov batuz...@ispras.ru 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

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

2015-01-30 Thread Kirill Batuzov 
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.

This patch addresses the issue by throwing illegal instruction exception
if LSB  MSB. Only ARM decoder is affected because Thumb decoder already
has this check in place.

To reproduce issue run the following program

int main(void) {
asm volatile (.long 0x07c00c12 :: );
return 0;
}

compiled with
  gcc -marm -static badop_arm.c -o badop_arm

Signed-off-by: Kirill Batuzov batuz...@ispras.ru
---
 target-arm/translate.c |2 ++
 1 file changed, 2 insertions(+)

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;
 i = i + 1 - shift;
 if (rm == 15) {
 tmp = tcg_temp_new_i32();
-- 
1.7.10.4