[Qemu-devel] [PATCH v2 0/5] misc set of fixes for warnings under GCC 9

[Qemu-devel] [PATCH v2 0/5] misc set of fixes for warnings under GCC 9

[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

[Qemu-devel] [PATCH v2 0/5] misc set of fixes for warnings under GCC 9

[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

[Qemu-devel] [PATCH v2 1/5] linux-user: avoid string truncation warnings in uname field copying

[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

[Qemu-devel] [PATCH v2 1/5] linux-user: avoid string truncation warnings in uname field copying

[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

Re: [Qemu-devel] [PATCH v2 1/5] linux-user: avoid string truncation warnings in uname field copying

[Laurent Vivier]

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

[Qemu-devel] [PATCH v2 2/5] linux-user: avoid string truncation warnings in elf field copying

[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

[Qemu-devel] [PATCH v2 2/5] linux-user: avoid string truncation warnings in elf field copying

[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

Re: [Qemu-devel] [PATCH v2 2/5] linux-user: avoid string truncation warnings in elf field copying

[Laurent Vivier]

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

[Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path

[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

[Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path

[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

Re: [Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path

[Laurent Vivier]

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

Re: [Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path

[Daniel P. Berrangé]

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

Re: [Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path

[Daniel P. Berrangé]

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

Re: [Qemu-devel] [PATCH v2 3/5] sockets: avoid string truncation warnings when copying UNIX path

[Laurent Vivier]

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

[Qemu-devel] [PATCH v2 4/5] hw/usb: avoid format truncation warning when formatting port name

[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

[Qemu-devel] [PATCH v2 4/5] hw/usb: avoid format truncation warning when formatting port name

[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

Re: [Qemu-devel] [PATCH v2 4/5] hw/usb: avoid format truncation warning when formatting port name

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH v2 4/5] hw/usb: avoid format truncation warning when formatting port name

[Gerd Hoffmann]

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

[Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes

[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

[Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes

[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

Re: [Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes

[Philippe Mathieu-Daudé]

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

Re: [Qemu-devel] [PATCH v2 5/5] qxl: avoid unaligned pointer reads/writes

[Gerd Hoffmann]

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