| Message ID | 20201022132445.25039-1-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | disas/capstone: Fix monitor disassembly of >32 bytes | expand |
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>
On Thu, 22 Oct 2020 at 14:52, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > 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: > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Thanks; applying to target-arm.next since nobody else has picked it up yet. -- PMM
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)) {
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(-)