diff mbox series

[for-2.9,4/6] disas/microblaze: Avoid unintended sign extension

Message ID 1488556233-31246-5-git-send-email-peter.maydell@linaro.org
State Accepted
Commit 1d153a3388b150b8aeedde32242db86b79c45473
Headers show
Series disas: Fix various coverity nits | expand

Commit Message

Peter Maydell March 3, 2017, 3:50 p.m. UTC
In read_insn_microblaze() we assemble 4 bytes into an 'unsigned
long'.  If 'unsigned long' is 64 bits and the high byte has its top
bit set, then C's implicit conversion from 'unsigned char' to 'int'
for the shift will result in an unintended sign extension which sets
the top 32 bits in 'inst'.  Add casts to prevent this.  (Spotted by
Coverity, CID 1005401.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 disas/microblaze.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.7.4

Comments

Edgar E. Iglesias March 3, 2017, 3:58 p.m. UTC | #1
On Fri, Mar 03, 2017 at 03:50:31PM +0000, Peter Maydell wrote:
> In read_insn_microblaze() we assemble 4 bytes into an 'unsigned

> long'.  If 'unsigned long' is 64 bits and the high byte has its top

> bit set, then C's implicit conversion from 'unsigned char' to 'int'

> for the shift will result in an unintended sign extension which sets

> the top 32 bits in 'inst'.  Add casts to prevent this.  (Spotted by

> Coverity, CID 1005401.)


I'm OK with this but perhaps it would have been more readable to
change inst to uint32_t ? (All microblaze insns are 32bit).

Anyway:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


Thanks,
Edgar


> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  disas/microblaze.c | 6 ++++--

>  1 file changed, 4 insertions(+), 2 deletions(-)

> 

> diff --git a/disas/microblaze.c b/disas/microblaze.c

> index 91b30ac..407c0a3 100644

> --- a/disas/microblaze.c

> +++ b/disas/microblaze.c

> @@ -748,9 +748,11 @@ read_insn_microblaze (bfd_vma memaddr,

>      }

>  

>    if (info->endian == BFD_ENDIAN_BIG)

> -    inst = (ibytes[0] << 24) | (ibytes[1] << 16) | (ibytes[2] << 8) | ibytes[3];

> +    inst = ((unsigned)ibytes[0] << 24) | (ibytes[1] << 16)

> +      | (ibytes[2] << 8) | ibytes[3];

>    else if (info->endian == BFD_ENDIAN_LITTLE)

> -    inst = (ibytes[3] << 24) | (ibytes[2] << 16) | (ibytes[1] << 8) | ibytes[0];

> +    inst = ((unsigned)ibytes[3] << 24) | (ibytes[2] << 16)

> +      | (ibytes[1] << 8) | ibytes[0];

>    else

>      abort ();

>  

> -- 

> 2.7.4

>
Peter Maydell March 3, 2017, 4:02 p.m. UTC | #2
On 3 March 2017 at 15:58, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Fri, Mar 03, 2017 at 03:50:31PM +0000, Peter Maydell wrote:

>> In read_insn_microblaze() we assemble 4 bytes into an 'unsigned

>> long'.  If 'unsigned long' is 64 bits and the high byte has its top

>> bit set, then C's implicit conversion from 'unsigned char' to 'int'

>> for the shift will result in an unintended sign extension which sets

>> the top 32 bits in 'inst'.  Add casts to prevent this.  (Spotted by

>> Coverity, CID 1005401.)

>

> I'm OK with this but perhaps it would have been more readable to

> change inst to uint32_t ? (All microblaze insns are 32bit).


I thought about that, but it was more invasive a change than
I was happy with: you'd want to change not just 'inst' but
also the return type of read_insn_microblaze, the opcode_mask
field of struct op_code_struct, 'inst' and 'prev_inst' in
print_insn_microblaze, the type of the instr arguments to
get_field, get_field_imm, etc...

thanks
-- PMM
Edgar E. Iglesias March 3, 2017, 4:06 p.m. UTC | #3
On Fri, Mar 03, 2017 at 04:02:07PM +0000, Peter Maydell wrote:
> On 3 March 2017 at 15:58, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:

> > On Fri, Mar 03, 2017 at 03:50:31PM +0000, Peter Maydell wrote:

> >> In read_insn_microblaze() we assemble 4 bytes into an 'unsigned

> >> long'.  If 'unsigned long' is 64 bits and the high byte has its top

> >> bit set, then C's implicit conversion from 'unsigned char' to 'int'

> >> for the shift will result in an unintended sign extension which sets

> >> the top 32 bits in 'inst'.  Add casts to prevent this.  (Spotted by

> >> Coverity, CID 1005401.)

> >

> > I'm OK with this but perhaps it would have been more readable to

> > change inst to uint32_t ? (All microblaze insns are 32bit).

> 

> I thought about that, but it was more invasive a change than

> I was happy with: you'd want to change not just 'inst' but

> also the return type of read_insn_microblaze, the opcode_mask

> field of struct op_code_struct, 'inst' and 'prev_inst' in

> print_insn_microblaze, the type of the instr arguments to

> get_field, get_field_imm, etc...


Ah, I see, didn't think about that...

Cheers,
Edgar
diff mbox series

Patch

diff --git a/disas/microblaze.c b/disas/microblaze.c
index 91b30ac..407c0a3 100644
--- a/disas/microblaze.c
+++ b/disas/microblaze.c
@@ -748,9 +748,11 @@  read_insn_microblaze (bfd_vma memaddr,
     }
 
   if (info->endian == BFD_ENDIAN_BIG)
-    inst = (ibytes[0] << 24) | (ibytes[1] << 16) | (ibytes[2] << 8) | ibytes[3];
+    inst = ((unsigned)ibytes[0] << 24) | (ibytes[1] << 16)
+      | (ibytes[2] << 8) | ibytes[3];
   else if (info->endian == BFD_ENDIAN_LITTLE)
-    inst = (ibytes[3] << 24) | (ibytes[2] << 16) | (ibytes[1] << 8) | ibytes[0];
+    inst = ((unsigned)ibytes[3] << 24) | (ibytes[2] << 16)
+      | (ibytes[1] << 8) | ibytes[0];
   else
     abort ();