[PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)

Kevin Wolf posted 3 patches 3 years, 4 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20201113114326.97663-1-kwolf@redhat.com
Maintainers: Artyom Tarasenko <atar4qemu@gmail.com>, Laurent Vivier <laurent@vivier.eu>, Alistair Francis <Alistair.Francis@wdc.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Max Filippov <jcmvbkbc@gmail.com>, Markus Armbruster <armbru@redhat.com>, David Gibson <david@gibson.dropbear.id.au>, Eduardo Habkost <ehabkost@redhat.com>, Chris Wulff <crwulff@gmail.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>, Palmer Dabbelt <palmer@dabbelt.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Richard Henderson <rth@twiddle.net>, Marek Vasut <marex@denx.de>, Yoshinori Sato <ysato@users.sourceforge.jp>, Paolo Bonzini <pbonzini@redhat.com>
include/monitor/hmp-target.h |  7 ++++---
monitor/monitor-internal.h   |  2 +-
monitor/hmp.c                |  2 +-
monitor/misc.c               | 24 ++++++++++++------------
target/i386/monitor.c        | 11 ++++++-----
target/m68k/monitor.c        |  2 +-
target/nios2/monitor.c       |  2 +-
target/ppc/monitor.c         | 22 +++++++++++++---------
target/riscv/monitor.c       |  2 +-
target/sh4/monitor.c         |  2 +-
target/sparc/monitor.c       | 12 +++++++-----
target/xtensa/monitor.c      |  2 +-
12 files changed, 49 insertions(+), 41 deletions(-)
[PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)
Posted by Kevin Wolf 3 years, 4 months ago
When I restricted the section where the current monitor is set to only
the command handler, I missed that monitor_parse_arguments() can use it
indirectly, too, when evaluating register variables. These cases get
NULL now and crash (easy to reproduce with "x $pc").

This series passes the right monitor object down instead of using
monitor_cur(), which fixes the crash.

Kevin Wolf (3):
  hmp: Pass monitor to mon_get_cpu()
  hmp: Pass monitor to MonitorDef.get_value()
  hmp: Pass monitor to mon_get_cpu_env()

 include/monitor/hmp-target.h |  7 ++++---
 monitor/monitor-internal.h   |  2 +-
 monitor/hmp.c                |  2 +-
 monitor/misc.c               | 24 ++++++++++++------------
 target/i386/monitor.c        | 11 ++++++-----
 target/m68k/monitor.c        |  2 +-
 target/nios2/monitor.c       |  2 +-
 target/ppc/monitor.c         | 22 +++++++++++++---------
 target/riscv/monitor.c       |  2 +-
 target/sh4/monitor.c         |  2 +-
 target/sparc/monitor.c       | 12 +++++++-----
 target/xtensa/monitor.c      |  2 +-
 12 files changed, 49 insertions(+), 41 deletions(-)

-- 
2.28.0


Re: [PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)
Posted by Dr. David Alan Gilbert 3 years, 4 months ago
* Kevin Wolf (kwolf@redhat.com) wrote:
> When I restricted the section where the current monitor is set to only
> the command handler, I missed that monitor_parse_arguments() can use it
> indirectly, too, when evaluating register variables. These cases get
> NULL now and crash (easy to reproduce with "x $pc").
> 
> This series passes the right monitor object down instead of using
> monitor_cur(), which fixes the crash.

Why didn't the test-hmp.c find this?  It has a 'p $pc + 8'

Dave


> Kevin Wolf (3):
>   hmp: Pass monitor to mon_get_cpu()
>   hmp: Pass monitor to MonitorDef.get_value()
>   hmp: Pass monitor to mon_get_cpu_env()
> 
>  include/monitor/hmp-target.h |  7 ++++---
>  monitor/monitor-internal.h   |  2 +-
>  monitor/hmp.c                |  2 +-
>  monitor/misc.c               | 24 ++++++++++++------------
>  target/i386/monitor.c        | 11 ++++++-----
>  target/m68k/monitor.c        |  2 +-
>  target/nios2/monitor.c       |  2 +-
>  target/ppc/monitor.c         | 22 +++++++++++++---------
>  target/riscv/monitor.c       |  2 +-
>  target/sh4/monitor.c         |  2 +-
>  target/sparc/monitor.c       | 12 +++++++-----
>  target/xtensa/monitor.c      |  2 +-
>  12 files changed, 49 insertions(+), 41 deletions(-)
> 
> -- 
> 2.28.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


