[Qemu-devel] [PATCH v2 0/6] Fix misuse of ctype.h functions

[Qemu-devel] [PATCH v2 0/6] Fix misuse of ctype.h functions

[Markus Armbruster]

v2:
* PATCH 1: Use g_ascii_isspace(), adapt commit message [Philippe]
* PATCH 3: Add comment pointing to the GDB manual [Philippe]
* PATCH 5: Improve commit message [Thomas]

Markus Armbruster (6):
  qemu-bridge-helper: Fix misuse of isspace()
  tests/vhost-user-bridge: Fix misuse of isdigit()
  gdbstub: Reject invalid RLE repeat counts
  gdbstub: Fix misuse of isxdigit()
  pc-bios/s390-ccw: Clean up harmless misuse of isdigit()
  cutils: Simplify how parse_uint() checks for whitespace

 gdbstub.c                 | 20 ++++++++++++--------
 pc-bios/s390-ccw/libc.c   |  2 +-
 pc-bios/s390-ccw/menu.c   |  2 +-
 qemu-bridge-helper.c      |  6 +++---
 tests/vhost-user-bridge.c |  2 +-
 util/cutils.c             |  2 +-
 6 files changed, 19 insertions(+), 15 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v2 1/6] qemu-bridge-helper: Fix misuse of isspace()

[Markus Armbruster]

parse_acl_file() passes char values to isspace().  Undefined behavior
when the value is negative.  Not a security issue, because the
characters come from trusted $prefix/etc/qemu/bridge.conf and the
files it includes.

Furthermore, isspace()'s locale-dependence means qemu-bridge-helper
uses the user's locale for parsing $prefix/etc/bridge.conf.  Feels
wrong.

Use g_ascii_isspace() instead.  This fixes the undefined behavior, and
makes parsing of $prefix/etc/bridge.conf locale-independent.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-bridge-helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 5396fbfbb6..f9940deefd 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -75,7 +75,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
         char *ptr = line;
         char *cmd, *arg, *argend;
 
