[Qemu-devel] [PATCH] fix gdbserver_state pointer validation

[Qemu-devel] [PATCH] fix gdbserver_state pointer validation

[stephane duverger]

Hi,

This is a small patch to gdbstub rather insignificant at first sight:
fix null pointer dereference. It actually allows to take benefit of
gdb features (breakpoints/sstep) internally (ie. special purpose
board) without connecting a gdb client to the Qemu instance gdbserver
stub.

Regards,

Signed-off-by: Stephane Duverger <stephane.duverger@gmail.com>
---
 gdbstub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index d6ab95006c..788fd625ab 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1412,6 +1412,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
 
 void gdb_set_stop_cpu(CPUState *cpu)
 {
+    if (!gdbserver_state) {
+        return;
+    }
     gdbserver_state->c_cpu = cpu;
     gdbserver_state->g_cpu = cpu;
 }
-- 
2.14.1

Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation

[Alex Bennée]

stephane duverger <stephane.duverger@gmail.com> writes:

> Hi,
>
> This is a small patch to gdbstub rather insignificant at first sight:
> fix null pointer dereference. It actually allows to take benefit of
> gdb features (breakpoints/sstep) internally (ie. special purpose
> board) without connecting a gdb client to the Qemu instance gdbserver
> stub.

There don't seem to be any other patches attached? I would NACK a patch
that isn't actually used in-tree. I would also like to see how this
would be used because we certainly have different paths for KVM and TCG
break-point emulation that don't need to go through the gdbstub to
achieve what they are doing.

>
> Regards,
>
> Signed-off-by: Stephane Duverger <stephane.duverger@gmail.com>
> ---
>  gdbstub.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index d6ab95006c..788fd625ab 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1412,6 +1412,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>
>  void gdb_set_stop_cpu(CPUState *cpu)
>  {
> +    if (!gdbserver_state) {
> +        return;
> +    }
>      gdbserver_state->c_cpu = cpu;
>      gdbserver_state->g_cpu = cpu;
>  }


--
Alex Bennée

Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation

[stephane duverger]

Hi Alex,

There don't seem to be any other patches attached? I would NACK a patch
> that isn't actually used in-tree.


No there isn't ! I should have not been so prolix. Actually the patch
corrects a
possible null pointer dereference in the gdbserver code. That's all folks.

Below is how I discovered it and why it matters, imho, to be fixed
(out of a pending issue).

This null deref does not happen in normal operation because of the way
gdbserver is initialized. However what I was telling you, is that it may be
really interesting to be able to take benefit of some gdb features
internally
without starting a gdbserver and the overhead of the gdb protocol when
there is no need for a client/server interaction while debugging.

For instance, I developed a special purpose x86 board where I needed to
break at some instructions, do some automation (snapshots, vm memory
access, cpu analysis) and then resume the VM. Implementing a
"vm_change_state handler" and dealing with RUN_STATE_DEBUG was
perfect for my need.

