[Qemu-devel] [PATCH] arm: Ensure LSB of BLX is set

[Qemu-devel] [PATCH] arm: Ensure LSB of BLX is set

[meadori]

From: Meador Inge <meadori@codesourcery.com>

This small patch adds a sanity check when disassembling
the BLX instruction.  The use case came to light when
doing toolchain development and a similar check was
upstreamed for Binutils:

  * https://sourceware.org/ml/binutils/2011-01/msg00077.html

Patch by Nathan Sidwell.

Signed-off-by: Meador Inge <meadori@codesourcery.com>
---
 target-arm/translate.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 69ac18c..fedc8f3 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -9912,6 +9912,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                     gen_jmp(s, offset);
                 } else {
                     /* blx */
+                    /* The instruction must have bit zero unset, even
+                       though it is part of the offset.  Real hardware
+                       will abort, so we do too.  */
+                    if (insn & 1) {
+                        goto illegal_op;
+                    }
                     offset &= ~(uint32_t)2;
                     /* thumb2 bx, no need to check */
                     gen_bx_im(s, offset);
-- 
1.8.1.1

Re: [Qemu-devel] [PATCH] arm: Ensure LSB of BLX is set

[Peter Maydell]

On 6 July 2015 at 19:09,  <meadori@codesourcery.com> wrote:
> From: Meador Inge <meadori@codesourcery.com>
>
> This small patch adds a sanity check when disassembling
> the BLX instruction.  The use case came to light when
> doing toolchain development and a similar check was
> upstreamed for Binutils:
>
>   * https://sourceware.org/ml/binutils/2011-01/msg00077.html
>
> Patch by Nathan Sidwell.
>
> Signed-off-by: Meador Inge <meadori@codesourcery.com>
> ---
>  target-arm/translate.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 69ac18c..fedc8f3 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -9912,6 +9912,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>                      gen_jmp(s, offset);
>                  } else {
>                      /* blx */
> +                    /* The instruction must have bit zero unset, even
> +                       though it is part of the offset.  Real hardware
> +                       will abort, so we do too.  */

This comment isn't really correct -- bit zero in this encoding
(BLX T2) isn't part of the immediate, it's described
as a one bit field H which causes UNDEFINED if it's 1.
This is architecturally mandated, not just somewhere
we're following h/w on an IMPDEF or UNPREDICTABLE case.

> +                    if (insn & 1) {
> +                        goto illegal_op;
> +                    }

This check is happening too late -- we've already
done the write to R14. We need to UNDEF before that.

>                      offset &= ~(uint32_t)2;

The new check makes this masking unnecessary now, right?

>                      /* thumb2 bx, no need to check */
>                      gen_bx_im(s, offset);
> --

thanks
-- PMM

Re: [Qemu-devel] [PATCH] arm: Ensure LSB of BLX is set

[Peter Maydell]

On 6 July 2015 at 19:09,  <meadori@codesourcery.com> wrote:
> From: Meador Inge <meadori@codesourcery.com>
>
> This small patch adds a sanity check when disassembling
> the BLX instruction.  The use case came to light when
> doing toolchain development and a similar check was
> upstreamed for Binutils:
>
>   * https://sourceware.org/ml/binutils/2011-01/msg00077.html
>
> Patch by Nathan Sidwell.
>
> Signed-off-by: Meador Inge <meadori@codesourcery.com>

Hi; are you planning to send a v2 of this patch?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] arm: Ensure LSB of BLX is set

[Meador Inge]

On Tue, Sep 01, 2015 at 05:28:10PM +0100, Peter Maydell wrote:
> On 6 July 2015 at 19:09,  <meadori@codesourcery.com> wrote:
> > From: Meador Inge <meadori@codesourcery.com>
> >
> > This small patch adds a sanity check when disassembling
> > the BLX instruction.  The use case came to light when
> > doing toolchain development and a similar check was
> > upstreamed for Binutils:
> >
> >   * https://sourceware.org/ml/binutils/2011-01/msg00077.html
> >
> > Patch by Nathan Sidwell.
> >
> > Signed-off-by: Meador Inge <meadori@codesourcery.com>
> 
> Hi; are you planning to send a v2 of this patch?

I am.  Thank you for the reminder.  I will send a v2 some time tomorrow.

Thanks,

-- Meador