-        while (isspace(*ptr)) {
+        while (g_ascii_isspace(*ptr)) {
             ptr++;
         }
 
@@ -99,12 +99,12 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 
         *arg = 0;
         arg++;
-        while (isspace(*arg)) {
+        while (g_ascii_isspace(*arg)) {
             arg++;
         }
 
         argend = arg + strlen(arg);
-        while (arg != argend && isspace(*(argend - 1))) {
+        while (arg != argend && g_ascii_isspace(*(argend - 1))) {
             argend--;
         }
         *argend = 0;
-- 
2.17.2

[Qemu-devel] [PATCH v2 2/6] tests/vhost-user-bridge: Fix misuse of isdigit()

[Markus Armbruster]

vubr_set_host() passes char values to isdigit().  Undefined behavior
when the value is negative.

Fix by using qemu_isdigit() instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/vhost-user-bridge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
index 0033b61f2e..d70b107ebc 100644
--- a/tests/vhost-user-bridge.c
+++ b/tests/vhost-user-bridge.c
@@ -645,7 +645,7 @@ vubr_host_notifier_setup(VubrDev *dev)
 static void
 vubr_set_host(struct sockaddr_in *saddr, const char *host)
 {
-    if (isdigit(host[0])) {
+    if (qemu_isdigit(host[0])) {
         if (!inet_aton(host, &saddr->sin_addr)) {
             fprintf(stderr, "inet_aton() failed.\n");
             exit(1);
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 2/6] tests/vhost-user-bridge: Fix misuse of isdigit()

[Philippe Mathieu-Daudé]

On 5/14/19 8:03 PM, Markus Armbruster wrote:
> vubr_set_host() passes char values to isdigit().  Undefined behavior

"happens"?

> when the value is negative.
> 
> Fix by using qemu_isdigit() instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/vhost-user-bridge.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 0033b61f2e..d70b107ebc 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -645,7 +645,7 @@ vubr_host_notifier_setup(VubrDev *dev)
>  static void
>  vubr_set_host(struct sockaddr_in *saddr, const char *host)
>  {
> -    if (isdigit(host[0])) {
> +    if (qemu_isdigit(host[0])) {
>          if (!inet_aton(host, &saddr->sin_addr)) {
>              fprintf(stderr, "inet_aton() failed.\n");
>              exit(1);
> 

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

Re: [Qemu-devel] [PATCH v2 2/6] tests/vhost-user-bridge: Fix misuse of isdigit()

[Thomas Huth]

On 14/05/2019 20.03, Markus Armbruster wrote:
> vubr_set_host() passes char values to isdigit().  Undefined behavior
> when the value is negative.
> 
> Fix by using qemu_isdigit() instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/vhost-user-bridge.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 0033b61f2e..d70b107ebc 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -645,7 +645,7 @@ vubr_host_notifier_setup(VubrDev *dev)
>  static void
>  vubr_set_host(struct sockaddr_in *saddr, const char *host)
>  {
> -    if (isdigit(host[0])) {
> +    if (qemu_isdigit(host[0])) {
>          if (!inet_aton(host, &saddr->sin_addr)) {
>              fprintf(stderr, "inet_aton() failed.\n");
>              exit(1);

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/6] tests/vhost-user-bridge: Fix misuse of isdigit()

[Thomas Huth]

On 14/05/2019 20.41, Thomas Huth wrote:
> On 14/05/2019 20.03, Markus Armbruster wrote:
>> vubr_set_host() passes char values to isdigit().  Undefined behavior
>> when the value is negative.
>>
>> Fix by using qemu_isdigit() instead.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/vhost-user-bridge.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
>> index 0033b61f2e..d70b107ebc 100644
>> --- a/tests/vhost-user-bridge.c
>> +++ b/tests/vhost-user-bridge.c
>> @@ -645,7 +645,7 @@ vubr_host_notifier_setup(VubrDev *dev)
>>  static void
>>  vubr_set_host(struct sockaddr_in *saddr, const char *host)
>>  {
>> -    if (isdigit(host[0])) {
>> +    if (qemu_isdigit(host[0])) {
>>          if (!inet_aton(host, &saddr->sin_addr)) {
>>              fprintf(stderr, "inet_aton() failed.\n");
>>              exit(1);
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

By the way, how do you compile / run this test? The original commit
message say one should compile it with "make tests/vhost-user-bridge"
but that does not work for me:

$ make tests/vhost-user-bridge
cc     tests/vhost-user-bridge.c   -o tests/vhost-user-bridge
tests/vhost-user-bridge.c:32:24: fatal error: qemu/osdep.h: No such file
or directory

 Thomas

Re: [Qemu-devel] [PATCH v2 2/6] tests/vhost-user-bridge: Fix misuse of isdigit()

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 14/05/2019 20.41, Thomas Huth wrote:
>> On 14/05/2019 20.03, Markus Armbruster wrote:
>>> vubr_set_host() passes char values to isdigit().  Undefined behavior
>>> when the value is negative.
>>>
>>> Fix by using qemu_isdigit() instead.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  tests/vhost-user-bridge.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
>>> index 0033b61f2e..d70b107ebc 100644
>>> --- a/tests/vhost-user-bridge.c
>>> +++ b/tests/vhost-user-bridge.c

Squashing in

    @@ -30,6 +30,7 @@
     #define _FILE_OFFSET_BITS 64

     #include "qemu/osdep.h"
    +#include "qemu-common.h"
     #include "qemu/atomic.h"
     #include "qemu/iov.h"
     #include "standard-headers/linux/virtio_net.h"

>>> @@ -645,7 +645,7 @@ vubr_host_notifier_setup(VubrDev *dev)
>>>  static void
>>>  vubr_set_host(struct sockaddr_in *saddr, const char *host)
>>>  {
>>> -    if (isdigit(host[0])) {
>>> +    if (qemu_isdigit(host[0])) {
>>>          if (!inet_aton(host, &saddr->sin_addr)) {
>>>              fprintf(stderr, "inet_aton() failed.\n");
>>>              exit(1);
>> 
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
> By the way, how do you compile / run this test? The original commit
> message say one should compile it with "make tests/vhost-user-bridge"
> but that does not work for me:
>
> $ make tests/vhost-user-bridge
> cc     tests/vhost-user-bridge.c   -o tests/vhost-user-bridge
> tests/vhost-user-bridge.c:32:24: fatal error: qemu/osdep.h: No such file
> or directory

With that fixup, it compiles for me.

Thanks for your question!  I blindly assumed "make check" actually
compiled this.

[Qemu-devel] [PATCH v2 3/6] gdbstub: Reject invalid RLE repeat counts

[Markus Armbruster]

"Debugging with GDB / Appendix E GDB Remote Serial Protocol /
Overview" specifies "The printable characters '#' and '$' or with a
numeric value greater than 126 must not be used."  gdb_read_byte()
only rejects values < 32.  This is wrong.  Impact depends on the caller:

* gdb_handlesig() passes a char.  Incorrectly accepts '#', '$' and
  '\127'.

* gdb_chr_receive() passes an uint8_t.  Additionally accepts
  characters with the most-significant bit set.

Correct the validity check to match the specification.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 gdbstub.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdbstub.c b/gdbstub.c
index d54abd17cc..c41eb1de07 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2064,7 +2064,11 @@ static void gdb_read_byte(GDBState *s, int ch)
             }
             break;
         case RS_GETLINE_RLE:
-            if (ch < ' ') {
+            /*
+             * Run-length encoding is explained in "Debugging with GDB /
+             * Appendix E GDB Remote Serial Protocol / Overview".
+             */
+            if (ch < ' ' || ch == '#' || ch == '$' || ch > 126) {
                 /* invalid RLE count encoding */
                 trace_gdbstub_err_invalid_repeat((uint8_t)ch);
                 s->state = RS_GETLINE;
-- 
2.17.2

[Qemu-devel] [PATCH v2 4/6] gdbstub: Fix misuse of isxdigit()

[Markus Armbruster]

gdb_read_byte() passes its @ch argument to isxdigit().  Undefined
behavior when the value is negative.  Two callers:

* gdb_chr_receive() passes an uint8_t value.  Safe.

* gdb_handlesig() a char value.  Unsafe.  Not a security issue,
  because the characters come from the gdb client, which is trusted.

The obvious fix would be casting @ch to unsigned char.  But note that
gdb_read_byte() already casts @ch to uint8_t in many places.  Uses of
@ch without such a cast:

(1) Compare to a character constant with == or !=

(2) s->linesum += ch

(3) Store ch or ch ^ 0x20 into s->line_buf[]

(4) Check for invalid RLE count:
    ch < ' ' || ch == '#' || ch == '$' || ch > 126

(5) Pass to isxdigit()

(6) Pass to fromhex()

Change the parameter type from int to uint8_t, and drop the now
redundant casts.  Affects the above uses as follows:

(1) No change: the character constants are all non-negative.

(2) Effectively no change: we only ever use s->linesum & 0xff, and
    s->linesum is int.

(3) No change: s->line_buf[] is char[].

(4) No change.

(5) Avoid undefined behavior.

(6) No change: only reached when isxdigit(ch)

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 gdbstub.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index c41eb1de07..b129df4e59 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1987,7 +1987,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
     va_end(va);
 }
 
-static void gdb_read_byte(GDBState *s, int ch)
+static void gdb_read_byte(GDBState *s, uint8_t ch)
 {
     uint8_t reply;
 
@@ -2001,7 +2001,7 @@ static void gdb_read_byte(GDBState *s, int ch)
         } else if (ch == '+') {
             trace_gdbstub_io_got_ack();
         } else {
-            trace_gdbstub_io_got_unexpected((uint8_t)ch);
+            trace_gdbstub_io_got_unexpected(ch);
         }
 
         if (ch == '+' || ch == '$')
@@ -2024,7 +2024,7 @@ static void gdb_read_byte(GDBState *s, int ch)
                 s->line_sum = 0;
                 s->state = RS_GETLINE;
             } else {
-                trace_gdbstub_err_garbage((uint8_t)ch);
+                trace_gdbstub_err_garbage(ch);
             }
             break;
         case RS_GETLINE:
@@ -2070,11 +2070,11 @@ static void gdb_read_byte(GDBState *s, int ch)
              */
             if (ch < ' ' || ch == '#' || ch == '$' || ch > 126) {
                 /* invalid RLE count encoding */
-                trace_gdbstub_err_invalid_repeat((uint8_t)ch);
+                trace_gdbstub_err_invalid_repeat(ch);
                 s->state = RS_GETLINE;
             } else {
                 /* decode repeat length */
-                int repeat = (unsigned char)ch - ' ' + 3;
+                int repeat = ch - ' ' + 3;
                 if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) {
                     /* that many repeats would overrun the command buffer */
                     trace_gdbstub_err_overrun();
@@ -2096,7 +2096,7 @@ static void gdb_read_byte(GDBState *s, int ch)
         case RS_CHKSUM1:
             /* get high hex digit of checksum */
             if (!isxdigit(ch)) {
-                trace_gdbstub_err_checksum_invalid((uint8_t)ch);
+                trace_gdbstub_err_checksum_invalid(ch);
                 s->state = RS_GETLINE;
                 break;
             }
@@ -2107,7 +2107,7 @@ static void gdb_read_byte(GDBState *s, int ch)
         case RS_CHKSUM2:
             /* get low hex digit of checksum */
             if (!isxdigit(ch)) {
-                trace_gdbstub_err_checksum_invalid((uint8_t)ch);
+                trace_gdbstub_err_checksum_invalid(ch);
                 s->state = RS_GETLINE;
                 break;
             }
-- 
2.17.2

[Qemu-devel] [PATCH v2 5/6] pc-bios/s390-ccw: Clean up harmless misuse of isdigit()

[Markus Armbruster]

atoui() and get_index() pass char values to isdigit().  With a
standard isdigit(), we'd get undefined behavior when the value is
negative.  Can't happen as char is unsigned on s390x.  Even if it
could, we're actually using isdigit() from pc-bios/s390-ccw/libc.h
here, which works fine for negative values.  Clean up anyway, just
to avoid setting a bad example.

Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: qemu-s390x@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 pc-bios/s390-ccw/libc.c | 2 +-
 pc-bios/s390-ccw/menu.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/pc-bios/s390-ccw/libc.c b/pc-bios/s390-ccw/libc.c
index a786566c4c..3187923950 100644
--- a/pc-bios/s390-ccw/libc.c
+++ b/pc-bios/s390-ccw/libc.c
@@ -38,7 +38,7 @@ uint64_t atoui(const char *str)
     }
 
     while (*str) {
-        if (!isdigit(*str)) {
+        if (!isdigit(*(unsigned char *)str)) {
             break;
         }
         val = val * 10 + *str - '0';
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index 82a4ae6315..ce3815b201 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -134,7 +134,7 @@ static int get_index(void)
 
     /* Check for erroneous input */
     for (i = 0; i < len; i++) {
-        if (!isdigit(buf[i])) {
+        if (!isdigit((unsigned char)buf[i])) {
             return -1;
         }
     }
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 5/6] pc-bios/s390-ccw: Clean up harmless misuse of isdigit()

[Christian Borntraeger]

On 14.05.19 20:03, Markus Armbruster wrote:
> atoui() and get_index() pass char values to isdigit().  With a
> standard isdigit(), we'd get undefined behavior when the value is
> negative.  Can't happen as char is unsigned on s390x.  Even if it
> could, we're actually using isdigit() from pc-bios/s390-ccw/libc.h
> here, which works fine for negative values.  Clean up anyway, just
> to avoid setting a bad example.
> 
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: qemu-s390x@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  pc-bios/s390-ccw/libc.c | 2 +-
>  pc-bios/s390-ccw/menu.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/libc.c b/pc-bios/s390-ccw/libc.c
> index a786566c4c..3187923950 100644
> --- a/pc-bios/s390-ccw/libc.c
> +++ b/pc-bios/s390-ccw/libc.c
> @@ -38,7 +38,7 @@ uint64_t atoui(const char *str)
>      }
>  
>      while (*str) {
> -        if (!isdigit(*str)) {
> +        if (!isdigit(*(unsigned char *)str)) {
>              break;
>          }
>          val = val * 10 + *str - '0';
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> index 82a4ae6315..ce3815b201 100644
> --- a/pc-bios/s390-ccw/menu.c
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -134,7 +134,7 @@ static int get_index(void)
>  
>      /* Check for erroneous input */
>      for (i = 0; i < len; i++) {
> -        if (!isdigit(buf[i])) {
> +        if (!isdigit((unsigned char)buf[i])) {
>              return -1;
>          }
>      }
> 

Re: [Qemu-devel] [PATCH v2 5/6] pc-bios/s390-ccw: Clean up harmless misuse of isdigit()

[Thomas Huth]

On 14/05/2019 20.04, Christian Borntraeger wrote:
> 
> 
> On 14.05.19 20:03, Markus Armbruster wrote:
>> atoui() and get_index() pass char values to isdigit().  With a
>> standard isdigit(), we'd get undefined behavior when the value is
>> negative.  Can't happen as char is unsigned on s390x.  Even if it
>> could, we're actually using isdigit() from pc-bios/s390-ccw/libc.h
>> here, which works fine for negative values.  Clean up anyway, just
>> to avoid setting a bad example.
>>
>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

FYI, this patch is already queued in Cornelia's s390-next tree (and it
should also go via her tree, since you need to rebuild the bios binary
afterwards).

 Thomas

[Qemu-devel] [PATCH v2 6/6] cutils: Simplify how parse_uint() checks for whitespace

[Markus Armbruster]

Use qemu_isspace() so we don't have to cast to unsigned char.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/cutils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/cutils.c b/util/cutils.c
index d682c90901..9aacc422ca 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -683,7 +683,7 @@ int parse_uint(const char *s, unsigned long long *value, char **endptr,
     }
 
     /* make sure we reject negative numbers: */
-    while (isspace((unsigned char)*s)) {
+    while (qemu_isspace(*s)) {
         s++;
     }
     if (*s == '-') {
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 6/6] cutils: Simplify how parse_uint() checks for whitespace

[Philippe Mathieu-Daudé]

On 5/14/19 8:03 PM, Markus Armbruster wrote:
> Use qemu_isspace() so we don't have to cast to unsigned char.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/cutils.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/cutils.c b/util/cutils.c
> index d682c90901..9aacc422ca 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -683,7 +683,7 @@ int parse_uint(const char *s, unsigned long long *value, char **endptr,
>      }
>  
>      /* make sure we reject negative numbers: */
> -    while (isspace((unsigned char)*s)) {
> +    while (qemu_isspace(*s)) {
>          s++;
>      }
>      if (*s == '-') {
> 

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

Re: [Qemu-devel] [PATCH v2 0/6] Fix misuse of ctype.h functions

[Markus Armbruster]

Queued.