* [PATCH 0/2] gdbstub: don't crash if no gdb attached and gdb syscall attempted @ 2022-05-26 19:00 Peter Maydell 2022-05-26 19:00 ` [PATCH 1/2] gdbstub: Don't use GDB syscalls if no GDB is attached Peter Maydell ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Peter Maydell @ 2022-05-26 19:00 UTC (permalink / raw) To: qemu-devel Cc: Alex Bennée, Richard Henderson, Philippe Mathieu-Daudé, Liviu Ionescu This patchset fixes a couple of bugs reported by Liviu Ionescu related to semihosting syscall handling. Firstly, if the user sets up the gdbstub but doesn't connect a gdb (ie passes '-s' but not '-S'), and we try to use gdb for semihosting syscalls (either because the user explicitly set target=gdb or else via autodetection) then we currently segfault. This patch adjusts the behaviour so that "gdbstub initialized but no gdb attached to a vCPU" is treated the same way we already treat "gdbstub not initialized". Secondly, we weren't handling commandlines where --semihosting-config appears multiple times: these ought to be merged together but instead we were effectively ignoring all but the last one. thanks -- PMM Peter Maydell (2): gdbstub: Don't use GDB syscalls if no GDB is attached semihosting/config: Merge --semihosting-config option groups gdbstub.c | 14 +++++++++++--- semihosting/config.c | 1 + 2 files changed, 12 insertions(+), 3 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] gdbstub: Don't use GDB syscalls if no GDB is attached 2022-05-26 19:00 [PATCH 0/2] gdbstub: don't crash if no gdb attached and gdb syscall attempted Peter Maydell @ 2022-05-26 19:00 ` Peter Maydell 2022-05-29 11:06 ` Philippe Mathieu-Daudé via 2022-06-09 20:01 ` Luc Michel 2022-05-26 19:00 ` [PATCH 2/2] semihosting/config: Merge --semihosting-config option groups Peter Maydell 2022-06-10 13:04 ` [PATCH 0/2] gdbstub: don't crash if no gdb attached and gdb syscall attempted Peter Maydell 2 siblings, 2 replies; 8+ messages in thread From: Peter Maydell @ 2022-05-26 19:00 UTC (permalink / raw) To: qemu-devel Cc: Alex Bennée, Richard Henderson, Philippe Mathieu-Daudé, Liviu Ionescu In two places in gdbstub.c we look at gdbserver_state.init to decide whether we're going to do a semihosting syscall via the gdb remote protocol: * when setting up, if the user didn't explicitly select either native semihosting or gdb semihosting, we autoselect, with the intended behaviour "use gdb if gdb is connected" * when the semihosting layer attempts to do a syscall via gdb, we silently ignore it if the gdbstub wasn't actually set up However, if the user's commandline sets up the gdbstub but tells QEMU to start rather than waiting for a GDB to connect (eg using '-s' but not '-S'), then we will have gdbserver_state.init true but no actual connection; an attempt to use gdb syscalls will then crash because we try to use gdbserver_state.c_cpu when it hasn't been set up: #0 0x00007ffff6803ba8 in qemu_cpu_kick (cpu=0x0) at ../../softmmu/cpus.c:457 #1 0x00007ffff6c03913 in gdb_do_syscallv (cb=0x7ffff6c19944 <common_semi_cb>, fmt=0x7ffff7573b7e "", va=0x7ffff56294c0) at ../../gdbstub.c:2946 #2 0x00007ffff6c19c3a in common_semi_gdb_syscall (cs=0x7ffff83fe060, cb=0x7ffff6c19944 <common_semi_cb>, fmt=0x7ffff7573b75 "isatty,%x") at ../../semihosting/arm-compat-semi.c:494 #3 0x00007ffff6c1a064 in gdb_isattyfn (cs=0x7ffff83fe060, gf=0x7ffff86a3690) at ../../semihosting/arm-compat-semi.c:636 #4 0x00007ffff6c1b20f in do_common_semihosting (cs=0x7ffff83fe060) at ../../semihosting/arm-compat-semi.c:967 #5 0x00007ffff693a037 in handle_semihosting (cs=0x7ffff83fe060) at ../../target/arm/helper.c:10316 You can probably also get into this state via some odd corner cases involving connecting a GDB and then telling it to detach from all the vCPUs. Abstract out the test into a new gdb_attached() function which returns true only if there's actually a GDB connected to the debug stub and attached to at least one vCPU. Reported-by: Liviu Ionescu <ilg@livius.net> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Silently doing nothing in gdb_do_syscallv(), never calling the callback function, is kind of dodgy. But it's what the code is doing already, and besides it's not clear what we should do if the user specifically says "semihosting calls via the gdb stub" and then doesn't connect gdb... --- gdbstub.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/gdbstub.c b/gdbstub.c index a3ff8702cef..88a34c8f522 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -443,6 +443,15 @@ static int get_char(void) } #endif +/* + * Return true if there is a GDB currently connected to the stub + * and attached to a CPU + */ +static bool gdb_attached(void) +{ + return gdbserver_state.init && gdbserver_state.c_cpu; +} + static enum { GDB_SYS_UNKNOWN, GDB_SYS_ENABLED, @@ -464,8 +473,7 @@ int use_gdb_syscalls(void) /* -semihosting-config target=auto */ /* On the first call check if gdb is connected and remember. */ if (gdb_syscall_mode == GDB_SYS_UNKNOWN) { - gdb_syscall_mode = gdbserver_state.init ? - GDB_SYS_ENABLED : GDB_SYS_DISABLED; + gdb_syscall_mode = gdb_attached() ? GDB_SYS_ENABLED : GDB_SYS_DISABLED; } return gdb_syscall_mode == GDB_SYS_ENABLED; } @@ -2886,7 +2894,7 @@ void gdb_do_syscallv(gdb_syscall_complete_cb cb, const char *fmt, va_list va) target_ulong addr; uint64_t i64; - if (!gdbserver_state.init) { + if (!gdb_attached()) { return; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] gdbstub: Don't use GDB syscalls if no GDB is attached 2022-05-26 19:00 ` [PATCH 1/2] gdbstub: Don't use GDB syscalls if no GDB is attached Peter Maydell @ 2022-05-29 11:06 ` Philippe Mathieu-Daudé via 2022-06-09 20:01 ` Luc Michel 1 sibling, 0 replies; 8+ messages in thread From: Philippe Mathieu-Daudé via @ 2022-05-29 11:06 UTC (permalink / raw) To: Peter Maydell, qemu-devel Cc: Alex Bennée, Richard Henderson, Liviu Ionescu On 26/5/22 21:00, Peter Maydell wrote: > In two places in gdbstub.c we look at gdbserver_state.init to decide > whether we're going to do a semihosting syscall via the gdb remote > protocol: > * when setting up, if the user didn't explicitly select either > native semihosting or gdb semihosting, we autoselect, with the > intended behaviour "use gdb if gdb is connected" > * when the semihosting layer attempts to do a syscall via gdb, we > silently ignore it if the gdbstub wasn't actually set up > > However, if the user's commandline sets up the gdbstub but tells QEMU > to start rather than waiting for a GDB to connect (eg using '-s' but > not '-S'), then we will have gdbserver_state.init true but no actual > connection; an attempt to use gdb syscalls will then crash because we > try to use gdbserver_state.c_cpu when it hasn't been set up: > > #0 0x00007ffff6803ba8 in qemu_cpu_kick (cpu=0x0) at ../../softmmu/cpus.c:457 > #1 0x00007ffff6c03913 in gdb_do_syscallv (cb=0x7ffff6c19944 <common_semi_cb>, > fmt=0x7ffff7573b7e "", va=0x7ffff56294c0) at ../../gdbstub.c:2946 > #2 0x00007ffff6c19c3a in common_semi_gdb_syscall (cs=0x7ffff83fe060, > cb=0x7ffff6c19944 <common_semi_cb>, fmt=0x7ffff7573b75 "isatty,%x") > at ../../semihosting/arm-compat-semi.c:494 > #3 0x00007ffff6c1a064 in gdb_isattyfn (cs=0x7ffff83fe060, gf=0x7ffff86a3690) > at ../../semihosting/arm-compat-semi.c:636 > #4 0x00007ffff6c1b20f in do_common_semihosting (cs=0x7ffff83fe060) > at ../../semihosting/arm-compat-semi.c:967 > #5 0x00007ffff693a037 in handle_semihosting (cs=0x7ffff83fe060) > at ../../target/arm/helper.c:10316 > > You can probably also get into this state via some odd > corner cases involving connecting a GDB and then telling it > to detach from all the vCPUs. > > Abstract out the test into a new gdb_attached() function > which returns true only if there's actually a GDB connected > to the debug stub and attached to at least one vCPU. > > Reported-by: Liviu Ionescu <ilg@livius.net> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Silently doing nothing in gdb_do_syscallv(), never calling the > callback function, is kind of dodgy. But it's what the code is doing > already, and besides it's not clear what we should do if the user > specifically says "semihosting calls via the gdb stub" and then > doesn't connect gdb... When using a JTAG probe as GDB monitor, you get an error the probe can't get initialized. If the probe gets disconnected while running then the semihosting calls are executed but don't do anything (or return default errno). Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] gdbstub: Don't use GDB syscalls if no GDB is attached 2022-05-26 19:00 ` [PATCH 1/2] gdbstub: Don't use GDB syscalls if no GDB is attached Peter Maydell 2022-05-29 11:06 ` Philippe Mathieu-Daudé via @ 2022-06-09 20:01 ` Luc Michel 1 sibling, 0 replies; 8+ messages in thread From: Luc Michel @ 2022-06-09 20:01 UTC (permalink / raw) To: Peter Maydell Cc: qemu-devel, Alex Bennée, Richard Henderson, Philippe Mathieu-Daudé, Liviu Ionescu On 20:00 Thu 26 May , Peter Maydell wrote: > In two places in gdbstub.c we look at gdbserver_state.init to decide > whether we're going to do a semihosting syscall via the gdb remote > protocol: > * when setting up, if the user didn't explicitly select either > native semihosting or gdb semihosting, we autoselect, with the > intended behaviour "use gdb if gdb is connected" > * when the semihosting layer attempts to do a syscall via gdb, we > silently ignore it if the gdbstub wasn't actually set up > > However, if the user's commandline sets up the gdbstub but tells QEMU > to start rather than waiting for a GDB to connect (eg using '-s' but > not '-S'), then we will have gdbserver_state.init true but no actual > connection; an attempt to use gdb syscalls will then crash because we > try to use gdbserver_state.c_cpu when it hasn't been set up: > > #0 0x00007ffff6803ba8 in qemu_cpu_kick (cpu=0x0) at ../../softmmu/cpus.c:457 > #1 0x00007ffff6c03913 in gdb_do_syscallv (cb=0x7ffff6c19944 <common_semi_cb>, > fmt=0x7ffff7573b7e "", va=0x7ffff56294c0) at ../../gdbstub.c:2946 > #2 0x00007ffff6c19c3a in common_semi_gdb_syscall (cs=0x7ffff83fe060, > cb=0x7ffff6c19944 <common_semi_cb>, fmt=0x7ffff7573b75 "isatty,%x") > at ../../semihosting/arm-compat-semi.c:494 > #3 0x00007ffff6c1a064 in gdb_isattyfn (cs=0x7ffff83fe060, gf=0x7ffff86a3690) > at ../../semihosting/arm-compat-semi.c:636 > #4 0x00007ffff6c1b20f in do_common_semihosting (cs=0x7ffff83fe060) > at ../../semihosting/arm-compat-semi.c:967 > #5 0x00007ffff693a037 in handle_semihosting (cs=0x7ffff83fe060) > at ../../target/arm/helper.c:10316 > > You can probably also get into this state via some odd > corner cases involving connecting a GDB and then telling it > to detach from all the vCPUs. > > Abstract out the test into a new gdb_attached() function > which returns true only if there's actually a GDB connected > to the debug stub and attached to at least one vCPU. > > Reported-by: Liviu Ionescu <ilg@livius.net> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Luc Michel <luc@lmichel.fr> > --- > Silently doing nothing in gdb_do_syscallv(), never calling the > callback function, is kind of dodgy. But it's what the code is doing > already, and besides it's not clear what we should do if the user > specifically says "semihosting calls via the gdb stub" and then > doesn't connect gdb... > --- > gdbstub.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index a3ff8702cef..88a34c8f522 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -443,6 +443,15 @@ static int get_char(void) > } > #endif > > +/* > + * Return true if there is a GDB currently connected to the stub > + * and attached to a CPU > + */ > +static bool gdb_attached(void) > +{ > + return gdbserver_state.init && gdbserver_state.c_cpu; > +} > + > static enum { > GDB_SYS_UNKNOWN, > GDB_SYS_ENABLED, > @@ -464,8 +473,7 @@ int use_gdb_syscalls(void) > /* -semihosting-config target=auto */ > /* On the first call check if gdb is connected and remember. */ > if (gdb_syscall_mode == GDB_SYS_UNKNOWN) { > - gdb_syscall_mode = gdbserver_state.init ? > - GDB_SYS_ENABLED : GDB_SYS_DISABLED; > + gdb_syscall_mode = gdb_attached() ? GDB_SYS_ENABLED : GDB_SYS_DISABLED; > } > return gdb_syscall_mode == GDB_SYS_ENABLED; > } > @@ -2886,7 +2894,7 @@ void gdb_do_syscallv(gdb_syscall_complete_cb cb, const char *fmt, va_list va) > target_ulong addr; > uint64_t i64; > > - if (!gdbserver_state.init) { > + if (!gdb_attached()) { > return; > } > > -- > 2.25.1 > > -- +---------------------+--------------------------+ | Luc MICHEL | TIMA Lab / SLS Team | | | Phone: +33 4 76 57 43 34 | +---------------------+--------------------------+ ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] semihosting/config: Merge --semihosting-config option groups 2022-05-26 19:00 [PATCH 0/2] gdbstub: don't crash if no gdb attached and gdb syscall attempted Peter Maydell 2022-05-26 19:00 ` [PATCH 1/2] gdbstub: Don't use GDB syscalls if no GDB is attached Peter Maydell @ 2022-05-26 19:00 ` Peter Maydell 2022-06-09 20:02 ` Luc Michel 2022-06-10 13:04 ` [PATCH 0/2] gdbstub: don't crash if no gdb attached and gdb syscall attempted Peter Maydell 2 siblings, 1 reply; 8+ messages in thread From: Peter Maydell @ 2022-05-26 19:00 UTC (permalink / raw) To: qemu-devel Cc: Alex Bennée, Richard Henderson, Philippe Mathieu-Daudé, Liviu Ionescu Currently we mishandle the --semihosting-config option if the user specifies it on the command line more than once. For example with: --semihosting-config target=gdb --semihosting-config arg=foo,arg=bar the function qemu_semihosting_config_options() is called twice, once for each argument. But that function expects to be called only once, and it always unconditionally sets the semihosting.enabled, semihost_chardev and semihosting.target variables. This means that if any of those options were set anywhere except the last --semihosting-config option on the command line, those settings are ignored. In the example above, 'target=gdb' in the first option is overridden by an implied default 'target=auto' in the second. The QemuOptsList machinery has a flag for handling this kind of "option group is setting global state": by setting .merge_lists = true; we make the machinery merge all the --semihosting-config arguments the user passes into a single set of options and call our qemu_semihosting_config_options() just once. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- semihosting/config.c | 1 + 1 file changed, 1 insertion(+) diff --git a/semihosting/config.c b/semihosting/config.c index 50d82108e6e..3afacf54ab2 100644 --- a/semihosting/config.c +++ b/semihosting/config.c @@ -27,6 +27,7 @@ QemuOptsList qemu_semihosting_config_opts = { .name = "semihosting-config", + .merge_lists = true, .implied_opt_name = "enable", .head = QTAILQ_HEAD_INITIALIZER(qemu_semihosting_config_opts.head), .desc = { -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] semihosting/config: Merge --semihosting-config option groups 2022-05-26 19:00 ` [PATCH 2/2] semihosting/config: Merge --semihosting-config option groups Peter Maydell @ 2022-06-09 20:02 ` Luc Michel 0 siblings, 0 replies; 8+ messages in thread From: Luc Michel @ 2022-06-09 20:02 UTC (permalink / raw) To: Peter Maydell Cc: qemu-devel, Alex Bennée, Richard Henderson, Philippe Mathieu-Daudé, Liviu Ionescu On 20:00 Thu 26 May , Peter Maydell wrote: > Currently we mishandle the --semihosting-config option if the > user specifies it on the command line more than once. For > example with: > --semihosting-config target=gdb --semihosting-config arg=foo,arg=bar > > the function qemu_semihosting_config_options() is called twice, once > for each argument. But that function expects to be called only once, > and it always unconditionally sets the semihosting.enabled, > semihost_chardev and semihosting.target variables. This means that > if any of those options were set anywhere except the last > --semihosting-config option on the command line, those settings are > ignored. In the example above, 'target=gdb' in the first option is > overridden by an implied default 'target=auto' in the second. > > The QemuOptsList machinery has a flag for handling this kind of > "option group is setting global state": by setting > .merge_lists = true; > we make the machinery merge all the --semihosting-config arguments > the user passes into a single set of options and call our > qemu_semihosting_config_options() just once. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Luc Michel <luc@lmichel.fr> > --- > semihosting/config.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/semihosting/config.c b/semihosting/config.c > index 50d82108e6e..3afacf54ab2 100644 > --- a/semihosting/config.c > +++ b/semihosting/config.c > @@ -27,6 +27,7 @@ > > QemuOptsList qemu_semihosting_config_opts = { > .name = "semihosting-config", > + .merge_lists = true, > .implied_opt_name = "enable", > .head = QTAILQ_HEAD_INITIALIZER(qemu_semihosting_config_opts.head), > .desc = { > -- > 2.25.1 > > -- ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] gdbstub: don't crash if no gdb attached and gdb syscall attempted 2022-05-26 19:00 [PATCH 0/2] gdbstub: don't crash if no gdb attached and gdb syscall attempted Peter Maydell 2022-05-26 19:00 ` [PATCH 1/2] gdbstub: Don't use GDB syscalls if no GDB is attached Peter Maydell 2022-05-26 19:00 ` [PATCH 2/2] semihosting/config: Merge --semihosting-config option groups Peter Maydell @ 2022-06-10 13:04 ` Peter Maydell 2022-06-10 14:40 ` Alex Bennée 2 siblings, 1 reply; 8+ messages in thread From: Peter Maydell @ 2022-06-10 13:04 UTC (permalink / raw) To: qemu-devel Cc: Alex Bennée, Richard Henderson, Philippe Mathieu-Daudé, Liviu Ionescu On Thu, 26 May 2022 at 20:00, Peter Maydell <peter.maydell@linaro.org> wrote: > > This patchset fixes a couple of bugs reported by > Liviu Ionescu related to semihosting syscall handling. > > Firstly, if the user sets up the gdbstub but doesn't connect a gdb > (ie passes '-s' but not '-S'), and we try to use gdb for semihosting > syscalls (either because the user explicitly set target=gdb or else > via autodetection) then we currently segfault. This patch adjusts > the behaviour so that "gdbstub initialized but no gdb attached to a > vCPU" is treated the same way we already treat "gdbstub not > initialized". > > Secondly, we weren't handling commandlines where > --semihosting-config appears multiple times: these ought to be > merged together but instead we were effectively ignoring > all but the last one. > > thanks > -- PMM > > Peter Maydell (2): > gdbstub: Don't use GDB syscalls if no GDB is attached > semihosting/config: Merge --semihosting-config option groups I'll take these via target-arm.next. thanks -- PMM ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] gdbstub: don't crash if no gdb attached and gdb syscall attempted 2022-06-10 13:04 ` [PATCH 0/2] gdbstub: don't crash if no gdb attached and gdb syscall attempted Peter Maydell @ 2022-06-10 14:40 ` Alex Bennée 0 siblings, 0 replies; 8+ messages in thread From: Alex Bennée @ 2022-06-10 14:40 UTC (permalink / raw) To: Peter Maydell Cc: qemu-devel, Richard Henderson, Philippe Mathieu-Daudé, Liviu Ionescu Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 26 May 2022 at 20:00, Peter Maydell <peter.maydell@linaro.org> wrote: >> >> This patchset fixes a couple of bugs reported by >> Liviu Ionescu related to semihosting syscall handling. >> >> Firstly, if the user sets up the gdbstub but doesn't connect a gdb >> (ie passes '-s' but not '-S'), and we try to use gdb for semihosting >> syscalls (either because the user explicitly set target=gdb or else >> via autodetection) then we currently segfault. This patch adjusts >> the behaviour so that "gdbstub initialized but no gdb attached to a >> vCPU" is treated the same way we already treat "gdbstub not >> initialized". >> >> Secondly, we weren't handling commandlines where >> --semihosting-config appears multiple times: these ought to be >> merged together but instead we were effectively ignoring >> all but the last one. >> >> thanks >> -- PMM >> >> Peter Maydell (2): >> gdbstub: Don't use GDB syscalls if no GDB is attached >> semihosting/config: Merge --semihosting-config option groups > > I'll take these via target-arm.next. Acked-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-06-10 14:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-26 19:00 [PATCH 0/2] gdbstub: don't crash if no gdb attached and gdb syscall attempted Peter Maydell 2022-05-26 19:00 ` [PATCH 1/2] gdbstub: Don't use GDB syscalls if no GDB is attached Peter Maydell 2022-05-29 11:06 ` Philippe Mathieu-Daudé via 2022-06-09 20:01 ` Luc Michel 2022-05-26 19:00 ` [PATCH 2/2] semihosting/config: Merge --semihosting-config option groups Peter Maydell 2022-06-09 20:02 ` Luc Michel 2022-06-10 13:04 ` [PATCH 0/2] gdbstub: don't crash if no gdb attached and gdb syscall attempted Peter Maydell 2022-06-10 14:40 ` Alex Bennée
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.