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

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

[Markus Armbruster]

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                 | 16 ++++++++--------
 pc-bios/s390-ccw/libc.c   |  2 +-
 pc-bios/s390-ccw/menu.c   |  2 +-
 qemu-bridge-helper.c      |  7 ++++---
 tests/vhost-user-bridge.c |  2 +-
 util/cutils.c             |  2 +-
 6 files changed, 16 insertions(+), 15 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH 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.

Fix by using qemu_isspace() instead.

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

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 5396fbfbb6..0d60c07655 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -29,6 +29,7 @@
 #include <linux/if_bridge.h>
 #endif
 
+#include "qemu-common.h"
 #include "qemu/queue.h"
 
 #include "net/tap-linux.h"
@@ -75,7 +76,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
         char *ptr = line;
         char *cmd, *arg, *argend;
 
-        while (isspace(*ptr)) {
+        while (qemu_isspace(*ptr)) {
             ptr++;
         }
 
@@ -99,12 +100,12 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 
         *arg = 0;
         arg++;
-        while (isspace(*arg)) {
+        while (qemu_isspace(*arg)) {
             arg++;
         }
 
         argend = arg + strlen(arg);
-        while (arg != argend && isspace(*(argend - 1))) {
+        while (arg != argend && qemu_isspace(*(argend - 1))) {
             argend--;
         }
         *argend = 0;
-- 
2.17.2

[Qemu-devel] [PATCH 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

[Qemu-devel] [PATCH 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>
---
 gdbstub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbstub.c b/gdbstub.c
index d54abd17cc..a6dce1b027 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2064,7 +2064,7 @@ static void gdb_read_byte(GDBState *s, int ch)
             }
             break;
         case RS_GETLINE_RLE:
-            if (ch < ' ') {
+            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 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 a6dce1b027..166ccbfbf4 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:
@@ -2066,11 +2066,11 @@ static void gdb_read_byte(GDBState *s, int ch)
         case RS_GETLINE_RLE:
             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();
@@ -2092,7 +2092,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;
             }
@@ -2103,7 +2103,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 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.  But we're using isdigit() from pc-bios/s390-ccw/libc.h
here, which behaves nicely.  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

[Qemu-devel] [PATCH 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 e098debdc0..bed63fc2f1 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 1/6] qemu-bridge-helper: Fix misuse of isspace()

[Philippe Mathieu-Daudé]

On 4/18/19 4:53 PM, Markus Armbruster wrote:
> 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.
> 
> Fix by using qemu_isspace() instead.

Can we use g_ascii_isspace() and remove qemu_isspace()?

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-bridge-helper.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
> index 5396fbfbb6..0d60c07655 100644
> --- a/qemu-bridge-helper.c
> +++ b/qemu-bridge-helper.c
> @@ -29,6 +29,7 @@
>  #include <linux/if_bridge.h>
>  #endif
>  
> +#include "qemu-common.h"
>  #include "qemu/queue.h"
>  
>  #include "net/tap-linux.h"
> @@ -75,7 +76,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>          char *ptr = line;
>          char *cmd, *arg, *argend;
>  
> -        while (isspace(*ptr)) {
> +        while (qemu_isspace(*ptr)) {
>              ptr++;
>          }
>  
> @@ -99,12 +100,12 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>  
>          *arg = 0;
>          arg++;
> -        while (isspace(*arg)) {
> +        while (qemu_isspace(*arg)) {
>              arg++;
>          }
>  
>          argend = arg + strlen(arg);
> -        while (arg != argend && isspace(*(argend - 1))) {
> +        while (arg != argend && qemu_isspace(*(argend - 1))) {
>              argend--;
>          }
>          *argend = 0;
> 

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

[Philippe Mathieu-Daudé]

On 4/18/19 4:53 PM, Markus Armbruster wrote:
> "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>
> ---
>  gdbstub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index d54abd17cc..a6dce1b027 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2064,7 +2064,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>              }
>              break;
>          case RS_GETLINE_RLE:
> -            if (ch < ' ') {

Can you add a comment referring to the ""Debugging with GDB / Appendix E
GDB Remote Serial Protocol / Overview" here?

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

> +            if (ch < ' ' || ch == '#' || ch == '$' || ch > 126) {
>                  /* invalid RLE count encoding */
>                  trace_gdbstub_err_invalid_repeat((uint8_t)ch);
>                  s->state = RS_GETLINE;
> 

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

[Thomas Huth]

On 18/04/2019 16.53, 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.  But we're using isdigit() from pc-bios/s390-ccw/libc.h
> here, which behaves nicely.  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;
>          }
>      }

FWIW, "char" is unsigned by default on s390x, so this is doing nothing.

 Thomas

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

[Thomas Huth]

On 18/04/2019 16.53, 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.  But we're using isdigit() from pc-bios/s390-ccw/libc.h
> here, which behaves nicely.  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;
>          }
>      }

FWIW, "char" is unsigned by default on s390x, so this is doing nothing.

 Thomas

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

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 18/04/2019 16.53, 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.  But we're using isdigit() from pc-bios/s390-ccw/libc.h
>> here, which behaves nicely.  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;
>>          }
>>      }
>
> FWIW, "char" is unsigned by default on s390x, so this is doing nothing.

I see.

If we decide to keep the patch, the commit message needs tweaking.
Perhaps:

    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.

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

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 18/04/2019 16.53, 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.  But we're using isdigit() from pc-bios/s390-ccw/libc.h
>> here, which behaves nicely.  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;
>>          }
>>      }
>
> FWIW, "char" is unsigned by default on s390x, so this is doing nothing.

I see.

If we decide to keep the patch, the commit message needs tweaking.
Perhaps:

    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.

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

[Thomas Huth]

On 18/04/2019 20.13, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 18/04/2019 16.53, 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.  But we're using isdigit() from pc-bios/s390-ccw/libc.h
>>> here, which behaves nicely.  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;
>>>          }
>>>      }
>>
>> FWIW, "char" is unsigned by default on s390x, so this is doing nothing.
> 
> I see.
> 
> If we decide to keep the patch, the commit message needs tweaking.
> Perhaps:
> 
>     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.

Ok, I'll pick this up with the updated commit message.

 Thomas

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

[Markus Armbruster]

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

