gdbstub: Correct misparsing of vCont C/S requests

[PATCH] gdbstub: Correct misparsing of vCont C/S requests

Posted by Peter Maydell 3 years, 5 months ago

In the vCont packet, two of the command actions (C and S) take an
argument specifying the signal to be sent to the process/thread, which is
sent as an ASCII string of two hex digits which immediately follow the
'C' or 'S' character.

Our code for parsing this packet accidentally skipped the first of the
two bytes of the signal value, because it started parsing the hex string
at 'p + 1' when the preceding code had already moved past the 'C' or
'S' with "cur_action = *p++".

This meant that we would only do the right thing for signals below
10, and would misinterpret the rest.  For instance, when the debugger
wants to send the process a SIGPROF (27 on x86-64) we mangle this into
a SIGSEGV (11).

Remove the accidental double increment.

Fixes: https://bugs.launchpad.net/qemu/+bug/1773743
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Haven't really given this enough testing to want to put it into 5.2,
I think (though it does fix the repro in the bug report).
The bug has been present since commit 544177ad1cfd78 from 2017.

 gdbstub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbstub.c b/gdbstub.c
index f19f98ab1ab..d99bc0bf2ea 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1243,7 +1243,7 @@ static int gdb_handle_vcont(const char *p)
         cur_action = *p++;
         if (cur_action == 'C' || cur_action == 'S') {
             cur_action = qemu_tolower(cur_action);
-            res = qemu_strtoul(p + 1, &p, 16, &tmp);
+            res = qemu_strtoul(p, &p, 16, &tmp);
             if (res) {
                 goto out;
             }
-- 
2.20.1

Re: [PATCH] gdbstub: Correct misparsing of vCont C/S requests

Posted by Philippe Mathieu-Daudé 3 years, 5 months ago

On 11/21/20 10:03 PM, Peter Maydell wrote:
> In the vCont packet, two of the command actions (C and S) take an
> argument specifying the signal to be sent to the process/thread, which is
> sent as an ASCII string of two hex digits which immediately follow the
> 'C' or 'S' character.
> 
> Our code for parsing this packet accidentally skipped the first of the
> two bytes of the signal value, because it started parsing the hex string
> at 'p + 1' when the preceding code had already moved past the 'C' or
> 'S' with "cur_action = *p++".
> 
> This meant that we would only do the right thing for signals below
> 10, and would misinterpret the rest.  For instance, when the debugger
> wants to send the process a SIGPROF (27 on x86-64) we mangle this into
> a SIGSEGV (11).
> 
> Remove the accidental double increment.
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1773743

Eventually more precise using:

Fixes: 544177ad1cf ("gdbstub: Fix vCont behaviour")
Buglink: https://bugs.launchpad.net/qemu/+bug/1773743

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Haven't really given this enough testing to want to put it into 5.2,
> I think (though it does fix the repro in the bug report).
> The bug has been present since commit 544177ad1cfd78 from 2017.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

I am not against having it fixed for 5.2 (trivial fix,
reporter can use sigprof again).

Thanks for the fix!

Phil.

> 
>  gdbstub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index f19f98ab1ab..d99bc0bf2ea 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1243,7 +1243,7 @@ static int gdb_handle_vcont(const char *p)
>          cur_action = *p++;
>          if (cur_action == 'C' || cur_action == 'S') {
>              cur_action = qemu_tolower(cur_action);
> -            res = qemu_strtoul(p + 1, &p, 16, &tmp);
> +            res = qemu_strtoul(p, &p, 16, &tmp);
>              if (res) {
>                  goto out;
>              }
>

Re: [PATCH] gdbstub: Correct misparsing of vCont C/S requests

Posted by Alex Bennée 3 years, 5 months ago

Peter Maydell <peter.maydell@linaro.org> writes:

> In the vCont packet, two of the command actions (C and S) take an
> argument specifying the signal to be sent to the process/thread, which is
> sent as an ASCII string of two hex digits which immediately follow the
> 'C' or 'S' character.
>
> Our code for parsing this packet accidentally skipped the first of the
> two bytes of the signal value, because it started parsing the hex string
> at 'p + 1' when the preceding code had already moved past the 'C' or
> 'S' with "cur_action = *p++".
>
> This meant that we would only do the right thing for signals below
> 10, and would misinterpret the rest.  For instance, when the debugger
> wants to send the process a SIGPROF (27 on x86-64) we mangle this into
> a SIGSEGV (11).
>
> Remove the accidental double increment.
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1773743
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

LGTM

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
> Haven't really given this enough testing to want to put it into 5.2,
> I think (though it does fix the repro in the bug report).
> The bug has been present since commit 544177ad1cfd78 from 2017.

I'd be happy including it. I don't have any gdbstub patches queued at
the moment but I could put together one if you want or you could just
include it directly if you are now happy to.

>
>  gdbstub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index f19f98ab1ab..d99bc0bf2ea 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1243,7 +1243,7 @@ static int gdb_handle_vcont(const char *p)
>          cur_action = *p++;
>          if (cur_action == 'C' || cur_action == 'S') {
>              cur_action = qemu_tolower(cur_action);
> -            res = qemu_strtoul(p + 1, &p, 16, &tmp);
> +            res = qemu_strtoul(p, &p, 16, &tmp);
>              if (res) {
>                  goto out;
>              }


-- 
Alex Bennée

Re: [PATCH] gdbstub: Correct misparsing of vCont C/S requests

Posted by Peter Maydell 3 years, 4 months ago

On Mon, 23 Nov 2020 at 13:41, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > In the vCont packet, two of the command actions (C and S) take an
> > argument specifying the signal to be sent to the process/thread, which is
> > sent as an ASCII string of two hex digits which immediately follow the
> > 'C' or 'S' character.
> >
> > Our code for parsing this packet accidentally skipped the first of the
> > two bytes of the signal value, because it started parsing the hex string
> > at 'p + 1' when the preceding code had already moved past the 'C' or
> > 'S' with "cur_action = *p++".
> >
> > This meant that we would only do the right thing for signals below
> > 10, and would misinterpret the rest.  For instance, when the debugger
> > wants to send the process a SIGPROF (27 on x86-64) we mangle this into
> > a SIGSEGV (11).
> >
> > Remove the accidental double increment.
> >
> > Fixes: https://bugs.launchpad.net/qemu/+bug/1773743
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> LGTM
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
> > ---
> > Haven't really given this enough testing to want to put it into 5.2,
> > I think (though it does fix the repro in the bug report).
> > The bug has been present since commit 544177ad1cfd78 from 2017.
>
> I'd be happy including it. I don't have any gdbstub patches queued at
> the moment but I could put together one if you want or you could just
> include it directly if you are now happy to.

Now that 6.0 has opened up, I'll put this in via target-arm.next
unless you'd prefer to take it.

-- PMM

Re: [PATCH] gdbstub: Correct misparsing of vCont C/S requests

Posted by Alex Bennée 3 years, 4 months ago

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 23 Nov 2020 at 13:41, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > In the vCont packet, two of the command actions (C and S) take an
>> > argument specifying the signal to be sent to the process/thread, which is
>> > sent as an ASCII string of two hex digits which immediately follow the
>> > 'C' or 'S' character.
>> >
>> > Our code for parsing this packet accidentally skipped the first of the
>> > two bytes of the signal value, because it started parsing the hex string
>> > at 'p + 1' when the preceding code had already moved past the 'C' or
>> > 'S' with "cur_action = *p++".
>> >
>> > This meant that we would only do the right thing for signals below
>> > 10, and would misinterpret the rest.  For instance, when the debugger
>> > wants to send the process a SIGPROF (27 on x86-64) we mangle this into
>> > a SIGSEGV (11).
>> >
>> > Remove the accidental double increment.
>> >
>> > Fixes: https://bugs.launchpad.net/qemu/+bug/1773743
>> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> LGTM
>>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> > ---
>> > Haven't really given this enough testing to want to put it into 5.2,
>> > I think (though it does fix the repro in the bug report).
>> > The bug has been present since commit 544177ad1cfd78 from 2017.
>>
>> I'd be happy including it. I don't have any gdbstub patches queued at
>> the moment but I could put together one if you want or you could just
>> include it directly if you are now happy to.
>
> Now that 6.0 has opened up, I'll put this in via target-arm.next
> unless you'd prefer to take it.

Go for it, I have nothing else at the moment.

>
> -- PMM


-- 
Alex Bennée