Keys are int.  HMP sendkey assigns them from the value strtoul(),
silently truncating values greater than INT_MAX.  Fix to reject them.

While there, use qemu_strtoul() instead of strtoul() so checkpatch.pl
won't complain.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 monitor/hmp-cmds.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index ed78a87ddd..b8e294e6fa 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1549,8 +1549,13 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
         v = g_malloc0(sizeof(*v));
 
         if (strstart(keys, "0x", NULL)) {
-            char *endp;
-            int value = strtoul(keys, &endp, 0);
+            const char *endp;
+            unsigned long value;
+
+            if (qemu_strtoul(keys, &endp, 0, &value) < 0
+                || value >= INT_MAX) {
+                goto err_out;
+            }
             assert(endp <= keys + keyname_len);
             if (endp != keys + keyname_len) {
                 goto err_out;
-- 
2.38.1

* Markus Armbruster (armbru@redhat.com) wrote:
> Keys are int.  HMP sendkey assigns them from the value strtoul(),
> silently truncating values greater than INT_MAX.  Fix to reject them.
> 
> While there, use qemu_strtoul() instead of strtoul() so checkpatch.pl
> won't complain.

Last time through you said you could switch to qemu_strtoui, but
I just noticed we've actually got a qemu_strto*i* - that
would remove the value comparison

Dave

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  monitor/hmp-cmds.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index ed78a87ddd..b8e294e6fa 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1549,8 +1549,13 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>          v = g_malloc0(sizeof(*v));
>  
>          if (strstart(keys, "0x", NULL)) {
> -            char *endp;
> -            int value = strtoul(keys, &endp, 0);
> +            const char *endp;
> +            unsigned long value;
> +
> +            if (qemu_strtoul(keys, &endp, 0, &value) < 0
> +                || value >= INT_MAX) {
> +                goto err_out;
> +            }
>              assert(endp <= keys + keyname_len);
>              if (endp != keys + keyname_len) {
>                  goto err_out;
> -- 
> 2.38.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Keys are int.  HMP sendkey assigns them from the value strtoul(),
>> silently truncating values greater than INT_MAX.  Fix to reject them.
>> 
>> While there, use qemu_strtoul() instead of strtoul() so checkpatch.pl
>> won't complain.
>
> Last time through you said you could switch to qemu_strtoui, but

Did I?  Oh, I did in review of

    [PATCH 08/12] pci: Fix silent truncation of pcie_aer_inject_error argument

but failed to do it here, too.

> I just noticed we've actually got a qemu_strto*i* - that
> would remove the value comparison

Thanks!