> On 4/18/19 4:53 PM, Markus Armbruster wrote:
>> "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>
>> ---
>>  gdbstub.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/gdbstub.c b/gdbstub.c
>> index d54abd17cc..a6dce1b027 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -2064,7 +2064,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>>              }
>>              break;
>>          case RS_GETLINE_RLE:
>> -            if (ch < ' ') {
>
> Can you add a comment referring to the ""Debugging with GDB / Appendix E
> GDB Remote Serial Protocol / Overview" here?

Like this?

        case RS_GETLINE_RLE:
            /*
             * Run-length encoding is explained in "Debugging with GDB /
             * Appendix E GDB Remote Serial Protocol / Overview".
             */
            if (ch < ' ') {


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

Thanks!

>> +            if (ch < ' ' || ch == '#' || ch == '$' || ch > 126) {
>>                  /* invalid RLE count encoding */
>>                  trace_gdbstub_err_invalid_repeat((uint8_t)ch);
>>                  s->state = RS_GETLINE;
>> 

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

[Philippe Mathieu-Daudé]

On 5/13/19 2:39 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> On 4/18/19 4:53 PM, Markus Armbruster wrote:
>>> "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>
>>> ---
>>>  gdbstub.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index d54abd17cc..a6dce1b027 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -2064,7 +2064,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>>>              }
>>>              break;
>>>          case RS_GETLINE_RLE:
>>> -            if (ch < ' ') {
>>
>> Can you add a comment referring to the ""Debugging with GDB / Appendix E
>> GDB Remote Serial Protocol / Overview" here?
> 
> Like this?
> 
>         case RS_GETLINE_RLE:
>             /*
>              * Run-length encoding is explained in "Debugging with GDB /
>              * Appendix E GDB Remote Serial Protocol / Overview".
>              */
>             if (ch < ' ') {

Yes, thanks!

> 
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Thanks!
> 
>>> +            if (ch < ' ' || ch == '#' || ch == '$' || ch > 126) {
>>>                  /* invalid RLE count encoding */
>>>                  trace_gdbstub_err_invalid_repeat((uint8_t)ch);
>>>                  s->state = RS_GETLINE;
>>>

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

[Markus Armbruster]

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

> On 4/18/19 4:53 PM, Markus Armbruster wrote:
>> 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.
>> 
>> Fix by using qemu_isspace() instead.
>
> Can we use g_ascii_isspace() and remove qemu_isspace()?

Hmm, I wasn't aware of that one.

                    arg type  arg values              obeys locale?
ctype.h isFOO()     int       unsigned char, EOF [1]  yes [4]
qemu_isFOO()        int       any char [2]            yes [4]
g_ascii_isFOO()     char      any char [3]            no

[1] Undefined behavior when the argument is a negative plain or signed
char.  Well-known trap.

[2] qemu_isFOO(arg) expands into isFOO((unsigned char)arg), which works
fine for plain, signed and unsigned char arg.

[3] When plain char is signed, and the actual argument is unsigned char
and not representable as char, then behavior is implementation-defined.
No problem; the implementations we care for reinterpret as two's
complement.

[4] Obeying the locale is commonly unwanted.

Replacing isFOO() by qemu_isFOO() gets rid of trap [1] (and loses EOF,
but that rarely matters).

Replacing qemu_isFOO() by g_ascii_isFOO() gets rid of the commonly
unwanted locale dependence.  Each such replacement needs review, no
matter how common "unwanted" may be.

I suspect we'd almost always be better off with g_ascii_isFOO() instead
of qemu_isFOO().  If that's the case, then the few exceptions (if any)
could use standard isFOO(), so we can drop qemu_isFOO().

I'd rather not do that myself globally now due to the "needs review"
part.

Perhaps I should do it just for this file while I touch it anyway.  The
question to ask: should parse_acl_file() obey the locale for whitespace
recognition?

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qemu-bridge-helper.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>> 
>> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
>> index 5396fbfbb6..0d60c07655 100644
>> --- a/qemu-bridge-helper.c
>> +++ b/qemu-bridge-helper.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/if_bridge.h>
>>  #endif
>>  
>> +#include "qemu-common.h"
>>  #include "qemu/queue.h"
>>  
>>  #include "net/tap-linux.h"
>> @@ -75,7 +76,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>>          char *ptr = line;
>>          char *cmd, *arg, *argend;
>>  
>> -        while (isspace(*ptr)) {
>> +        while (qemu_isspace(*ptr)) {
>>              ptr++;
>>          }
>>  
>> @@ -99,12 +100,12 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
>>  
>>          *arg = 0;
>>          arg++;
>> -        while (isspace(*arg)) {
>> +        while (qemu_isspace(*arg)) {
>>              arg++;
>>          }
>>  
>>          argend = arg + strlen(arg);
>> -        while (arg != argend && isspace(*(argend - 1))) {
>> +        while (arg != argend && qemu_isspace(*(argend - 1))) {
>>              argend--;
>>          }
>>          *argend = 0;
>> 

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

[Peter Maydell]

On Mon, 13 May 2019 at 14:21, Markus Armbruster <armbru@redhat.com> wrote:
> Perhaps I should do it just for this file while I touch it anyway.  The
> question to ask: should parse_acl_file() obey the locale for whitespace
> recognition?

I vote for "no".

Q: do we document the format of the ACL file anywhere ?

thanks
-- PMM

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

[Markus Armbruster]

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

> On Mon, 13 May 2019 at 14:21, Markus Armbruster <armbru@redhat.com> wrote:
>> Perhaps I should do it just for this file while I touch it anyway.  The
>> question to ask: should parse_acl_file() obey the locale for whitespace
>> recognition?
>
> I vote for "no".
>
> Q: do we document the format of the ACL file anywhere ?

Support for it was added in commit bdef79a2994, v1.1.  Just code, no
documentation.

Grepping for qemu-bridge-helper finds just qemu-options.hx.  Contains
-help output and some .texi that goes into qemu-doc and the manual page.
None of it mentions how qemu-bridge-helper is run, or the ACL file
feature, let alone what its format might be.

I'm afraid all we have is the commit message.  Which doesn't really
define the file format, it merely gives a bunch of examples.

As far as I can tell, qemu-bridge-helper is for use with -netdev tap and
-netdev bridge.

Both variations of -netdev call net_bridge_run_helper() to run the
helper.  First argument is -netdev parameter "helper", default usually
"$prefix/libexec/qemu-bridge-helper".  Second argument is parameter
"br", default "br0".

If @helper contains space or tab, net_bridge_run_helper() guesses its a
full command, else it guesses its the name of the executable.  Bad
magic.

If it guesses name of executable, it execv()s this executable with
arguments "--use-vnet", "--fd=FD", "--br=@bridge".

If it guesses full command, it appends "--use-vnet --fd=FD", where FD is
the helper's half of the socketpair used to connect QEMU and the helper.
It further appends "--br=@bridge", unless @helper contains "--br=".
More bad magic.

It executes the resulting string with sh -c.  Magic cherry on top.

When the helper fails, netdev creation fails.

The helper we ship with QEMU unconditionally tries to read
"$prefix/etc/bridge.conf".  Fatal error if this file doesn't exist.
Errors in this file are fatal.  Errors in files it includes are not
fatal; instead, the remainder of the erroneous file is ignored.
*Boggle*

As far as I can tell, libvirt runs qemu-bridge-helper itself (Paolo's
commit 2d80fbb14df).  Makes sense, because running QEMU with the
necessary privileges would be unwise, and so would be letting it execute
setuid helpers.  Also bypasses the bad magic in QEMU's
net_bridge_run_helper().

qemu-bridge-helper should have a manual page, and its handling of errors
in ACL include files needs work.  There's probably more; I just glanced
at it.  I'm not volunteering, though.  It lacks a maintainer.  Should we
add it to Jason's "Network device backends"?

-netdev's helper parameter is seriously underdocumented.  Document or
deprecate?

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

[Philippe Mathieu-Daudé]

On 5/14/19 2:18 PM, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On Mon, 13 May 2019 at 14:21, Markus Armbruster <armbru@redhat.com> wrote:
>>> Perhaps I should do it just for this file while I touch it anyway.  The
>>> question to ask: should parse_acl_file() obey the locale for whitespace
>>> recognition?
>>
>> I vote for "no".
>>
>> Q: do we document the format of the ACL file anywhere ?
> 
> Support for it was added in commit bdef79a2994, v1.1.  Just code, no
> documentation.
> 
> Grepping for qemu-bridge-helper finds just qemu-options.hx.  Contains
> -help output and some .texi that goes into qemu-doc and the manual page.
> None of it mentions how qemu-bridge-helper is run, or the ACL file
> feature, let alone what its format might be.
> 
> I'm afraid all we have is the commit message.  Which doesn't really
> define the file format, it merely gives a bunch of examples.
> 
> As far as I can tell, qemu-bridge-helper is for use with -netdev tap and
> -netdev bridge.
> 
> Both variations of -netdev call net_bridge_run_helper() to run the
> helper.  First argument is -netdev parameter "helper", default usually
> "$prefix/libexec/qemu-bridge-helper".  Second argument is parameter
> "br", default "br0".
> 
> If @helper contains space or tab, net_bridge_run_helper() guesses its a
> full command, else it guesses its the name of the executable.  Bad
> magic.
> 
> If it guesses name of executable, it execv()s this executable with
> arguments "--use-vnet", "--fd=FD", "--br=@bridge".
> 
> If it guesses full command, it appends "--use-vnet --fd=FD", where FD is
> the helper's half of the socketpair used to connect QEMU and the helper.
> It further appends "--br=@bridge", unless @helper contains "--br=".
> More bad magic.
> 
> It executes the resulting string with sh -c.  Magic cherry on top.
> 
> When the helper fails, netdev creation fails.
> 
> The helper we ship with QEMU unconditionally tries to read
> "$prefix/etc/bridge.conf".  Fatal error if this file doesn't exist.
> Errors in this file are fatal.  Errors in files it includes are not
> fatal; instead, the remainder of the erroneous file is ignored.
> *Boggle*
> 
> As far as I can tell, libvirt runs qemu-bridge-helper itself (Paolo's
> commit 2d80fbb14df).  Makes sense, because running QEMU with the
> necessary privileges would be unwise, and so would be letting it execute
> setuid helpers.  Also bypasses the bad magic in QEMU's
> net_bridge_run_helper().
> 
> qemu-bridge-helper should have a manual page, and its handling of errors
> in ACL include files needs work.  There's probably more; I just glanced
> at it.  I'm not volunteering, though.  It lacks a maintainer.  Should we
> add it to Jason's "Network device backends"?
> 
> -netdev's helper parameter is seriously underdocumented.  Document or
> deprecate?
> 

I understood the project policy is "deprecate until maintained or
tested"... If not, we might start to think about it :)

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

[Jason Wang]

On 2019/5/14 下午8:18, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On Mon, 13 May 2019 at 14:21, Markus Armbruster <armbru@redhat.com> wrote:
>>> Perhaps I should do it just for this file while I touch it anyway.  The
>>> question to ask: should parse_acl_file() obey the locale for whitespace
>>> recognition?
>> I vote for "no".
>>
>> Q: do we document the format of the ACL file anywhere ?
> Support for it was added in commit bdef79a2994, v1.1.  Just code, no
> documentation.
>
> Grepping for qemu-bridge-helper finds just qemu-options.hx.  Contains
> -help output and some .texi that goes into qemu-doc and the manual page.
> None of it mentions how qemu-bridge-helper is run, or the ACL file
> feature, let alone what its format might be.
>
> I'm afraid all we have is the commit message.  Which doesn't really
> define the file format, it merely gives a bunch of examples.
>
> As far as I can tell, qemu-bridge-helper is for use with -netdev tap and
> -netdev bridge.
>
> Both variations of -netdev call net_bridge_run_helper() to run the
> helper.  First argument is -netdev parameter "helper", default usually
> "$prefix/libexec/qemu-bridge-helper".  Second argument is parameter
> "br", default "br0".
>
> If @helper contains space or tab, net_bridge_run_helper() guesses its a
> full command, else it guesses its the name of the executable.  Bad
> magic.
>
> If it guesses name of executable, it execv()s this executable with
> arguments "--use-vnet", "--fd=FD", "--br=@bridge".
>
> If it guesses full command, it appends "--use-vnet --fd=FD", where FD is
> the helper's half of the socketpair used to connect QEMU and the helper.
> It further appends "--br=@bridge", unless @helper contains "--br=".
> More bad magic.
>
> It executes the resulting string with sh -c.  Magic cherry on top.
>
> When the helper fails, netdev creation fails.
>
> The helper we ship with QEMU unconditionally tries to read
> "$prefix/etc/bridge.conf".  Fatal error if this file doesn't exist.
> Errors in this file are fatal.  Errors in files it includes are not
> fatal; instead, the remainder of the erroneous file is ignored.
> *Boggle*
>
> As far as I can tell, libvirt runs qemu-bridge-helper itself (Paolo's
> commit 2d80fbb14df).  Makes sense, because running QEMU with the
> necessary privileges would be unwise, and so would be letting it execute
> setuid helpers.  Also bypasses the bad magic in QEMU's
> net_bridge_run_helper().


I don't notice this before. Is this only for the convenience of 
development? I guess libvirt should have native support like adding port 
to bridge/OVS without the help any external command or script.


>
> qemu-bridge-helper should have a manual page, and its handling of errors
> in ACL include files needs work.  There's probably more; I just glanced
> at it.  I'm not volunteering, though.  It lacks a maintainer.  Should we
> add it to Jason's "Network device backends"?


Yes.


>
> -netdev's helper parameter is seriously underdocumented.  Document or
> deprecate?


I believe management should only use fd parameter of TAP. If we have 
other, it should be a duplication. So I suggest to deprecate the bridge 
helper and -netdev bridge.

Thanks

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

[Markus Armbruster]

Jason Wang <jasowang@redhat.com> writes:

> On 2019/5/14 下午8:18, Markus Armbruster wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On Mon, 13 May 2019 at 14:21, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Perhaps I should do it just for this file while I touch it anyway.  The
>>>> question to ask: should parse_acl_file() obey the locale for whitespace
>>>> recognition?
>>> I vote for "no".
>>>
>>> Q: do we document the format of the ACL file anywhere ?
>> Support for it was added in commit bdef79a2994, v1.1.  Just code, no
>> documentation.
>>
>> Grepping for qemu-bridge-helper finds just qemu-options.hx.  Contains
>> -help output and some .texi that goes into qemu-doc and the manual page.
>> None of it mentions how qemu-bridge-helper is run, or the ACL file
>> feature, let alone what its format might be.
>>
>> I'm afraid all we have is the commit message.  Which doesn't really
>> define the file format, it merely gives a bunch of examples.
>>
>> As far as I can tell, qemu-bridge-helper is for use with -netdev tap and
>> -netdev bridge.
>>
>> Both variations of -netdev call net_bridge_run_helper() to run the
>> helper.  First argument is -netdev parameter "helper", default usually
>> "$prefix/libexec/qemu-bridge-helper".  Second argument is parameter
>> "br", default "br0".
>>
>> If @helper contains space or tab, net_bridge_run_helper() guesses its a
>> full command, else it guesses its the name of the executable.  Bad
>> magic.
>>
>> If it guesses name of executable, it execv()s this executable with
>> arguments "--use-vnet", "--fd=FD", "--br=@bridge".
>>
>> If it guesses full command, it appends "--use-vnet --fd=FD", where FD is
>> the helper's half of the socketpair used to connect QEMU and the helper.
>> It further appends "--br=@bridge", unless @helper contains "--br=".
>> More bad magic.
>>
>> It executes the resulting string with sh -c.  Magic cherry on top.
>>
>> When the helper fails, netdev creation fails.
>>
>> The helper we ship with QEMU unconditionally tries to read
>> "$prefix/etc/bridge.conf".  Fatal error if this file doesn't exist.
>> Errors in this file are fatal.  Errors in files it includes are not
>> fatal; instead, the remainder of the erroneous file is ignored.
>> *Boggle*
>>
>> As far as I can tell, libvirt runs qemu-bridge-helper itself (Paolo's
>> commit 2d80fbb14df).  Makes sense, because running QEMU with the
>> necessary privileges would be unwise, and so would be letting it execute
>> setuid helpers.  Also bypasses the bad magic in QEMU's
>> net_bridge_run_helper().
>
>
> I don't notice this before. Is this only for the convenience of
> development? I guess libvirt should have native support like adding
> port to bridge/OVS without the help any external command or script.

Commit 2d80fbb14df hints at the reason:

    <source type='bridge'> uses a helper application to do the necessary
    TUN/TAP setup to use an existing network bridge, thus letting
    unprivileged users use TUN/TAP interfaces.
    ~~~~~~~~~~~~~~~~~~

The code confirms:

    /* qemuInterfaceBridgeConnect:
     * @def: the definition of the VM
     * @driver: qemu driver data
     * @net: pointer to the VM's interface description
     * @tapfd: array of file descriptor return value for the new device
     * @tapfdsize: number of file descriptors in @tapfd
     *
---> * Called *only* called if actualType is VIR_DOMAIN_NET_TYPE_NETWORK or
---> * VIR_DOMAIN_NET_TYPE_BRIDGE (i.e. if the connection is made with a tap
     * device connecting to a bridge device)
     */
    int
    qemuInterfaceBridgeConnect(virDomainDefPtr def,
                               virQEMUDriverPtr driver,
                               virDomainNetDefPtr net,
                               int *tapfd,
                               size_t *tapfdSize)
    {
        [...]
--->    if (virQEMUDriverIsPrivileged(driver)) {
            [...]
        } else {
            if (qemuCreateInBridgePortWithHelper(cfg, brname,
                                                 &net->ifname,
                                                 tapfd, tap_create_flags) < 0) {
                virDomainAuditNetDevice(def, net, tunpath, false);
                goto cleanup;
            }
            [...]
        }
        [...]
    }

>> qemu-bridge-helper should have a manual page, and its handling of errors
>> in ACL include files needs work.  There's probably more; I just glanced
>> at it.  I'm not volunteering, though.  It lacks a maintainer.  Should we
>> add it to Jason's "Network device backends"?
>
>
> Yes.
>
>> -netdev's helper parameter is seriously underdocumented.  Document or
>> deprecate?
>
>
> I believe management should only use fd parameter of TAP. If we have
> other, it should be a duplication. So I suggest to deprecate the
> bridge helper and -netdev bridge.

Objections, anyone?

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

[Peter Maydell]

On Wed, 15 May 2019 at 07:34, Markus Armbruster <armbru@redhat.com> wrote:
>
> Jason Wang <jasowang@redhat.com> writes:
>
> > On 2019/5/14 下午8:18, Markus Armbruster wrote:
> >> -netdev's helper parameter is seriously underdocumented.  Document or
> >> deprecate?
> >
> >
> > I believe management should only use fd parameter of TAP. If we have
> > other, it should be a duplication. So I suggest to deprecate the
> > bridge helper and -netdev bridge.
>
> Objections, anyone?

Only the usual "only if we clearly document what the intended
new functionality is and how to convert your setup from an old
command line using the old thing to a new one using the new thing"...

thanks
-- PMM

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

[Daniel P. Berrangé]

On Wed, May 15, 2019 at 08:34:17AM +0200, Markus Armbruster wrote:
> Jason Wang <jasowang@redhat.com> writes:
> 
> > On 2019/5/14 下午8:18, Markus Armbruster wrote:
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >>
> >>> On Mon, 13 May 2019 at 14:21, Markus Armbruster <armbru@redhat.com> wrote:
> >>>> Perhaps I should do it just for this file while I touch it anyway.  The
> >>>> question to ask: should parse_acl_file() obey the locale for whitespace
> >>>> recognition?
> >>> I vote for "no".
> >>>
> >>> Q: do we document the format of the ACL file anywhere ?
> >> Support for it was added in commit bdef79a2994, v1.1.  Just code, no
> >> documentation.
> >>
> >> Grepping for qemu-bridge-helper finds just qemu-options.hx.  Contains
> >> -help output and some .texi that goes into qemu-doc and the manual page.
> >> None of it mentions how qemu-bridge-helper is run, or the ACL file
> >> feature, let alone what its format might be.
> >>
> >> I'm afraid all we have is the commit message.  Which doesn't really
> >> define the file format, it merely gives a bunch of examples.
> >>
> >> As far as I can tell, qemu-bridge-helper is for use with -netdev tap and
> >> -netdev bridge.
> >>
> >> Both variations of -netdev call net_bridge_run_helper() to run the
> >> helper.  First argument is -netdev parameter "helper", default usually
> >> "$prefix/libexec/qemu-bridge-helper".  Second argument is parameter
> >> "br", default "br0".
> >>
> >> If @helper contains space or tab, net_bridge_run_helper() guesses its a
> >> full command, else it guesses its the name of the executable.  Bad
> >> magic.
> >>
> >> If it guesses name of executable, it execv()s this executable with
> >> arguments "--use-vnet", "--fd=FD", "--br=@bridge".
> >>
> >> If it guesses full command, it appends "--use-vnet --fd=FD", where FD is
> >> the helper's half of the socketpair used to connect QEMU and the helper.
> >> It further appends "--br=@bridge", unless @helper contains "--br=".
> >> More bad magic.
> >>
> >> It executes the resulting string with sh -c.  Magic cherry on top.
> >>
> >> When the helper fails, netdev creation fails.
> >>
> >> The helper we ship with QEMU unconditionally tries to read
> >> "$prefix/etc/bridge.conf".  Fatal error if this file doesn't exist.
> >> Errors in this file are fatal.  Errors in files it includes are not
> >> fatal; instead, the remainder of the erroneous file is ignored.
> >> *Boggle*
> >>
> >> As far as I can tell, libvirt runs qemu-bridge-helper itself (Paolo's
> >> commit 2d80fbb14df).  Makes sense, because running QEMU with the
> >> necessary privileges would be unwise, and so would be letting it execute
> >> setuid helpers.  Also bypasses the bad magic in QEMU's
> >> net_bridge_run_helper().
> >
> >
> > I don't notice this before. Is this only for the convenience of
> > development? I guess libvirt should have native support like adding
> > port to bridge/OVS without the help any external command or script.
> 
> Commit 2d80fbb14df hints at the reason:
> 
>     <source type='bridge'> uses a helper application to do the necessary
>     TUN/TAP setup to use an existing network bridge, thus letting
>     unprivileged users use TUN/TAP interfaces.
>     ~~~~~~~~~~~~~~~~~~
> 
> The code confirms:
> 
>     /* qemuInterfaceBridgeConnect:
>      * @def: the definition of the VM
>      * @driver: qemu driver data
>      * @net: pointer to the VM's interface description
>      * @tapfd: array of file descriptor return value for the new device
>      * @tapfdsize: number of file descriptors in @tapfd
>      *
> ---> * Called *only* called if actualType is VIR_DOMAIN_NET_TYPE_NETWORK or
> ---> * VIR_DOMAIN_NET_TYPE_BRIDGE (i.e. if the connection is made with a tap
>      * device connecting to a bridge device)
>      */
>     int
>     qemuInterfaceBridgeConnect(virDomainDefPtr def,
>                                virQEMUDriverPtr driver,
>                                virDomainNetDefPtr net,
>                                int *tapfd,
>                                size_t *tapfdSize)
>     {
>         [...]
> --->    if (virQEMUDriverIsPrivileged(driver)) {
>             [...]
>         } else {
>             if (qemuCreateInBridgePortWithHelper(cfg, brname,
>                                                  &net->ifname,
>                                                  tapfd, tap_create_flags) < 0) {
>                 virDomainAuditNetDevice(def, net, tunpath, false);
>                 goto cleanup;
>             }
>             [...]
>         }
>         [...]
>     }
> 
> >> qemu-bridge-helper should have a manual page, and its handling of errors
> >> in ACL include files needs work.  There's probably more; I just glanced
> >> at it.  I'm not volunteering, though.  It lacks a maintainer.  Should we
> >> add it to Jason's "Network device backends"?
> >
> >
> > Yes.
> >
> >> -netdev's helper parameter is seriously underdocumented.  Document or
> >> deprecate?
> >
> >
> > I believe management should only use fd parameter of TAP. If we have
> > other, it should be a duplication. So I suggest to deprecate the
> > bridge helper and -netdev bridge.
> 
> Objections, anyone?

Libvirt runs the qemu bridge helper command directly, and we have
applications using this functionality.

I'd like libvirt to be able to avoid use of the QEMU bridge helper and
instead have unprivileged libvirt talk to privileged libvirtd to open a
TAP fd on its behalf. If we ever get that done, then libvirt would not
need the qemu bridge helper command anymore.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Paolo Bonzini]

On 15/05/19 08:34, Markus Armbruster wrote:
>>> qemu-bridge-helper should have a manual page, and its handling of errors
>>> in ACL include files needs work.  There's probably more; I just glanced
>>> at it.  I'm not volunteering, though.  It lacks a maintainer.  Should we
>>> add it to Jason's "Network device backends"?
>>
>>
>> Yes.
>>
>>> -netdev's helper parameter is seriously underdocumented.  Document or
>>> deprecate?
>>
>>
>> I believe management should only use fd parameter of TAP. If we have
>> other, it should be a duplication. So I suggest to deprecate the
>> bridge helper and -netdev bridge.
> 
> Objections, anyone?

Yes, your honor. :)  The helper is the only way for unprivileged users
to set up TAP networking, which is basically the only really way to have
*working* network.  It's widely used in the wild, it's self-contained
and the only alternative for users is the S-word (hint, it's five
letters long and ends with LIRP).

However, I have no problem with deprecating the helper argument of
"-netdev tap", which is a useless duplication with "-netdev bridge".

Paolo

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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, May 15, 2019 at 08:34:17AM +0200, Markus Armbruster wrote:
>> Jason Wang <jasowang@redhat.com> writes:
>> 
>> > On 2019/5/14 下午8:18, Markus Armbruster wrote:
>> >> Peter Maydell <peter.maydell@linaro.org> writes:
>> >>
>> >>> On Mon, 13 May 2019 at 14:21, Markus Armbruster <armbru@redhat.com> wrote:
>> >>>> Perhaps I should do it just for this file while I touch it anyway.  The
>> >>>> question to ask: should parse_acl_file() obey the locale for whitespace
>> >>>> recognition?
>> >>> I vote for "no".
>> >>>
>> >>> Q: do we document the format of the ACL file anywhere ?
>> >> Support for it was added in commit bdef79a2994, v1.1.  Just code, no
>> >> documentation.
>> >>
>> >> Grepping for qemu-bridge-helper finds just qemu-options.hx.  Contains
>> >> -help output and some .texi that goes into qemu-doc and the manual page.
>> >> None of it mentions how qemu-bridge-helper is run, or the ACL file
>> >> feature, let alone what its format might be.
>> >>
>> >> I'm afraid all we have is the commit message.  Which doesn't really
>> >> define the file format, it merely gives a bunch of examples.
>> >>
>> >> As far as I can tell, qemu-bridge-helper is for use with -netdev tap and
>> >> -netdev bridge.
>> >>
>> >> Both variations of -netdev call net_bridge_run_helper() to run the
>> >> helper.  First argument is -netdev parameter "helper", default usually
>> >> "$prefix/libexec/qemu-bridge-helper".  Second argument is parameter
>> >> "br", default "br0".
>> >>
>> >> If @helper contains space or tab, net_bridge_run_helper() guesses its a
>> >> full command, else it guesses its the name of the executable.  Bad
>> >> magic.
>> >>
>> >> If it guesses name of executable, it execv()s this executable with
>> >> arguments "--use-vnet", "--fd=FD", "--br=@bridge".
>> >>
>> >> If it guesses full command, it appends "--use-vnet --fd=FD", where FD is
>> >> the helper's half of the socketpair used to connect QEMU and the helper.
>> >> It further appends "--br=@bridge", unless @helper contains "--br=".
>> >> More bad magic.
>> >>
>> >> It executes the resulting string with sh -c.  Magic cherry on top.
>> >>
>> >> When the helper fails, netdev creation fails.
>> >>
>> >> The helper we ship with QEMU unconditionally tries to read
>> >> "$prefix/etc/bridge.conf".  Fatal error if this file doesn't exist.
>> >> Errors in this file are fatal.  Errors in files it includes are not
>> >> fatal; instead, the remainder of the erroneous file is ignored.
>> >> *Boggle*
>> >>
>> >> As far as I can tell, libvirt runs qemu-bridge-helper itself (Paolo's
>> >> commit 2d80fbb14df).  Makes sense, because running QEMU with the
>> >> necessary privileges would be unwise, and so would be letting it execute
>> >> setuid helpers.  Also bypasses the bad magic in QEMU's
>> >> net_bridge_run_helper().
>> >
>> >
>> > I don't notice this before. Is this only for the convenience of
>> > development? I guess libvirt should have native support like adding
>> > port to bridge/OVS without the help any external command or script.
>> 
>> Commit 2d80fbb14df hints at the reason:
>> 
>>     <source type='bridge'> uses a helper application to do the necessary
>>     TUN/TAP setup to use an existing network bridge, thus letting
>>     unprivileged users use TUN/TAP interfaces.
>>     ~~~~~~~~~~~~~~~~~~
>> 
>> The code confirms:
>> 
>>     /* qemuInterfaceBridgeConnect:
>>      * @def: the definition of the VM
>>      * @driver: qemu driver data
>>      * @net: pointer to the VM's interface description
>>      * @tapfd: array of file descriptor return value for the new device
>>      * @tapfdsize: number of file descriptors in @tapfd
>>      *
>> ---> * Called *only* called if actualType is VIR_DOMAIN_NET_TYPE_NETWORK or
>> ---> * VIR_DOMAIN_NET_TYPE_BRIDGE (i.e. if the connection is made with a tap
>>      * device connecting to a bridge device)
>>      */
>>     int
>>     qemuInterfaceBridgeConnect(virDomainDefPtr def,
>>                                virQEMUDriverPtr driver,
>>                                virDomainNetDefPtr net,
>>                                int *tapfd,
>>                                size_t *tapfdSize)
>>     {
>>         [...]
>> --->    if (virQEMUDriverIsPrivileged(driver)) {
>>             [...]
>>         } else {
>>             if (qemuCreateInBridgePortWithHelper(cfg, brname,
>>                                                  &net->ifname,
>>                                                  tapfd, tap_create_flags) < 0) {
>>                 virDomainAuditNetDevice(def, net, tunpath, false);
>>                 goto cleanup;
>>             }
>>             [...]
>>         }
>>         [...]
>>     }
>> 
>> >> qemu-bridge-helper should have a manual page, and its handling of errors
>> >> in ACL include files needs work.  There's probably more; I just glanced
>> >> at it.  I'm not volunteering, though.  It lacks a maintainer.  Should we
>> >> add it to Jason's "Network device backends"?
>> >
>> >
>> > Yes.
>> >
>> >> -netdev's helper parameter is seriously underdocumented.  Document or
>> >> deprecate?
>> >
>> >
>> > I believe management should only use fd parameter of TAP. If we have
>> > other, it should be a duplication. So I suggest to deprecate the
>> > bridge helper and -netdev bridge.
>> 
>> Objections, anyone?
>
> Libvirt runs the qemu bridge helper command directly, and we have
> applications using this functionality.

Specifically, when libvirt lacks the privileges to set up a TAP fd, it
farms out the job to setuid qemu-bridge-helper.  Correct?

> I'd like libvirt to be able to avoid use of the QEMU bridge helper and
> instead have unprivileged libvirt talk to privileged libvirtd to open a
> TAP fd on its behalf. If we ever get that done, then libvirt would not
> need the qemu bridge helper command anymore.

We don't want to deprecate qemu-bridge-helper while libvirt has a
sensible use for it.

We can still deprecate -netdev tap parameter "helper" and -netdev bridge
entirely.

Once they're gone, qemu-bridge-helper wull have no user within QEMU.  We
could discuss moving it to libvirt then, but I doubt it'll be worth the
trouble.

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

[Daniel P. Berrangé]

On Wed, May 15, 2019 at 04:54:04PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Wed, May 15, 2019 at 08:34:17AM +0200, Markus Armbruster wrote:
> >> Jason Wang <jasowang@redhat.com> writes:
> >> 
> >> > On 2019/5/14 下午8:18, Markus Armbruster wrote:
> >> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >> >>
> >> >>> On Mon, 13 May 2019 at 14:21, Markus Armbruster <armbru@redhat.com> wrote:
> >> >>>> Perhaps I should do it just for this file while I touch it anyway.  The
> >> >>>> question to ask: should parse_acl_file() obey the locale for whitespace
> >> >>>> recognition?
> >> >>> I vote for "no".
> >> >>>
> >> >>> Q: do we document the format of the ACL file anywhere ?
> >> >> Support for it was added in commit bdef79a2994, v1.1.  Just code, no
> >> >> documentation.
> >> >>
> >> >> Grepping for qemu-bridge-helper finds just qemu-options.hx.  Contains
> >> >> -help output and some .texi that goes into qemu-doc and the manual page.
> >> >> None of it mentions how qemu-bridge-helper is run, or the ACL file
> >> >> feature, let alone what its format might be.
> >> >>
> >> >> I'm afraid all we have is the commit message.  Which doesn't really
> >> >> define the file format, it merely gives a bunch of examples.
> >> >>
> >> >> As far as I can tell, qemu-bridge-helper is for use with -netdev tap and
> >> >> -netdev bridge.
> >> >>
> >> >> Both variations of -netdev call net_bridge_run_helper() to run the
> >> >> helper.  First argument is -netdev parameter "helper", default usually
> >> >> "$prefix/libexec/qemu-bridge-helper".  Second argument is parameter
> >> >> "br", default "br0".
> >> >>
> >> >> If @helper contains space or tab, net_bridge_run_helper() guesses its a
> >> >> full command, else it guesses its the name of the executable.  Bad
> >> >> magic.
> >> >>
> >> >> If it guesses name of executable, it execv()s this executable with
> >> >> arguments "--use-vnet", "--fd=FD", "--br=@bridge".
> >> >>
> >> >> If it guesses full command, it appends "--use-vnet --fd=FD", where FD is
> >> >> the helper's half of the socketpair used to connect QEMU and the helper.
> >> >> It further appends "--br=@bridge", unless @helper contains "--br=".
> >> >> More bad magic.
> >> >>
> >> >> It executes the resulting string with sh -c.  Magic cherry on top.
> >> >>
> >> >> When the helper fails, netdev creation fails.
> >> >>
> >> >> The helper we ship with QEMU unconditionally tries to read
> >> >> "$prefix/etc/bridge.conf".  Fatal error if this file doesn't exist.
> >> >> Errors in this file are fatal.  Errors in files it includes are not
> >> >> fatal; instead, the remainder of the erroneous file is ignored.
> >> >> *Boggle*
> >> >>
> >> >> As far as I can tell, libvirt runs qemu-bridge-helper itself (Paolo's
> >> >> commit 2d80fbb14df).  Makes sense, because running QEMU with the
> >> >> necessary privileges would be unwise, and so would be letting it execute
> >> >> setuid helpers.  Also bypasses the bad magic in QEMU's
> >> >> net_bridge_run_helper().
> >> >
> >> >
> >> > I don't notice this before. Is this only for the convenience of
> >> > development? I guess libvirt should have native support like adding
> >> > port to bridge/OVS without the help any external command or script.
> >> 
> >> Commit 2d80fbb14df hints at the reason:
> >> 
> >>     <source type='bridge'> uses a helper application to do the necessary
> >>     TUN/TAP setup to use an existing network bridge, thus letting
> >>     unprivileged users use TUN/TAP interfaces.
> >>     ~~~~~~~~~~~~~~~~~~
> >> 
> >> The code confirms:
> >> 
> >>     /* qemuInterfaceBridgeConnect:
> >>      * @def: the definition of the VM
> >>      * @driver: qemu driver data
> >>      * @net: pointer to the VM's interface description
> >>      * @tapfd: array of file descriptor return value for the new device
> >>      * @tapfdsize: number of file descriptors in @tapfd
> >>      *
> >> ---> * Called *only* called if actualType is VIR_DOMAIN_NET_TYPE_NETWORK or
> >> ---> * VIR_DOMAIN_NET_TYPE_BRIDGE (i.e. if the connection is made with a tap
> >>      * device connecting to a bridge device)
> >>      */
> >>     int
> >>     qemuInterfaceBridgeConnect(virDomainDefPtr def,
> >>                                virQEMUDriverPtr driver,
> >>                                virDomainNetDefPtr net,
> >>                                int *tapfd,
> >>                                size_t *tapfdSize)
> >>     {
> >>         [...]
> >> --->    if (virQEMUDriverIsPrivileged(driver)) {
> >>             [...]
> >>         } else {
> >>             if (qemuCreateInBridgePortWithHelper(cfg, brname,
> >>                                                  &net->ifname,
> >>                                                  tapfd, tap_create_flags) < 0) {
> >>                 virDomainAuditNetDevice(def, net, tunpath, false);
> >>                 goto cleanup;
> >>             }
> >>             [...]
> >>         }
> >>         [...]
> >>     }
> >> 
> >> >> qemu-bridge-helper should have a manual page, and its handling of errors
> >> >> in ACL include files needs work.  There's probably more; I just glanced
> >> >> at it.  I'm not volunteering, though.  It lacks a maintainer.  Should we
> >> >> add it to Jason's "Network device backends"?
> >> >
> >> >
> >> > Yes.
> >> >
> >> >> -netdev's helper parameter is seriously underdocumented.  Document or
> >> >> deprecate?
> >> >
> >> >
> >> > I believe management should only use fd parameter of TAP. If we have
> >> > other, it should be a duplication. So I suggest to deprecate the
> >> > bridge helper and -netdev bridge.
> >> 
> >> Objections, anyone?
> >
> > Libvirt runs the qemu bridge helper command directly, and we have
> > applications using this functionality.
> 
> Specifically, when libvirt lacks the privileges to set up a TAP fd, it
> farms out the job to setuid qemu-bridge-helper.  Correct?

Yes, this is for when using libvirt as an unpriv user - typically the
desktop virt use case.

> > I'd like libvirt to be able to avoid use of the QEMU bridge helper and
> > instead have unprivileged libvirt talk to privileged libvirtd to open a
> > TAP fd on its behalf. If we ever get that done, then libvirt would not
> > need the qemu bridge helper command anymore.
> 
> We don't want to deprecate qemu-bridge-helper while libvirt has a
> sensible use for it.
> 
> We can still deprecate -netdev tap parameter "helper" and -netdev bridge
> entirely.

No objection to that.

> Once they're gone, qemu-bridge-helper wull have no user within QEMU.  We
> could discuss moving it to libvirt then, but I doubt it'll be worth the
> trouble.

I'm fine with that either way.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Richard Henderson]

On 5/15/19 7:54 AM, Markus Armbruster wrote:
> We don't want to deprecate qemu-bridge-helper while libvirt has a
> sensible use for it.
> 
> We can still deprecate -netdev tap parameter "helper" and -netdev bridge
> entirely.
> 
> Once they're gone, qemu-bridge-helper wull have no user within QEMU.  We
> could discuss moving it to libvirt then, but I doubt it'll be worth the
> trouble.

At present, one can do reasonable testing of QEMU by itself, without needing a
management layer on top.  If you remove -netdev bridge, then that is no longer
possible.  (I discount slirp as reasonable; things Just Work with bridging.)

I am opposed to requiring libvirt in order to test QEMU.


r~

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Daniel P. Berrangé <berrange@redhat.com> writes:
>
>> On Wed, May 15, 2019 at 08:34:17AM +0200, Markus Armbruster wrote:
>>> Jason Wang <jasowang@redhat.com> writes:
>>> 
>>> > On 2019/5/14 下午8:18, Markus Armbruster wrote:
>>> >> Peter Maydell <peter.maydell@linaro.org> writes:
>>> >>
>>> >>> On Mon, 13 May 2019 at 14:21, Markus Armbruster <armbru@redhat.com> wrote:
>>> >>>> Perhaps I should do it just for this file while I touch it anyway.  The
>>> >>>> question to ask: should parse_acl_file() obey the locale for whitespace
>>> >>>> recognition?
>>> >>> I vote for "no".
>>> >>>
>>> >>> Q: do we document the format of the ACL file anywhere ?
>>> >> Support for it was added in commit bdef79a2994, v1.1.  Just code, no
>>> >> documentation.
>>> >>
>>> >> Grepping for qemu-bridge-helper finds just qemu-options.hx.  Contains
>>> >> -help output and some .texi that goes into qemu-doc and the manual page.
>>> >> None of it mentions how qemu-bridge-helper is run, or the ACL file
>>> >> feature, let alone what its format might be.
>>> >>
>>> >> I'm afraid all we have is the commit message.  Which doesn't really
>>> >> define the file format, it merely gives a bunch of examples.
>>> >>
>>> >> As far as I can tell, qemu-bridge-helper is for use with -netdev tap and
>>> >> -netdev bridge.
>>> >>
>>> >> Both variations of -netdev call net_bridge_run_helper() to run the
>>> >> helper.  First argument is -netdev parameter "helper", default usually
>>> >> "$prefix/libexec/qemu-bridge-helper".  Second argument is parameter
>>> >> "br", default "br0".
>>> >>
>>> >> If @helper contains space or tab, net_bridge_run_helper() guesses its a
>>> >> full command, else it guesses its the name of the executable.  Bad
>>> >> magic.
>>> >>
>>> >> If it guesses name of executable, it execv()s this executable with
>>> >> arguments "--use-vnet", "--fd=FD", "--br=@bridge".
>>> >>
>>> >> If it guesses full command, it appends "--use-vnet --fd=FD", where FD is
>>> >> the helper's half of the socketpair used to connect QEMU and the helper.
>>> >> It further appends "--br=@bridge", unless @helper contains "--br=".
>>> >> More bad magic.
>>> >>
>>> >> It executes the resulting string with sh -c.  Magic cherry on top.
>>> >>
>>> >> When the helper fails, netdev creation fails.
>>> >>
>>> >> The helper we ship with QEMU unconditionally tries to read
>>> >> "$prefix/etc/bridge.conf".  Fatal error if this file doesn't exist.

Correction: $prefix/etc/qemu/bridge.conf

>>> >> Errors in this file are fatal.  Errors in files it includes are not
>>> >> fatal; instead, the remainder of the erroneous file is ignored.
>>> >> *Boggle*
>>> >>
>>> >> As far as I can tell, libvirt runs qemu-bridge-helper itself (Paolo's
>>> >> commit 2d80fbb14df).  Makes sense, because running QEMU with the
>>> >> necessary privileges would be unwise, and so would be letting it execute
>>> >> setuid helpers.  Also bypasses the bad magic in QEMU's
>>> >> net_bridge_run_helper().
>>> >
>>> >
>>> > I don't notice this before. Is this only for the convenience of
>>> > development? I guess libvirt should have native support like adding
>>> > port to bridge/OVS without the help any external command or script.
>>> 
>>> Commit 2d80fbb14df hints at the reason:
>>> 
>>>     <source type='bridge'> uses a helper application to do the necessary
>>>     TUN/TAP setup to use an existing network bridge, thus letting
>>>     unprivileged users use TUN/TAP interfaces.
>>>     ~~~~~~~~~~~~~~~~~~
>>> 
>>> The code confirms:
>>> 
>>>     /* qemuInterfaceBridgeConnect:
>>>      * @def: the definition of the VM
>>>      * @driver: qemu driver data
>>>      * @net: pointer to the VM's interface description
>>>      * @tapfd: array of file descriptor return value for the new device
>>>      * @tapfdsize: number of file descriptors in @tapfd
>>>      *
>>> ---> * Called *only* called if actualType is VIR_DOMAIN_NET_TYPE_NETWORK or
>>> ---> * VIR_DOMAIN_NET_TYPE_BRIDGE (i.e. if the connection is made with a tap
>>>      * device connecting to a bridge device)
>>>      */
>>>     int
>>>     qemuInterfaceBridgeConnect(virDomainDefPtr def,
>>>                                virQEMUDriverPtr driver,
>>>                                virDomainNetDefPtr net,
>>>                                int *tapfd,
>>>                                size_t *tapfdSize)
>>>     {
>>>         [...]
>>> --->    if (virQEMUDriverIsPrivileged(driver)) {
>>>             [...]
>>>         } else {
>>>             if (qemuCreateInBridgePortWithHelper(cfg, brname,
>>>                                                  &net->ifname,
>>>                                                  tapfd, tap_create_flags) < 0) {
>>>                 virDomainAuditNetDevice(def, net, tunpath, false);
>>>                 goto cleanup;
>>>             }
>>>             [...]
>>>         }
>>>         [...]
>>>     }
>>> 
>>> >> qemu-bridge-helper should have a manual page, and its handling of errors
>>> >> in ACL include files needs work.  There's probably more; I just glanced
>>> >> at it.  I'm not volunteering, though.  It lacks a maintainer.  Should we
>>> >> add it to Jason's "Network device backends"?
>>> >
>>> >
>>> > Yes.
>>> >
>>> >> -netdev's helper parameter is seriously underdocumented.  Document or
>>> >> deprecate?
>>> >
>>> >
>>> > I believe management should only use fd parameter of TAP. If we have
>>> > other, it should be a duplication. So I suggest to deprecate the
>>> > bridge helper and -netdev bridge.
>>> 
>>> Objections, anyone?
>>
>> Libvirt runs the qemu bridge helper command directly, and we have
>> applications using this functionality.
>
> Specifically, when libvirt lacks the privileges to set up a TAP fd, it
> farms out the job to setuid qemu-bridge-helper.  Correct?
>
>> I'd like libvirt to be able to avoid use of the QEMU bridge helper and
>> instead have unprivileged libvirt talk to privileged libvirtd to open a
>> TAP fd on its behalf. If we ever get that done, then libvirt would not
>> need the qemu bridge helper command anymore.
>
> We don't want to deprecate qemu-bridge-helper while libvirt has a
> sensible use for it.
>
> We can still deprecate -netdev tap parameter "helper" and -netdev bridge
> entirely.

Paolo and Richard objected to deprecating -netdev bridge, so that's out,
too.

Proposal:

1. Add qemu-bridge-helper.c to Jason's "Network device backends"

2. Deprecate -netdev tap parameter "helper"

3. Improve documentation of -netdev bridge

4. Create a manual page for qemu-bridge-helper that also covers
   /etc/qemu/bridge.conf.

5. Fix the nutty error handling in parse_acl_file()

Objections?

[...]

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

[Richard Henderson]

On 5/15/19 9:55 AM, Markus Armbruster wrote:
> Proposal:
> 
> 1. Add qemu-bridge-helper.c to Jason's "Network device backends"
> 
> 2. Deprecate -netdev tap parameter "helper"
> 
> 3. Improve documentation of -netdev bridge
> 
> 4. Create a manual page for qemu-bridge-helper that also covers
>    /etc/qemu/bridge.conf.
> 
> 5. Fix the nutty error handling in parse_acl_file()

LGTM.  Thanks!


r~

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

[Jason Wang]

On 2019/5/15 下午9:35, Paolo Bonzini wrote:
> On 15/05/19 08:34, Markus Armbruster wrote:
>>>> qemu-bridge-helper should have a manual page, and its handling of errors
>>>> in ACL include files needs work.  There's probably more; I just glanced
>>>> at it.  I'm not volunteering, though.  It lacks a maintainer.  Should we
>>>> add it to Jason's "Network device backends"?
>>>
>>> Yes.
>>>
>>>> -netdev's helper parameter is seriously underdocumented.  Document or
>>>> deprecate?
>>>
>>> I believe management should only use fd parameter of TAP. If we have
>>> other, it should be a duplication. So I suggest to deprecate the
>>> bridge helper and -netdev bridge.
>> Objections, anyone?
> Yes, your honor. :)  The helper is the only way for unprivileged users
> to set up TAP networking, which is basically the only really way to have
> *working* network.  It's widely used in the wild, it's self-contained
> and the only alternative for users is the S-word (hint, it's five
> letters long and ends with LIRP).


The issue is it can't deal with e.g vhost-net and multiqueue. We can 
have a simple privileged launcher to do network configuration and pass 
the fds to unprivileged qemu.

Thanks


>
> However, I have no problem with deprecating the helper argument of
> "-netdev tap", which is a useless duplication with "-netdev bridge".
>
> Paolo
>

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

[Paolo Bonzini]

On 17/05/19 06:35, Jason Wang wrote:
>> Yes, your honor. :)  The helper is the only way for unprivileged users
>> to set up TAP networking, which is basically the only really way to have
>> *working* network.  It's widely used in the wild, it's self-contained
>> and the only alternative for users is the S-word (hint, it's five
>> letters long and ends with LIRP).
> 
> The issue is it can't deal with e.g vhost-net and multiqueue.


vhost-net does work with qemu-bridge-helper, the problem is that distros
set its permissions to 600 and there is really no reason for that.

There is also no reason why multiqueue shouldn't work with
qemu-bridge-helper, all it would take is a new command line argument
--num-queues probably.

Paolo

> We can
> have a simple privileged launcher to do network configuration and pass
> the fds to unprivileged qemu.