The debug events (#BP, #DB) are tied to the gdbserver stub, at some point
when "cpu_handle_guest_debug()" is called, a call to "gdb_set_stop_cpu()"
triggers the NULL deref because the gdbserver is not initialized.

My little fix addresses this error and allows to make use of the following
Qemu
breakpoint functions without touching "cpus.c" or "exec.c":
- cpu_breakpoint_remove/insert()
- kvm_remove/insert_breakpoint()

Notice, that in the particular case of KVM, I had to async_run_on_cpu() my
debug handler instead of directly executing it in the context of
vm_change_state(). The vCPU started to significantly slow down
(low irq rate), while this never happens with the TCG.

Is that more clear to you Alex ? (and hopefully lead to patch ACK :)

Regards,

stephane

> Signed-off-by: Stephane Duverger <stephane.duverger@gmail.com>
> > ---
> >  gdbstub.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index d6ab95006c..788fd625ab 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -1412,6 +1412,9 @@ static int gdb_handle_packet(GDBState *s, const
> char *line_buf)
> >
> >  void gdb_set_stop_cpu(CPUState *cpu)
> >  {
> > +    if (!gdbserver_state) {
> > +        return;
> > +    }
> >      gdbserver_state->c_cpu = cpu;
> >      gdbserver_state->g_cpu = cpu;
> >  }
>
>
> --
> Alex Bennée
>

Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation

[Philippe Mathieu-Daudé]

On 07/10/2018 08:44 AM, stephane duverger wrote:
> Hi Alex,
> 
> There don't seem to be any other patches attached? I would NACK a patch
>> that isn't actually used in-tree.
> 
> 
> No there isn't ! I should have not been so prolix. Actually the patch
> corrects a
> possible null pointer dereference in the gdbserver code. That's all folks.
> 
> Below is how I discovered it and why it matters, imho, to be fixed
> (out of a pending issue).
> 
> This null deref does not happen in normal operation because of the way
> gdbserver is initialized. However what I was telling you, is that it may be
> really interesting to be able to take benefit of some gdb features
> internally
> without starting a gdbserver and the overhead of the gdb protocol when
> there is no need for a client/server interaction while debugging.

To reach gdb_set_stop_cpu() with gdbserver_state == NULL, you previously
entered gdb_vm_state_change() with and use CPUState *cpu =
gdbserver_state->c_cpu = NULL deref, which shouldn't happen.
Also in gdb_set_stop_cpu() you finally call cpu_single_step(cpu=crap)
which then deref crap->singlestep_enabled.

So I think this is not the correct fix.

Since this shouldn't happen, I'd add an assert(gdbserver_state) in
gdb_set_stop_cpu(), and clean gdb_vm_state_change()'s code flow to not
reach this.

> For instance, I developed a special purpose x86 board where I needed to
> break at some instructions, do some automation (snapshots, vm memory
> access, cpu analysis) and then resume the VM. Implementing a
> "vm_change_state handler" and dealing with RUN_STATE_DEBUG was
> perfect for my need.

I recommend you to use "configure --enable-sanitizers" to build your
special purpose board, that will show up those issues.

> The debug events (#BP, #DB) are tied to the gdbserver stub, at some point
> when "cpu_handle_guest_debug()" is called, a call to "gdb_set_stop_cpu()"
> triggers the NULL deref because the gdbserver is not initialized.
> 
> My little fix addresses this error and allows to make use of the following
> Qemu
> breakpoint functions without touching "cpus.c" or "exec.c":
> - cpu_breakpoint_remove/insert()
> - kvm_remove/insert_breakpoint()
> 
> Notice, that in the particular case of KVM, I had to async_run_on_cpu() my
> debug handler instead of directly executing it in the context of
> vm_change_state(). The vCPU started to significantly slow down
> (low irq rate), while this never happens with the TCG.
> 
> Is that more clear to you Alex ? (and hopefully lead to patch ACK :)
> 
> Regards,
> 
> stephane
> 
>> Signed-off-by: Stephane Duverger <stephane.duverger@gmail.com>
>>> ---
>>>  gdbstub.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index d6ab95006c..788fd625ab 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -1412,6 +1412,9 @@ static int gdb_handle_packet(GDBState *s, const
>> char *line_buf)
>>>
>>>  void gdb_set_stop_cpu(CPUState *cpu)
>>>  {
>>> +    if (!gdbserver_state) {
>>> +        return;
>>> +    }
>>>      gdbserver_state->c_cpu = cpu;
>>>      gdbserver_state->g_cpu = cpu;

I find it safer the opposite way, if (s) { s-> ... }

>>>  }

Regards,

Phil.

Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation

[stephane duverger]

To reach gdb_set_stop_cpu() with gdbserver_state == NULL, you previously
> entered gdb_vm_state_change() with and use CPUState *cpu =
> gdbserver_state->c_cpu = NULL deref, which shouldn't happen.
> Also in gdb_set_stop_cpu() you finally call cpu_single_step(cpu=crap)
> which then deref crap->singlestep_enabled.
>
> So I think this is not the correct fix.
>

I think you are wrong. You can enter gdb_vm_state_change() only if it has
been registered through qemu_add_vm_change_state_handler(). And this
happens in gdbserver_start() which is called only when you start the
gdbserver stub.

This is exactly what we don't want to do here: use qemu breakpoints and
debug events forwarding without the need to enable a gdb stub.

There might be an historical reason that vCPU debug events are always
forwarded to the gdbserver (from cpu_handle_guest_debug()) but this
should not be mandatory.

One could consider a check to the gdbserver state right before:

if (gdbserver_enabled())
    gdb_set_stop_cpu(cpu);

As found in other part of qemu code: kvm_enabled, hax_enabled, ...


> Since this shouldn't happen, I'd add an assert(gdbserver_state) in
> gdb_set_stop_cpu(), and clean gdb_vm_state_change()'s code flow to not
> reach this.
>

Correct if you previously add the gdbserver_enabled() check. Else this
would abort the qemu instance each time a debug event is triggered
and forwarded to a vm_change_state handler.

>>>  void gdb_set_stop_cpu(CPUState *cpu)
> >>>  {
> >>> +    if (!gdbserver_state) {
> >>> +        return;
> >>> +    }
> >>>      gdbserver_state->c_cpu = cpu;
> >>>      gdbserver_state->g_cpu = cpu;
>
> I find it safer the opposite way, if (s) { s-> ... }
>

Sincerely, this argument is subjective. If it's part of Qemu coding
standard,
i would agree. Again there is a lot of situations in the Qemu code where
exit conditions are checked first and then lead to a return, preventing an
unneeded level of indentation. Seriously, we are talking about a 2 lines
function.

stephane

Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 2606 bytes --]

Hi Stephane,

On 07/11/2018 04:52 AM, stephane duverger wrote:
> To reach gdb_set_stop_cpu() with gdbserver_state == NULL, you previously
>> entered gdb_vm_state_change() with and use CPUState *cpu =
>> gdbserver_state->c_cpu = NULL deref, which shouldn't happen.
>> Also in gdb_set_stop_cpu() you finally call cpu_single_step(cpu=crap)
>> which then deref crap->singlestep_enabled.
>>
>> So I think this is not the correct fix.
>>
> 
> I think you are wrong. You can enter gdb_vm_state_change() only if it has
> been registered through qemu_add_vm_change_state_handler(). And this
> happens in gdbserver_start() which is called only when you start the
> gdbserver stub.
> 
> This is exactly what we don't want to do here: use qemu breakpoints and
> debug events forwarding without the need to enable a gdb stub.

Well, at least we agree the gdb stub code is not straightforward.

And apparently without seeing the bigger picture about how you are using
this, I am missing something.

> There might be an historical reason that vCPU debug events are always
> forwarded to the gdbserver (from cpu_handle_guest_debug()) but this
> should not be mandatory.
> 
> One could consider a check to the gdbserver state right before:
> 
> if (gdbserver_enabled())
>     gdb_set_stop_cpu(cpu);
> 
> As found in other part of qemu code: kvm_enabled, hax_enabled, ...
> 
> 
>> Since this shouldn't happen, I'd add an assert(gdbserver_state) in
>> gdb_set_stop_cpu(), and clean gdb_vm_state_change()'s code flow to not
>> reach this.
>>
> 
> Correct if you previously add the gdbserver_enabled() check. Else this
> would abort the qemu instance each time a debug event is triggered
> and forwarded to a vm_change_state handler.
> 
>>>>  void gdb_set_stop_cpu(CPUState *cpu)
>>>>>  {
>>>>> +    if (!gdbserver_state) {
>>>>> +        return;
>>>>> +    }
>>>>>      gdbserver_state->c_cpu = cpu;
>>>>>      gdbserver_state->g_cpu = cpu;
>>
>> I find it safer the opposite way, if (s) { s-> ... }
>>
> 
> Sincerely, this argument is subjective. If it's part of Qemu coding
> standard,
> i would agree. Again there is a lot of situations in the Qemu code where
> exit conditions are checked first and then lead to a return, preventing an
> unneeded level of indentation. Seriously, we are talking about a 2 lines
> function.

This is indeed a personal subjective suggestion, not part of QEMU Coding
standard. Rational is, if in the future someone needs to modify
gdb_set_stop_cpu(), it would be easier/safer for him.

No worries ;)

Regards,

Phil.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation

[stephane duverger]

Hi Phil,

Well, at least we agree the gdb stub code is not straightforward.
>
> And apparently without seeing the bigger picture about how you are using
> this, I am missing something.


Sorry for that, i will try to explain it clearly.
This is something rather simple indeed:

1 - during MachineState init, install a BP1 at an "OS_ready" location
2 - handle BP1 hit (thanks to vm_state_change())
3 - delete BP1 and save the VM
4 - inject some generated code
5 - install BP2 at injected code end
6 - resume the VM
7 - handle BP2 hit and restore the VM
8 - go to 4

So as you may guess, I didn't need a full gdbstub with a gdbclient
to do that. I just wanted to install breakpoints and handle them
internally. To be fast, no overhead with the gdb protocol.

I didn't find something ready to use in Qemu API, but if there exists
something to do it, I would be glad to know. By the time, I decided to
make use of the GDB breakpoint type. It was a perfect candidate,
but unfortunately any time cpu_handle_guest_debug() is called,
gdb_set_stop_cpu() is systematically called too.

That was my issue. As gdbserver is not up and running,
gdbserver_state is NULL and Qemu segfaults.

Out of this gdb centric function call, the standard Qemu vm stop
notification process seems perfect to me:
- qemu_system_debug_request
- do_vm_stop
- vm_state_notify
- and then my "vm_state_change_handler" is called

So that's why I thought it would be great to be able
to install breakpoints with either
cpu_breakpoint_insert() or kvm_insert_breakpoint().

What could have been done to be pragmatic:
1 - implement a new breakpoint type for internal purpose ?
2 - implement a gdbserver_enabled state ?
3 - check for the gdbserver_state NULL pointer case and return ?

The point 3 was the most simple, tied to only one gdb function,
didn't touch anything else and actually "secured" that
gdb_set_stop_cpu() function.

This is indeed a personal subjective suggestion, not part of QEMU Coding
> standard. Rational is, if in the future someone needs to modify
> gdb_set_stop_cpu(), it would be easier/safer for him.
>

I do understand that point. So what do we do right now :
1 - ACK the patch as is ?
2 - do you want a new patch with:
    - a gdbserver_enabled state
    - a safe execution condition in gdb_set_stop_cpu()
    - an abort() in it if something is wrong

Regards,

stephane