Re: [PATCH] disas/capstone: Fix monitor disassembly of >32 bytes

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 10/22/20 3:24 PM, Peter Maydell wrote:
> If we're using the capstone disassembler, disassembly of a run of
> instructions more than 32 bytes long disassembles the wrong data for
> instructions beyond the 32 byte mark:
> 
> (qemu) xp /16x 0x100
> 0000000000000100: 0x00000005 0x54410001 0x00000001 0x00001000
> 0000000000000110: 0x00000000 0x00000004 0x54410002 0x3c000000
> 0000000000000120: 0x00000000 0x00000004 0x54410009 0x74736574
> 0000000000000130: 0x00000000 0x00000000 0x00000000 0x00000000
> (qemu) xp /16i 0x100
> 0x00000100: 00000005 andeq r0, r0, r5
> 0x00000104: 54410001 strbpl r0, [r1], #-1
> 0x00000108: 00000001 andeq r0, r0, r1
> 0x0000010c: 00001000 andeq r1, r0, r0
> 0x00000110: 00000000 andeq r0, r0, r0
> 0x00000114: 00000004 andeq r0, r0, r4
> 0x00000118: 54410002 strbpl r0, [r1], #-2
> 0x0000011c: 3c000000 .byte 0x00, 0x00, 0x00, 0x3c
> 0x00000120: 54410001 strbpl r0, [r1], #-1
> 0x00000124: 00000001 andeq r0, r0, r1
> 0x00000128: 00001000 andeq r1, r0, r0
> 0x0000012c: 00000000 andeq r0, r0, r0
> 0x00000130: 00000004 andeq r0, r0, r4
> 0x00000134: 54410002 strbpl r0, [r1], #-2
> 0x00000138: 3c000000 .byte 0x00, 0x00, 0x00, 0x3c
> 0x0000013c: 00000000 andeq r0, r0, r0
> 
> Here the disassembly of 0x120..0x13f is using the data that is in
> 0x104..0x123.
> 
> This is caused by passing the wrong value to the read_memory_func().
> The intention is that at this point in the loop the 'cap_buf' buffer
> already contains 'csize' bytes of data for the instruction at guest
> addr 'pc', and we want to read in an extra 'tsize' bytes.  Those
> extra bytes are therefore at 'pc + csize', not 'pc'.  On the first
> time through the loop 'csize' happens to be zero, so the initial read
> of 32 bytes into cap_buf is correct and as long as the disassembly
> never needs to read more data we return the correct information.
> 
> Use the correct guest address in the call to read_memory_func().
> 
> Cc: qemu-stable@nongnu.org
> Fixes: https://bugs.launchpad.net/qemu/+bug/1900779
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Note for qemu-stable: in 5.1 this function was in disas.c so the
> patch won't literally apply to it, but the same change in that
> file should be correct.
> ---
>   disas/capstone.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/disas/capstone.c b/disas/capstone.c
> index 0a9ef9c8927..7462c0e3053 100644
> --- a/disas/capstone.c
> +++ b/disas/capstone.c
> @@ -286,7 +286,7 @@ bool cap_disas_monitor(disassemble_info *info, uint64_t pc, int count)
>   
>           /* Make certain that we can make progress.  */
>           assert(tsize != 0);
> -        info->read_memory_func(pc, cap_buf + csize, tsize, info);
> +        info->read_memory_func(pc + csize, cap_buf + csize, tsize, info);
>           csize += tsize;
>   
>           if (cs_disasm_iter(handle, &cbuf, &csize, &pc, insn)) {
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>