| Message ID | 1436206141-13361-1-git-send-email-meadori@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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);