Re: [PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)
Posted by Kevin Wolf 3 years, 4 months ago
Am 13.11.2020 um 13:13 hat Dr. David Alan Gilbert geschrieben:
> * Kevin Wolf (kwolf@redhat.com) wrote:
> > When I restricted the section where the current monitor is set to only
> > the command handler, I missed that monitor_parse_arguments() can use it
> > indirectly, too, when evaluating register variables. These cases get
> > NULL now and crash (easy to reproduce with "x $pc").
> > 
> > This series passes the right monitor object down instead of using
> > monitor_cur(), which fixes the crash.
> 
> Why didn't the test-hmp.c find this?  It has a 'p $pc + 8'

Good question, a manual 'p $pc + 8' crashes for me on master.

Aha, it doesn't use a real HMP monitor, but QMP human-monitor-command.
Then it would just get the wrong monitor (the QMP one instead of the
temporary HMP monitor) and not NULL. The accessed CPU is even the same
because neither QMP nor the temporary HMP monitor have a current CPU
set, so even if the test case did check the result, it wouldn't catch
this.

Only if the test case were using multiple CPUs and cpu-index had been
set for human-monitor-command (to something other than the default), we
would get a wrong result. But of course, it still wouldn't crash.

Kevin


Re: [PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)
Posted by Dr. David Alan Gilbert 3 years, 4 months ago
* Kevin Wolf (kwolf@redhat.com) wrote:
> Am 13.11.2020 um 13:13 hat Dr. David Alan Gilbert geschrieben:
> > * Kevin Wolf (kwolf@redhat.com) wrote:
> > > When I restricted the section where the current monitor is set to only
> > > the command handler, I missed that monitor_parse_arguments() can use it
> > > indirectly, too, when evaluating register variables. These cases get
> > > NULL now and crash (easy to reproduce with "x $pc").
> > > 
> > > This series passes the right monitor object down instead of using
> > > monitor_cur(), which fixes the crash.
> > 
> > Why didn't the test-hmp.c find this?  It has a 'p $pc + 8'
> 
> Good question, a manual 'p $pc + 8' crashes for me on master.
> 
> Aha, it doesn't use a real HMP monitor, but QMP human-monitor-command.
> Then it would just get the wrong monitor (the QMP one instead of the
> temporary HMP monitor) and not NULL. The accessed CPU is even the same
> because neither QMP nor the temporary HMP monitor have a current CPU
> set, so even if the test case did check the result, it wouldn't catch
> this.
> 
> Only if the test case were using multiple CPUs and cpu-index had been
> set for human-monitor-command (to something other than the default), we
> would get a wrong result. But of course, it still wouldn't crash.

Ah, fair enough.

Dave

> Kevin
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


Re: [PATCH for-5.2 0/3] hmp: Fix arg evaluation crash (regression)
Posted by Dr. David Alan Gilbert 3 years, 4 months ago
* Kevin Wolf (kwolf@redhat.com) wrote:
> When I restricted the section where the current monitor is set to only
> the command handler, I missed that monitor_parse_arguments() can use it
> indirectly, too, when evaluating register variables. These cases get
> NULL now and crash (easy to reproduce with "x $pc").
> 
> This series passes the right monitor object down instead of using
> monitor_cur(), which fixes the crash.
> 

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

and queued; I'll send out an HMP pull shortly.

> Kevin Wolf (3):
>   hmp: Pass monitor to mon_get_cpu()
>   hmp: Pass monitor to MonitorDef.get_value()
>   hmp: Pass monitor to mon_get_cpu_env()
> 
>  include/monitor/hmp-target.h |  7 ++++---
>  monitor/monitor-internal.h   |  2 +-
>  monitor/hmp.c                |  2 +-
>  monitor/misc.c               | 24 ++++++++++++------------
>  target/i386/monitor.c        | 11 ++++++-----
>  target/m68k/monitor.c        |  2 +-
>  target/nios2/monitor.c       |  2 +-
>  target/ppc/monitor.c         | 22 +++++++++++++---------
>  target/riscv/monitor.c       |  2 +-
>  target/sh4/monitor.c         |  2 +-
>  target/sparc/monitor.c       | 12 +++++++-----
>  target/xtensa/monitor.c      |  2 +-
>  12 files changed, 49 insertions(+), 41 deletions(-)
> 
> -- 
> 2.28.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK