All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] target-arm: check that LSB <= MSB in BFI instruction
@ 2015-01-30 12:36 Kirill Batuzov
  2015-01-30 12:44 ` Peter Maydell
  0 siblings, 1 reply; 4+ messages in thread
From: Kirill Batuzov @ 2015-01-30 12:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell, 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 <batuzovk@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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] [PATCH] target-arm: check that LSB <= MSB in BFI instruction
  2015-01-30 12:36 [Qemu-devel] [PATCH] target-arm: check that LSB <= MSB in BFI instruction Kirill Batuzov
@ 2015-01-30 12:44 ` Peter Maydell
  2015-01-30 12:59   ` [Qemu-devel] [PATCH v2] " Kirill Batuzov
  0 siblings, 1 reply; 4+ messages in thread
From: Peter Maydell @ 2015-01-30 12:44 UTC (permalink / raw)
  To: Kirill Batuzov; +Cc: QEMU Developers

On 30 January 2015 at 12:36, Kirill Batuzov <batuzovk@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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [Qemu-devel] [PATCH v2] target-arm: check that LSB <= MSB in BFI instruction
  2015-01-30 12:44 ` Peter Maydell
@ 2015-01-30 12:59   ` Kirill Batuzov
  2015-02-03 11:47     ` Peter Maydell
  0 siblings, 1 reply; 4+ messages in thread
From: Kirill Batuzov @ 2015-01-30 12:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell, 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 <batuzovk@ispras.ru>
---
 target-arm/translate.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index bdfcdf1..2c1c2a7 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8739,6 +8739,10 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
                         ARCH(6T2);
                         shift = (insn >> 7) & 0x1f;
                         i = (insn >> 16) & 0x1f;
+                        if (i < shift) {
+                            /* UNPREDICTABLE; we choose to UNDEF */
+                            goto illegal_op;
+                        }
                         i = i + 1 - shift;
                         if (rm == 15) {
                             tmp = tcg_temp_new_i32();
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] [PATCH v2] target-arm: check that LSB <= MSB in BFI instruction
  2015-01-30 12:59   ` [Qemu-devel] [PATCH v2] " Kirill Batuzov
@ 2015-02-03 11:47     ` Peter Maydell
  0 siblings, 0 replies; 4+ messages in thread
From: Peter Maydell @ 2015-02-03 11:47 UTC (permalink / raw)
  To: Kirill Batuzov; +Cc: QEMU Developers

On 30 January 2015 at 12:59, Kirill Batuzov <batuzovk@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.
>
> 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 <batuzovk@ispras.ru>



Applied to target-arm.next, thanks.

-- PMM

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-02-03 11:48 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-30 12:36 [Qemu-devel] [PATCH] target-arm: check that LSB <= MSB in BFI instruction Kirill Batuzov
2015-01-30 12:44 ` Peter Maydell
2015-01-30 12:59   ` [Qemu-devel] [PATCH v2] " Kirill Batuzov
2015-02-03 11:47     ` Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.