All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/5] misc set of fixes for warnings under GCC 9
@ 2019-04-12 12:16 ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-04-12 12:16 UTC (permalink / raw)
  To: qemu-devel
  Cc: Laurent Vivier, Riku Voipio, Gerd Hoffmann, Daniel P. Berrangé

This series addresses all the warnings I see when building on Fedora 30
x86_64 with GCC 9 (gcc-9.0.1-0.10.fc30.x86_64).

Changed in v2:

 - Dropped patches mistakenly included due to not correctly rebasing
 - Dropped s390x patches already merged
 - Dropped usb-mtp patches which need much more work to be correctly
   handling untrustworthy guest data.

Daniel P. Berrangé (5):
  linux-user: avoid string truncation warnings in uname field copying
  linux-user: avoid string truncation warnings in elf field copying
  sockets: avoid string truncation warnings when copying UNIX path
  hw/usb: avoid format truncation warning when formatting port name
  qxl: avoid unaligned pointer reads/writes

 hw/display/qxl.c     | 55 +++++++++++++++++++-------------------------
 hw/usb/hcd-xhci.c    |  2 ++
 linux-user/elfload.c | 10 ++++----
 linux-user/uname.c   |  2 +-
 util/qemu-sockets.c  | 12 ++++++----
 5 files changed, 39 insertions(+), 42 deletions(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Qemu-devel] [PATCH v2 0/5] misc set of fixes for warnings under GCC 9
@ 2019-04-12 12:16 ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-04-12 12:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: Riku Voipio, Laurent Vivier, Gerd Hoffmann

This series addresses all the warnings I see when building on Fedora 30
x86_64 with GCC 9 (gcc-9.0.1-0.10.fc30.x86_64).

Changed in v2:

 - Dropped patches mistakenly included due to not correctly rebasing
 - Dropped s390x patches already merged
 - Dropped usb-mtp patches which need much more work to be correctly
   handling untrustworthy guest data.

Daniel P. Berrangé (5):
  linux-user: avoid string truncation warnings in uname field copying
  linux-user: avoid string truncation warnings in elf field copying
  sockets: avoid string truncation warnings when copying UNIX path
  hw/usb: avoid format truncation warning when formatting port name
  qxl: avoid unaligned pointer reads/writes

 hw/display/qxl.c     | 55 +++++++++++++++++++-------------------------
 hw/usb/hcd-xhci.c    |  2 ++
 linux-user/elfload.c | 10 ++++----
 linux-user/uname.c   |  2 +-
 util/qemu-sockets.c  | 12 ++++++----
 5 files changed, 39 insertions(+), 42 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Qemu-devel] [PATCH v2 1/5] linux-user: avoid string truncation warnings in uname field copying
@ 2019-04-12 12:16   ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-04-12 12:16 UTC (permalink / raw)
  To: qemu-devel
  Cc: Laurent Vivier, Riku Voipio, Gerd Hoffmann, Daniel P. Berrangé

In file included from /usr/include/string.h:494,
                 from include/qemu/osdep.h:101,
                 from linux-user/uname.c:20:
In function ‘strncpy’,
    inlined from ‘sys_uname’ at linux-user/uname.c:94:3:
/usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ output may be truncated copying 64 bytes from a string of length 64 [-Wstringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

We don't care where the NUL terminator in the original uname
field was. It suffices to copy the entire original field and
simply force a NUL terminator at the end of the new field.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 linux-user/uname.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/uname.c b/linux-user/uname.c
index 313b79dbad..3dff33effe 100644
--- a/linux-user/uname.c
+++ b/linux-user/uname.c
@@ -73,7 +73,7 @@ const char *cpu_to_uname_machine(void *cpu_env)
 #define COPY_UTSNAME_FIELD(dest, src) \
   do { \
       /* __NEW_UTS_LEN doesn't include terminating null */ \
-      (void) strncpy((dest), (src), __NEW_UTS_LEN); \
+      memcpy((dest), (src), MIN(sizeof(src), __NEW_UTS_LEN));   \
       (dest)[__NEW_UTS_LEN] = '\0'; \
   } while (0)
 
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [Qemu-devel] [PATCH v2 1/5] linux-user: avoid string truncation warnings in uname field copying
@ 2019-04-12 12:16   ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-04-12 12:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: Riku Voipio, Laurent Vivier, Gerd Hoffmann

In file included from /usr/include/string.h:494,
                 from include/qemu/osdep.h:101,
                 from linux-user/uname.c:20:
In function ‘strncpy’,
    inlined from ‘sys_uname’ at linux-user/uname.c:94:3:
/usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ output may be truncated copying 64 bytes from a string of length 64 [-Wstringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

We don't care where the NUL terminator in the original uname
field was. It suffices to copy the entire original field and
simply force a NUL terminator at the end of the new field.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 linux-user/uname.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/uname.c b/linux-user/uname.c
index 313b79dbad..3dff33effe 100644
--- a/linux-user/uname.c
+++ b/linux-user/uname.c
@@ -73,7 +73,7 @@ const char *cpu_to_uname_machine(void *cpu_env)
 #define COPY_UTSNAME_FIELD(dest, src) \
   do { \
       /* __NEW_UTS_LEN doesn't include terminating null */ \
-      (void) strncpy((dest), (src), __NEW_UTS_LEN); \
+      memcpy((dest), (src), MIN(sizeof(src), __NEW_UTS_LEN));   \
       (dest)[__NEW_UTS_LEN] = '\0'; \
   } while (0)
 
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [Qemu-devel] [PATCH v2 2/5] linux-user: avoid string truncation warnings in elf field copying
@ 2019-04-12 12:16   ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-04-12 12:16 UTC (permalink / raw)
  To: qemu-devel
  Cc: Laurent Vivier, Riku Voipio, Gerd Hoffmann, Daniel P. Berrangé

In file included from /usr/include/string.h:494,
                 from include/qemu/osdep.h:101,
                 from linux-user/elfload.c:2:
In function ‘strncpy’,
    inlined from ‘fill_psinfo’ at linux-user/elfload.c:3208:12,
    inlined from ‘fill_note_info’ at linux-user/elfload.c:3390:5,
    inlined from ‘elf_core_dump’ at linux-user/elfload.c:3539:9:
/usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 16 equals destination size [-Wstringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

We don't require the field to be NUL terminated, so can just
copy the lower of the string length and the target field size
using memcpy.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 linux-user/elfload.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index c1a26021f8..caa060f7b7 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -3180,6 +3180,7 @@ static int fill_psinfo(struct target_elf_prpsinfo *psinfo, const TaskState *ts)
 {
     char *base_filename;
     unsigned int i, len;
+    size_t pathlen;
 
     (void) memset(psinfo, 0, sizeof (*psinfo));
 
@@ -3201,12 +3202,9 @@ static int fill_psinfo(struct target_elf_prpsinfo *psinfo, const TaskState *ts)
     psinfo->pr_gid = getgid();
 
     base_filename = g_path_get_basename(ts->bprm->filename);
-    /*
-     * Using strncpy here is fine: at max-length,
-     * this field is not NUL-terminated.
-     */
-    (void) strncpy(psinfo->pr_fname, base_filename,
-                   sizeof(psinfo->pr_fname));
+    pathlen = strlen(base_filename) + 1;
+    pathlen = MIN(pathlen, sizeof(psinfo->pr_fname));
+    memcpy(psinfo->pr_fname, base_filename, pathlen);
 
     g_free(base_filename);
     bswap_psinfo(psinfo);
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [Qemu-devel] [PATCH v2 2/5] linux-user: avoid string truncation warnings in elf field copying
@ 2019-04-12 12:16   ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-04-12 12:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: Riku Voipio, Laurent Vivier, Gerd Hoffmann

In file included from /usr/include/string.h:494,
                 from include/qemu/osdep.h:101,
                 from linux-user/elfload.c:2:
In function ‘strncpy’,
    inlined from ‘fill_psinfo’ at linux-user/elfload.c:3208:12,
    inlined from ‘fill_note_info’ at linux-user/elfload.c:3390:5,
    inlined from ‘elf_core_dump’ at linux-user/elfload.c:3539:9:
/usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 16 equals destination size [-Wstringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

We don't require the field to be NUL terminated, so can just
copy the lower of the string length and the target field size
using memcpy.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 linux-user/elfload.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index c1a26021f8..caa060f7b7 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -3180,6 +3180,7 @@ static int fill_psinfo(struct target_elf_prpsinfo *psinfo, const TaskState *ts)
 {
     char *base_filename;
     unsigned int i, len;
+    size_t pathlen;
 
     (void) memset(psinfo, 0, sizeof (*psinfo));
 
@@ -3201,12 +3202,9 @@ static int fill_psinfo(struct target_elf_prpsinfo *psinfo, const TaskState *ts)
     psinfo->pr_gid = getgid();
 
     base_filename = g_path_get_basename(ts->bprm->filename);
-    /*
-     * Using strncpy here is fine: at max-length,
-     * this field is not NUL-terminated.
-     */
-    (void) strncpy(psinfo->pr_fname, base_filename,
-                   sizeof(psinfo->pr_fname));
+    pathlen = strlen(base_filename) + 1;
+    pathlen = MIN(pathlen, sizeof(psinfo->pr_fname));
+    memcpy(psinfo->pr_fname, base_filename, pathlen);
 
     g_free(base_filename);
     bswap_psinfo(psinfo);
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path
@ 2019-04-12 12:16   ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-04-12 12:16 UTC (permalink / raw)
  To: qemu-devel
  Cc: Laurent Vivier, Riku Voipio, Gerd Hoffmann, Daniel P. Berrangé

In file included from /usr/include/string.h:494,
                 from include/qemu/osdep.h:101,
                 from util/qemu-sockets.c:18:
In function ‘strncpy’,
    inlined from ‘unix_connect_saddr.isra.0’ at util/qemu-sockets.c:925:5:
/usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 108 equals destination size [-Wstringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘strncpy’,
    inlined from ‘unix_listen_saddr.isra.0’ at util/qemu-sockets.c:880:5:
/usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 108 equals destination size [-Wstringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

We are already validating the UNIX socket path length earlier in
the functions. If we save this string length when we first check
it, then we can simply use memcpy instead of strcpy later, avoiding
the gcc truncation warnings.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/qemu-sockets.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 9705051690..ba6335e71a 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -830,6 +830,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
     int sock, fd;
     char *pathbuf = NULL;
     const char *path;
+    size_t pathlen;
 
     sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
@@ -845,7 +846,8 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
         path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX", tmpdir);
     }
 
-    if (strlen(path) > sizeof(un.sun_path)) {
+    pathlen = strlen(path);
+    if (pathlen > sizeof(un.sun_path)) {
         error_setg(errp, "UNIX socket path '%s' is too long", path);
         error_append_hint(errp, "Path must be less than %zu bytes\n",
                           sizeof(un.sun_path));
@@ -877,7 +879,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
 
     memset(&un, 0, sizeof(un));
     un.sun_family = AF_UNIX;
-    strncpy(un.sun_path, path, sizeof(un.sun_path));
+    memcpy(un.sun_path, path, pathlen);
 
     if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
         error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
@@ -901,6 +903,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
 {
     struct sockaddr_un un;
     int sock, rc;
+    size_t pathlen;
 
     if (saddr->path == NULL) {
         error_setg(errp, "unix connect: no path specified");
@@ -913,7 +916,8 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
         return -1;
     }
 
-    if (strlen(saddr->path) > sizeof(un.sun_path)) {
+    pathlen = strlen(saddr->path);
+    if (pathlen > sizeof(un.sun_path)) {
         error_setg(errp, "UNIX socket path '%s' is too long", saddr->path);
         error_append_hint(errp, "Path must be less than %zu bytes\n",
                           sizeof(un.sun_path));
@@ -922,7 +926,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
 
     memset(&un, 0, sizeof(un));
     un.sun_family = AF_UNIX;
-    strncpy(un.sun_path, saddr->path, sizeof(un.sun_path));
+    memcpy(un.sun_path, saddr->path, pathlen);
 
     /* connect to peer */
     do {
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path
@ 2019-04-12 12:16   ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-04-12 12:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: Riku Voipio, Laurent Vivier, Gerd Hoffmann

In file included from /usr/include/string.h:494,
                 from include/qemu/osdep.h:101,
                 from util/qemu-sockets.c:18:
In function ‘strncpy’,
    inlined from ‘unix_connect_saddr.isra.0’ at util/qemu-sockets.c:925:5:
/usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 108 equals destination size [-Wstringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘strncpy’,
    inlined from ‘unix_listen_saddr.isra.0’ at util/qemu-sockets.c:880:5:
/usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 108 equals destination size [-Wstringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

We are already validating the UNIX socket path length earlier in
the functions. If we save this string length when we first check
it, then we can simply use memcpy instead of strcpy later, avoiding
the gcc truncation warnings.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 util/qemu-sockets.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 9705051690..ba6335e71a 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -830,6 +830,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
     int sock, fd;
     char *pathbuf = NULL;
     const char *path;
+    size_t pathlen;
 
     sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
@@ -845,7 +846,8 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
         path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX", tmpdir);
     }
 
-    if (strlen(path) > sizeof(un.sun_path)) {
+    pathlen = strlen(path);
+    if (pathlen > sizeof(un.sun_path)) {
         error_setg(errp, "UNIX socket path '%s' is too long", path);
         error_append_hint(errp, "Path must be less than %zu bytes\n",
                           sizeof(un.sun_path));
@@ -877,7 +879,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
 
     memset(&un, 0, sizeof(un));
     un.sun_family = AF_UNIX;
-    strncpy(un.sun_path, path, sizeof(un.sun_path));
+    memcpy(un.sun_path, path, pathlen);
 
     if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
         error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
@@ -901,6 +903,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
 {
     struct sockaddr_un un;
     int sock, rc;
+    size_t pathlen;
 
     if (saddr->path == NULL) {
         error_setg(errp, "unix connect: no path specified");
@@ -913,7 +916,8 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
         return -1;
     }
 
-    if (strlen(saddr->path) > sizeof(un.sun_path)) {
+    pathlen = strlen(saddr->path);
+    if (pathlen > sizeof(un.sun_path)) {
         error_setg(errp, "UNIX socket path '%s' is too long", saddr->path);
         error_append_hint(errp, "Path must be less than %zu bytes\n",
                           sizeof(un.sun_path));
@@ -922,7 +926,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
 
     memset(&un, 0, sizeof(un));
     un.sun_family = AF_UNIX;
-    strncpy(un.sun_path, saddr->path, sizeof(un.sun_path));
+    memcpy(un.sun_path, saddr->path, pathlen);
 
     /* connect to peer */
     do {
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [Qemu-devel] [PATCH v2 4/5] hw/usb: avoid format truncation warning when formatting port name
@ 2019-04-12 12:16   ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-04-12 12:16 UTC (permalink / raw)
  To: qemu-devel
  Cc: Laurent Vivier, Riku Voipio, Gerd Hoffmann, Daniel P. Berrangé

hw/usb/hcd-xhci.c: In function ‘usb_xhci_realize’:
hw/usb/hcd-xhci.c:3339:66: warning: ‘%d’ directive output may be truncated writing between 1 and 10 bytes into a region of size 5 [-Wformat-trunca\
tion=]
 3339 |             snprintf(port->name, sizeof(port->name), "usb2 port #%d", i+1);
      |                                                                  ^~
hw/usb/hcd-xhci.c:3339:54: note: directive argument in the range [1, 2147483647]
 3339 |             snprintf(port->name, sizeof(port->name), "usb2 port #%d", i+1);
      |                                                      ^~~~~~~~~~~~~~~

The xhci code formats the port name into a fixed length
buffer which is only large enough to hold port numbers
upto 5 digits in decimal representation. We're never
going to have a port number that large, so aserting the
port number is sensible is sufficient to tell GCC the
formatted string won't be truncated.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/usb/hcd-xhci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index ec28bee319..7222f9b1af 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3336,6 +3336,7 @@ static void usb_xhci_init(XHCIState *xhci)
                 USB_SPEED_MASK_LOW  |
                 USB_SPEED_MASK_FULL |
                 USB_SPEED_MASK_HIGH;
+            assert(i < MAXPORTS);
             snprintf(port->name, sizeof(port->name), "usb2 port #%d", i+1);
             speedmask |= port->speedmask;
         }
@@ -3349,6 +3350,7 @@ static void usb_xhci_init(XHCIState *xhci)
             }
             port->uport = &xhci->uports[i];
             port->speedmask = USB_SPEED_MASK_SUPER;
+            assert(i < MAXPORTS);
             snprintf(port->name, sizeof(port->name), "usb3 port #%d", i+1);
             speedmask |= port->speedmask;
         }
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [Qemu-devel] [PATCH v2 4/5] hw/usb: avoid format truncation warning when formatting port name
@ 2019-04-12 12:16   ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-04-12 12:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: Riku Voipio, Laurent Vivier, Gerd Hoffmann

hw/usb/hcd-xhci.c: In function ‘usb_xhci_realize’:
hw/usb/hcd-xhci.c:3339:66: warning: ‘%d’ directive output may be truncated writing between 1 and 10 bytes into a region of size 5 [-Wformat-trunca\
tion=]
 3339 |             snprintf(port->name, sizeof(port->name), "usb2 port #%d", i+1);
      |                                                                  ^~
hw/usb/hcd-xhci.c:3339:54: note: directive argument in the range [1, 2147483647]
 3339 |             snprintf(port->name, sizeof(port->name), "usb2 port #%d", i+1);
      |                                                      ^~~~~~~~~~~~~~~

The xhci code formats the port name into a fixed length
buffer which is only large enough to hold port numbers
upto 5 digits in decimal representation. We're never
going to have a port number that large, so aserting the
port number is sensible is sufficient to tell GCC the
formatted string won't be truncated.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/usb/hcd-xhci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index ec28bee319..7222f9b1af 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3336,6 +3336,7 @@ static void usb_xhci_init(XHCIState *xhci)
                 USB_SPEED_MASK_LOW  |
                 USB_SPEED_MASK_FULL |
                 USB_SPEED_MASK_HIGH;
+            assert(i < MAXPORTS);
             snprintf(port->name, sizeof(port->name), "usb2 port #%d", i+1);
             speedmask |= port->speedmask;
         }
@@ -3349,6 +3350,7 @@ static void usb_xhci_init(XHCIState *xhci)
             }
             port->uport = &xhci->uports[i];
             port->speedmask = USB_SPEED_MASK_SUPER;
+            assert(i < MAXPORTS);
             snprintf(port->name, sizeof(port->name), "usb3 port #%d", i+1);
             speedmask |= port->speedmask;
         }
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes
@ 2019-04-12 12:16   ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-04-12 12:16 UTC (permalink / raw)
  To: qemu-devel
  Cc: Laurent Vivier, Riku Voipio, Gerd Hoffmann, Daniel P. Berrangé

The SPICE_RING_PROD_ITEM() macro is initializing a local
'uint64_t *' variable to point to the 'el' field inside
the QXLReleaseRing struct. This uint64_t field is not
guaranteed aligned as the struct is packed.

Code should not take the address of fields within a
packed struct. Changing the SPICE_RING_PROD_ITEM()
macro to avoid taking the address of the field is
impractical. It is clearer to just remove the macro
and inline its functionality in the three call sites
that need it.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/display/qxl.c | 55 +++++++++++++++++++++---------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index c8ce5781e0..5c38e6e906 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -33,24 +33,6 @@
 
 #include "qxl.h"
 
-/*
- * NOTE: SPICE_RING_PROD_ITEM accesses memory on the pci bar and as
- * such can be changed by the guest, so to avoid a guest trigerrable
- * abort we just qxl_set_guest_bug and set the return to NULL. Still
- * it may happen as a result of emulator bug as well.
- */
-#undef SPICE_RING_PROD_ITEM
-#define SPICE_RING_PROD_ITEM(qxl, r, ret) {                             \
-        uint32_t prod = (r)->prod & SPICE_RING_INDEX_MASK(r);           \
-        if (prod >= ARRAY_SIZE((r)->items)) {                           \
-            qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \
-                          "%u >= %zu", prod, ARRAY_SIZE((r)->items));   \
-            ret = NULL;                                                 \
-        } else {                                                        \
-            ret = &(r)->items[prod].el;                                 \
-        }                                                               \
-    }
-
 #undef SPICE_RING_CONS_ITEM
 #define SPICE_RING_CONS_ITEM(qxl, r, ret) {                             \
         uint32_t cons = (r)->cons & SPICE_RING_INDEX_MASK(r);           \
@@ -414,7 +396,8 @@ static void init_qxl_rom(PCIQXLDevice *d)
 static void init_qxl_ram(PCIQXLDevice *d)
 {
     uint8_t *buf;
-    uint64_t *item;
+    uint32_t prod;
+    QXLReleaseRing *ring;
 
     buf = d->vga.vram_ptr;
     d->ram = (QXLRam *)(buf + le32_to_cpu(d->shadow_rom.ram_header_offset));
@@ -426,9 +409,12 @@ static void init_qxl_ram(PCIQXLDevice *d)
     SPICE_RING_INIT(&d->ram->cmd_ring);
     SPICE_RING_INIT(&d->ram->cursor_ring);
     SPICE_RING_INIT(&d->ram->release_ring);
-    SPICE_RING_PROD_ITEM(d, &d->ram->release_ring, item);
-    assert(item);
-    *item = 0;
+
+    ring = &d->ram->release_ring;
+    prod = ring->prod & SPICE_RING_INDEX_MASK(ring);
+    assert(prod < ARRAY_SIZE(ring->items));
+    ring->items[prod].el = 0;
+
     qxl_ring_set_dirty(d);
 }
 
@@ -732,7 +718,7 @@ static int interface_req_cmd_notification(QXLInstance *sin)
 static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
 {
     QXLReleaseRing *ring = &d->ram->release_ring;
-    uint64_t *item;
+    uint32_t prod;
     int notify;
 
 #define QXL_FREE_BUNCH_SIZE 32
@@ -759,11 +745,15 @@ static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
     if (notify) {
         qxl_send_events(d, QXL_INTERRUPT_DISPLAY);
     }
-    SPICE_RING_PROD_ITEM(d, ring, item);
-    if (!item) {
+
+    ring = &d->ram->release_ring;
+    prod = ring->prod & SPICE_RING_INDEX_MASK(ring);
+    if (prod >= ARRAY_SIZE(ring->items)) {
+        qxl_set_guest_bug(d, "SPICE_RING_PROD_ITEM indices mismatch "
+                          "%u >= %zu", prod, ARRAY_SIZE(ring->items));
         return;
     }
-    *item = 0;
+    ring->items[prod].el = 0;
     d->num_free_res = 0;
     d->last_release = NULL;
     qxl_ring_set_dirty(d);
@@ -775,7 +765,8 @@ static void interface_release_resource(QXLInstance *sin,
 {
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
     QXLReleaseRing *ring;
-    uint64_t *item, id;
+    uint32_t prod;
+    uint64_t id;
 
     if (ext.group_id == MEMSLOT_GROUP_HOST) {
         /* host group -> vga mode update request */
@@ -792,16 +783,18 @@ static void interface_release_resource(QXLInstance *sin,
      * pci bar 0, $command.release_info
      */
     ring = &qxl->ram->release_ring;
-    SPICE_RING_PROD_ITEM(qxl, ring, item);
-    if (!item) {
+    prod = ring->prod & SPICE_RING_INDEX_MASK(ring);
+    if (prod >= ARRAY_SIZE(ring->items)) {
+        qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch "
+                          "%u >= %zu", prod, ARRAY_SIZE(ring->items));
         return;
     }
-    if (*item == 0) {
+    if (ring->items[prod].el == 0) {
         /* stick head into the ring */
         id = ext.info->id;
         ext.info->next = 0;
         qxl_ram_set_dirty(qxl, &ext.info->next);
-        *item = id;
+        ring->items[prod].el = id;
         qxl_ring_set_dirty(qxl);
     } else {
         /* append item to the list */
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes
@ 2019-04-12 12:16   ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-04-12 12:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: Riku Voipio, Laurent Vivier, Gerd Hoffmann

The SPICE_RING_PROD_ITEM() macro is initializing a local
'uint64_t *' variable to point to the 'el' field inside
the QXLReleaseRing struct. This uint64_t field is not
guaranteed aligned as the struct is packed.

Code should not take the address of fields within a
packed struct. Changing the SPICE_RING_PROD_ITEM()
macro to avoid taking the address of the field is
impractical. It is clearer to just remove the macro
and inline its functionality in the three call sites
that need it.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/display/qxl.c | 55 +++++++++++++++++++++---------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index c8ce5781e0..5c38e6e906 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -33,24 +33,6 @@
 
 #include "qxl.h"
 
-/*
- * NOTE: SPICE_RING_PROD_ITEM accesses memory on the pci bar and as
- * such can be changed by the guest, so to avoid a guest trigerrable
- * abort we just qxl_set_guest_bug and set the return to NULL. Still
- * it may happen as a result of emulator bug as well.
- */
-#undef SPICE_RING_PROD_ITEM
-#define SPICE_RING_PROD_ITEM(qxl, r, ret) {                             \
-        uint32_t prod = (r)->prod & SPICE_RING_INDEX_MASK(r);           \
-        if (prod >= ARRAY_SIZE((r)->items)) {                           \
-            qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch " \
-                          "%u >= %zu", prod, ARRAY_SIZE((r)->items));   \
-            ret = NULL;                                                 \
-        } else {                                                        \
-            ret = &(r)->items[prod].el;                                 \
-        }                                                               \
-    }
-
 #undef SPICE_RING_CONS_ITEM
 #define SPICE_RING_CONS_ITEM(qxl, r, ret) {                             \
         uint32_t cons = (r)->cons & SPICE_RING_INDEX_MASK(r);           \
@@ -414,7 +396,8 @@ static void init_qxl_rom(PCIQXLDevice *d)
 static void init_qxl_ram(PCIQXLDevice *d)
 {
     uint8_t *buf;
-    uint64_t *item;
+    uint32_t prod;
+    QXLReleaseRing *ring;
 
     buf = d->vga.vram_ptr;
     d->ram = (QXLRam *)(buf + le32_to_cpu(d->shadow_rom.ram_header_offset));
@@ -426,9 +409,12 @@ static void init_qxl_ram(PCIQXLDevice *d)
     SPICE_RING_INIT(&d->ram->cmd_ring);
     SPICE_RING_INIT(&d->ram->cursor_ring);
     SPICE_RING_INIT(&d->ram->release_ring);
-    SPICE_RING_PROD_ITEM(d, &d->ram->release_ring, item);
-    assert(item);
-    *item = 0;
+
+    ring = &d->ram->release_ring;
+    prod = ring->prod & SPICE_RING_INDEX_MASK(ring);
+    assert(prod < ARRAY_SIZE(ring->items));
+    ring->items[prod].el = 0;
+
     qxl_ring_set_dirty(d);
 }
 
@@ -732,7 +718,7 @@ static int interface_req_cmd_notification(QXLInstance *sin)
 static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
 {
     QXLReleaseRing *ring = &d->ram->release_ring;
-    uint64_t *item;
+    uint32_t prod;
     int notify;
 
 #define QXL_FREE_BUNCH_SIZE 32
@@ -759,11 +745,15 @@ static inline void qxl_push_free_res(PCIQXLDevice *d, int flush)
     if (notify) {
         qxl_send_events(d, QXL_INTERRUPT_DISPLAY);
     }
-    SPICE_RING_PROD_ITEM(d, ring, item);
-    if (!item) {
+
+    ring = &d->ram->release_ring;
+    prod = ring->prod & SPICE_RING_INDEX_MASK(ring);
+    if (prod >= ARRAY_SIZE(ring->items)) {
+        qxl_set_guest_bug(d, "SPICE_RING_PROD_ITEM indices mismatch "
+                          "%u >= %zu", prod, ARRAY_SIZE(ring->items));
         return;
     }
-    *item = 0;
+    ring->items[prod].el = 0;
     d->num_free_res = 0;
     d->last_release = NULL;
     qxl_ring_set_dirty(d);
@@ -775,7 +765,8 @@ static void interface_release_resource(QXLInstance *sin,
 {
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
     QXLReleaseRing *ring;
-    uint64_t *item, id;
+    uint32_t prod;
+    uint64_t id;
 
     if (ext.group_id == MEMSLOT_GROUP_HOST) {
         /* host group -> vga mode update request */
@@ -792,16 +783,18 @@ static void interface_release_resource(QXLInstance *sin,
      * pci bar 0, $command.release_info
      */
     ring = &qxl->ram->release_ring;
-    SPICE_RING_PROD_ITEM(qxl, ring, item);
-    if (!item) {
+    prod = ring->prod & SPICE_RING_INDEX_MASK(ring);
+    if (prod >= ARRAY_SIZE(ring->items)) {
+        qxl_set_guest_bug(qxl, "SPICE_RING_PROD_ITEM indices mismatch "
+                          "%u >= %zu", prod, ARRAY_SIZE(ring->items));
         return;
     }
-    if (*item == 0) {
+    if (ring->items[prod].el == 0) {
         /* stick head into the ring */
         id = ext.info->id;
         ext.info->next = 0;
         qxl_ram_set_dirty(qxl, &ext.info->next);
-        *item = id;
+        ring->items[prod].el = id;
         qxl_ring_set_dirty(qxl);
     } else {
         /* append item to the list */
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/5] linux-user: avoid string truncation warnings in uname field copying
  2019-04-12 12:16   ` Daniel P. Berrangé
  (?)
@ 2019-04-12 12:28   ` Laurent Vivier
  -1 siblings, 0 replies; 23+ messages in thread
From: Laurent Vivier @ 2019-04-12 12:28 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel; +Cc: Riku Voipio, Gerd Hoffmann

On 12/04/2019 14:16, Daniel P. Berrangé wrote:
> In file included from /usr/include/string.h:494,
>                  from include/qemu/osdep.h:101,
>                  from linux-user/uname.c:20:
> In function ‘strncpy’,
>     inlined from ‘sys_uname’ at linux-user/uname.c:94:3:
> /usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ output may be truncated copying 64 bytes from a string of length 64 [-Wstringop-truncation]
>   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> We don't care where the NUL terminator in the original uname
> field was. It suffices to copy the entire original field and
> simply force a NUL terminator at the end of the new field.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  linux-user/uname.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/uname.c b/linux-user/uname.c
> index 313b79dbad..3dff33effe 100644
> --- a/linux-user/uname.c
> +++ b/linux-user/uname.c
> @@ -73,7 +73,7 @@ const char *cpu_to_uname_machine(void *cpu_env)
>  #define COPY_UTSNAME_FIELD(dest, src) \
>    do { \
>        /* __NEW_UTS_LEN doesn't include terminating null */ \
> -      (void) strncpy((dest), (src), __NEW_UTS_LEN); \
> +      memcpy((dest), (src), MIN(sizeof(src), __NEW_UTS_LEN));   \

If we use sizeof(), I think we should use it for both:

  MIN(sizeof(dest), sizeof(src))

Thanks,
Laurent

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/5] linux-user: avoid string truncation warnings in elf field copying
  2019-04-12 12:16   ` Daniel P. Berrangé
  (?)
@ 2019-04-12 12:32   ` Laurent Vivier
  -1 siblings, 0 replies; 23+ messages in thread
From: Laurent Vivier @ 2019-04-12 12:32 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel; +Cc: Riku Voipio, Gerd Hoffmann

On 12/04/2019 14:16, Daniel P. Berrangé wrote:
> In file included from /usr/include/string.h:494,
>                  from include/qemu/osdep.h:101,
>                  from linux-user/elfload.c:2:
> In function ‘strncpy’,
>     inlined from ‘fill_psinfo’ at linux-user/elfload.c:3208:12,
>     inlined from ‘fill_note_info’ at linux-user/elfload.c:3390:5,
>     inlined from ‘elf_core_dump’ at linux-user/elfload.c:3539:9:
> /usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 16 equals destination size [-Wstringop-truncation]
>   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> We don't require the field to be NUL terminated, so can just
> copy the lower of the string length and the target field size
> using memcpy.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  linux-user/elfload.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index c1a26021f8..caa060f7b7 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -3180,6 +3180,7 @@ static int fill_psinfo(struct target_elf_prpsinfo *psinfo, const TaskState *ts)
>  {
>      char *base_filename;
>      unsigned int i, len;
> +    size_t pathlen;
>  
>      (void) memset(psinfo, 0, sizeof (*psinfo));
>  
> @@ -3201,12 +3202,9 @@ static int fill_psinfo(struct target_elf_prpsinfo *psinfo, const TaskState *ts)
>      psinfo->pr_gid = getgid();
>  
>      base_filename = g_path_get_basename(ts->bprm->filename);
> -    /*
> -     * Using strncpy here is fine: at max-length,
> -     * this field is not NUL-terminated.
> -     */

Keep and update the comment, it explains why we don't need to add the
NUL at the end when MIN() is sizeof(psinfo->pr_fname).

> -    (void) strncpy(psinfo->pr_fname, base_filename,
> -                   sizeof(psinfo->pr_fname));
> +    pathlen = strlen(base_filename) + 1;
> +    pathlen = MIN(pathlen, sizeof(psinfo->pr_fname));
> +    memcpy(psinfo->pr_fname, base_filename, pathlen);
>  
>      g_free(base_filename);
>      bswap_psinfo(psinfo);
> 

Thanks,
Laurent

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Qemu-devel] [PATCH v2 4/5] hw/usb: avoid format truncation warning when formatting port name
@ 2019-05-02  6:44     ` Gerd Hoffmann
  0 siblings, 0 replies; 23+ messages in thread
From: Gerd Hoffmann @ 2019-05-02  6:44 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: qemu-devel, Laurent Vivier, Riku Voipio

On Fri, Apr 12, 2019 at 01:16:25PM +0100, Daniel P. Berrangé wrote:
> hw/usb/hcd-xhci.c: In function ‘usb_xhci_realize’:
> hw/usb/hcd-xhci.c:3339:66: warning: ‘%d’ directive output may be truncated writing between 1 and 10 bytes into a region of size 5 [-Wformat-trunca\
> tion=]
>  3339 |             snprintf(port->name, sizeof(port->name), "usb2 port #%d", i+1);
>       |                                                                  ^~
> hw/usb/hcd-xhci.c:3339:54: note: directive argument in the range [1, 2147483647]
>  3339 |             snprintf(port->name, sizeof(port->name), "usb2 port #%d", i+1);
>       |                                                      ^~~~~~~~~~~~~~~
> 
> The xhci code formats the port name into a fixed length
> buffer which is only large enough to hold port numbers
> upto 5 digits in decimal representation. We're never
> going to have a port number that large, so aserting the
> port number is sensible is sufficient to tell GCC the
> formatted string won't be truncated.

Picked into the usb queue.

thanks,
  Gerd

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Qemu-devel] [PATCH v2 4/5] hw/usb: avoid format truncation warning when formatting port name
@ 2019-05-02  6:44     ` Gerd Hoffmann
  0 siblings, 0 replies; 23+ messages in thread
From: Gerd Hoffmann @ 2019-05-02  6:44 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: Riku Voipio, qemu-devel, Laurent Vivier

On Fri, Apr 12, 2019 at 01:16:25PM +0100, Daniel P. Berrangé wrote:
> hw/usb/hcd-xhci.c: In function ‘usb_xhci_realize’:
> hw/usb/hcd-xhci.c:3339:66: warning: ‘%d’ directive output may be truncated writing between 1 and 10 bytes into a region of size 5 [-Wformat-trunca\
> tion=]
>  3339 |             snprintf(port->name, sizeof(port->name), "usb2 port #%d", i+1);
>       |                                                                  ^~
> hw/usb/hcd-xhci.c:3339:54: note: directive argument in the range [1, 2147483647]
>  3339 |             snprintf(port->name, sizeof(port->name), "usb2 port #%d", i+1);
>       |                                                      ^~~~~~~~~~~~~~~
> 
> The xhci code formats the port name into a fixed length
> buffer which is only large enough to hold port numbers
> upto 5 digits in decimal representation. We're never
> going to have a port number that large, so aserting the
> port number is sensible is sufficient to tell GCC the
> formatted string won't be truncated.

Picked into the usb queue.

thanks,
  Gerd



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path
  2019-04-12 12:16   ` Daniel P. Berrangé
  (?)
@ 2019-05-02 15:45   ` Laurent Vivier
  2019-05-02 15:48       ` Daniel P. Berrangé
  -1 siblings, 1 reply; 23+ messages in thread
From: Laurent Vivier @ 2019-05-02 15:45 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel; +Cc: Riku Voipio, Gerd Hoffmann

Dan,

do you want I take this through the trivial branch queue or do you add
it into the Sockets branch queue?

Thanks,
Laurent

On 12/04/2019 14:16, Daniel P. Berrangé wrote:
> In file included from /usr/include/string.h:494,
>                  from include/qemu/osdep.h:101,
>                  from util/qemu-sockets.c:18:
> In function ‘strncpy’,
>     inlined from ‘unix_connect_saddr.isra.0’ at util/qemu-sockets.c:925:5:
> /usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 108 equals destination size [-Wstringop-truncation]
>   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In function ‘strncpy’,
>     inlined from ‘unix_listen_saddr.isra.0’ at util/qemu-sockets.c:880:5:
> /usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 108 equals destination size [-Wstringop-truncation]
>   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> We are already validating the UNIX socket path length earlier in
> the functions. If we save this string length when we first check
> it, then we can simply use memcpy instead of strcpy later, avoiding
> the gcc truncation warnings.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  util/qemu-sockets.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 9705051690..ba6335e71a 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -830,6 +830,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>      int sock, fd;
>      char *pathbuf = NULL;
>      const char *path;
> +    size_t pathlen;
>  
>      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>      if (sock < 0) {
> @@ -845,7 +846,8 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>          path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX", tmpdir);
>      }
>  
> -    if (strlen(path) > sizeof(un.sun_path)) {
> +    pathlen = strlen(path);
> +    if (pathlen > sizeof(un.sun_path)) {
>          error_setg(errp, "UNIX socket path '%s' is too long", path);
>          error_append_hint(errp, "Path must be less than %zu bytes\n",
>                            sizeof(un.sun_path));
> @@ -877,7 +879,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>  
>      memset(&un, 0, sizeof(un));
>      un.sun_family = AF_UNIX;
> -    strncpy(un.sun_path, path, sizeof(un.sun_path));
> +    memcpy(un.sun_path, path, pathlen);
>  
>      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
>          error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
> @@ -901,6 +903,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
>  {
>      struct sockaddr_un un;
>      int sock, rc;
> +    size_t pathlen;
>  
>      if (saddr->path == NULL) {
>          error_setg(errp, "unix connect: no path specified");
> @@ -913,7 +916,8 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
>          return -1;
>      }
>  
> -    if (strlen(saddr->path) > sizeof(un.sun_path)) {
> +    pathlen = strlen(saddr->path);
> +    if (pathlen > sizeof(un.sun_path)) {
>          error_setg(errp, "UNIX socket path '%s' is too long", saddr->path);
>          error_append_hint(errp, "Path must be less than %zu bytes\n",
>                            sizeof(un.sun_path));
> @@ -922,7 +926,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
>  
>      memset(&un, 0, sizeof(un));
>      un.sun_family = AF_UNIX;
> -    strncpy(un.sun_path, saddr->path, sizeof(un.sun_path));
> +    memcpy(un.sun_path, saddr->path, pathlen);
>  
>      /* connect to peer */
>      do {
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path
@ 2019-05-02 15:48       ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-05-02 15:48 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: qemu-devel, Riku Voipio, Gerd Hoffmann

On Thu, May 02, 2019 at 05:45:30PM +0200, Laurent Vivier wrote:
> Dan,
> 
> do you want I take this through the trivial branch queue or do you add
> it into the Sockets branch queue?

I'm fine with you sending it via trivial queue since there's nothing
else pending for the sockets code.


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 :|

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path
@ 2019-05-02 15:48       ` Daniel P. Berrangé
  0 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrangé @ 2019-05-02 15:48 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: Riku Voipio, qemu-devel, Gerd Hoffmann

On Thu, May 02, 2019 at 05:45:30PM +0200, Laurent Vivier wrote:
> Dan,
> 
> do you want I take this through the trivial branch queue or do you add
> it into the Sockets branch queue?

I'm fine with you sending it via trivial queue since there's nothing
else pending for the sockets code.


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 :|


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path
  2019-04-12 12:16   ` Daniel P. Berrangé
  (?)
  (?)
@ 2019-05-02 16:18   ` Laurent Vivier
  -1 siblings, 0 replies; 23+ messages in thread
From: Laurent Vivier @ 2019-05-02 16:18 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel; +Cc: Riku Voipio, Gerd Hoffmann

On 12/04/2019 14:16, Daniel P. Berrangé wrote:
> In file included from /usr/include/string.h:494,
>                  from include/qemu/osdep.h:101,
>                  from util/qemu-sockets.c:18:
> In function ‘strncpy’,
>     inlined from ‘unix_connect_saddr.isra.0’ at util/qemu-sockets.c:925:5:
> /usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 108 equals destination size [-Wstringop-truncation]
>   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In function ‘strncpy’,
>     inlined from ‘unix_listen_saddr.isra.0’ at util/qemu-sockets.c:880:5:
> /usr/include/bits/string_fortified.h:106:10: warning: ‘__builtin_strncpy’ specified bound 108 equals destination size [-Wstringop-truncation]
>   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> We are already validating the UNIX socket path length earlier in
> the functions. If we save this string length when we first check
> it, then we can simply use memcpy instead of strcpy later, avoiding
> the gcc truncation warnings.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  util/qemu-sockets.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 9705051690..ba6335e71a 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -830,6 +830,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>      int sock, fd;
>      char *pathbuf = NULL;
>      const char *path;
> +    size_t pathlen;
>  
>      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>      if (sock < 0) {
> @@ -845,7 +846,8 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>          path = pathbuf = g_strdup_printf("%s/qemu-socket-XXXXXX", tmpdir);
>      }
>  
> -    if (strlen(path) > sizeof(un.sun_path)) {
> +    pathlen = strlen(path);
> +    if (pathlen > sizeof(un.sun_path)) {
>          error_setg(errp, "UNIX socket path '%s' is too long", path);
>          error_append_hint(errp, "Path must be less than %zu bytes\n",
>                            sizeof(un.sun_path));
> @@ -877,7 +879,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>  
>      memset(&un, 0, sizeof(un));
>      un.sun_family = AF_UNIX;
> -    strncpy(un.sun_path, path, sizeof(un.sun_path));
> +    memcpy(un.sun_path, path, pathlen);
>  
>      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {
>          error_setg_errno(errp, errno, "Failed to bind socket to %s", path);
> @@ -901,6 +903,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
>  {
>      struct sockaddr_un un;
>      int sock, rc;
> +    size_t pathlen;
>  
>      if (saddr->path == NULL) {
>          error_setg(errp, "unix connect: no path specified");
> @@ -913,7 +916,8 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
>          return -1;
>      }
>  
> -    if (strlen(saddr->path) > sizeof(un.sun_path)) {
> +    pathlen = strlen(saddr->path);
> +    if (pathlen > sizeof(un.sun_path)) {
>          error_setg(errp, "UNIX socket path '%s' is too long", saddr->path);
>          error_append_hint(errp, "Path must be less than %zu bytes\n",
>                            sizeof(un.sun_path));
> @@ -922,7 +926,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
>  
>      memset(&un, 0, sizeof(un));
>      un.sun_family = AF_UNIX;
> -    strncpy(un.sun_path, saddr->path, sizeof(un.sun_path));
> +    memcpy(un.sun_path, saddr->path, pathlen);
>  
>      /* connect to peer */
>      do {
> 


Applied to my trivial-patches branch.

Thanks,
Laurent

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes
  2019-04-12 12:16   ` Daniel P. Berrangé
  (?)
@ 2019-05-07  7:54   ` Gerd Hoffmann
  2019-05-07  8:11     ` Philippe Mathieu-Daudé
  -1 siblings, 1 reply; 23+ messages in thread
From: Gerd Hoffmann @ 2019-05-07  7:54 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: Riku Voipio, qemu-devel, Laurent Vivier

On Fri, Apr 12, 2019 at 01:16:26PM +0100, Daniel P. Berrangé wrote:
> The SPICE_RING_PROD_ITEM() macro is initializing a local
> 'uint64_t *' variable to point to the 'el' field inside
> the QXLReleaseRing struct. This uint64_t field is not
> guaranteed aligned as the struct is packed.
> 
> Code should not take the address of fields within a
> packed struct. Changing the SPICE_RING_PROD_ITEM()
> macro to avoid taking the address of the field is
> impractical. It is clearer to just remove the macro
> and inline its functionality in the three call sites
> that need it.

Added patch to vga queue.

thanks,
  Gerd



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes
  2019-05-07  7:54   ` Gerd Hoffmann
@ 2019-05-07  8:11     ` Philippe Mathieu-Daudé
  2019-05-07  8:53       ` Gerd Hoffmann
  0 siblings, 1 reply; 23+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-05-07  8:11 UTC (permalink / raw)
  To: Gerd Hoffmann, Daniel P. Berrangé
  Cc: Marc-André Lureau, Riku Voipio, qemu-devel, Laurent Vivier

Hi Gerd,

On 5/7/19 9:54 AM, Gerd Hoffmann wrote:
> On Fri, Apr 12, 2019 at 01:16:26PM +0100, Daniel P. Berrangé wrote:
>> The SPICE_RING_PROD_ITEM() macro is initializing a local
>> 'uint64_t *' variable to point to the 'el' field inside
>> the QXLReleaseRing struct. This uint64_t field is not
>> guaranteed aligned as the struct is packed.
>>
>> Code should not take the address of fields within a
>> packed struct. Changing the SPICE_RING_PROD_ITEM()
>> macro to avoid taking the address of the field is
>> impractical. It is clearer to just remove the macro
>> and inline its functionality in the three call sites
>> that need it.
> 
> Added patch to vga queue.

What about the other patch Marc-André sent?
https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg01318.html


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes
  2019-05-07  8:11     ` Philippe Mathieu-Daudé
@ 2019-05-07  8:53       ` Gerd Hoffmann
  0 siblings, 0 replies; 23+ messages in thread
From: Gerd Hoffmann @ 2019-05-07  8:53 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Marc-André Lureau, Riku Voipio, qemu-devel, Laurent Vivier

On Tue, May 07, 2019 at 10:11:02AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Gerd,
> 
> On 5/7/19 9:54 AM, Gerd Hoffmann wrote:
> > On Fri, Apr 12, 2019 at 01:16:26PM +0100, Daniel P. Berrangé wrote:
> >> The SPICE_RING_PROD_ITEM() macro is initializing a local
> >> 'uint64_t *' variable to point to the 'el' field inside
> >> the QXLReleaseRing struct. This uint64_t field is not
> >> guaranteed aligned as the struct is packed.
> >>
> >> Code should not take the address of fields within a
> >> packed struct. Changing the SPICE_RING_PROD_ITEM()
> >> macro to avoid taking the address of the field is
> >> impractical. It is clearer to just remove the macro
> >> and inline its functionality in the three call sites
> >> that need it.
> > 
> > Added patch to vga queue.
> 
> What about the other patch Marc-André sent?
> https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg01318.html

Other patch version for the same issue, we need only one of the two.

cheers,
  Gerd


^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2019-05-07  8:55 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-12 12:16 [Qemu-devel] [PATCH v2 0/5] misc set of fixes for warnings under GCC 9 Daniel P. Berrangé
2019-04-12 12:16 ` Daniel P. Berrangé
2019-04-12 12:16 ` [Qemu-devel] [PATCH v2 1/5] linux-user: avoid string truncation warnings in uname field copying Daniel P. Berrangé
2019-04-12 12:16   ` Daniel P. Berrangé
2019-04-12 12:28   ` Laurent Vivier
2019-04-12 12:16 ` [Qemu-devel] [PATCH v2 2/5] linux-user: avoid string truncation warnings in elf " Daniel P. Berrangé
2019-04-12 12:16   ` Daniel P. Berrangé
2019-04-12 12:32   ` Laurent Vivier
2019-04-12 12:16 ` [Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path Daniel P. Berrangé
2019-04-12 12:16   ` Daniel P. Berrangé
2019-05-02 15:45   ` Laurent Vivier
2019-05-02 15:48     ` Daniel P. Berrangé
2019-05-02 15:48       ` Daniel P. Berrangé
2019-05-02 16:18   ` Laurent Vivier
2019-04-12 12:16 ` [Qemu-devel] [PATCH v2 4/5] hw/usb: avoid format truncation warning when formatting port name Daniel P. Berrangé
2019-04-12 12:16   ` Daniel P. Berrangé
2019-05-02  6:44   ` Gerd Hoffmann
2019-05-02  6:44     ` Gerd Hoffmann
2019-04-12 12:16 ` [Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes Daniel P. Berrangé
2019-04-12 12:16   ` Daniel P. Berrangé
2019-05-07  7:54   ` Gerd Hoffmann
2019-05-07  8:11     ` Philippe Mathieu-Daudé
2019-05-07  8:53       ` Gerd Hoffmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.