diff mbox series

hw/s390x/virtio-ccw.c: Don't take address of fields in packed structs

Message ID 20181210120436.30522-1-peter.maydell@linaro.org
State Superseded
Headers show
Series hw/s390x/virtio-ccw.c: Don't take address of fields in packed structs | expand

Commit Message

Peter Maydell Dec. 10, 2018, 12:04 p.m. UTC 
Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this. Avoid the bug by not using the
"modify in place" byte swapping functions.

Patch produced with scripts/coccinelle/inplace-byteswaps.cocci
(with a couple of long lines manually wrapped).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
s390 also has some warnings in hw/s390x/css.c which are going to
be harder to fix, relating to taking the address of the 'pmcw' and
'scsw' fields in the SCHUB struct...

 hw/s390x/virtio-ccw.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

-- 
2.19.2

Comments

Philippe Mathieu-Daudé Dec. 10, 2018, 12:35 p.m. UTC | #1 
On 12/10/18 1:04 PM, Peter Maydell wrote:
> Taking the address of a field in a packed struct is a bad idea, because

> it might not be actually aligned enough for that pointer type (and

> thus cause a crash on dereference on some host architectures). Newer

> versions of clang warn about this. Avoid the bug by not using the

> "modify in place" byte swapping functions.

> 

> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci

> (with a couple of long lines manually wrapped).

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


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


> ---

> s390 also has some warnings in hw/s390x/css.c which are going to

> be harder to fix, relating to taking the address of the 'pmcw' and

> 'scsw' fields in the SCHUB struct...

> 

>  hw/s390x/virtio-ccw.c | 42 ++++++++++++++++++++++--------------------

>  1 file changed, 22 insertions(+), 20 deletions(-)

> 

> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c

> index 212b3d3dead..c2b78c8e9b1 100644

> --- a/hw/s390x/virtio-ccw.c

> +++ b/hw/s390x/virtio-ccw.c

> @@ -287,18 +287,18 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,

>      }

>      if (is_legacy) {

>          ccw_dstream_read(&sch->cds, linfo);

> -        be64_to_cpus(&linfo.queue);

> -        be32_to_cpus(&linfo.align);

> -        be16_to_cpus(&linfo.index);

> -        be16_to_cpus(&linfo.num);

> +        linfo.queue = be64_to_cpu(linfo.queue);

> +        linfo.align = be32_to_cpu(linfo.align);

> +        linfo.index = be16_to_cpu(linfo.index);

> +        linfo.num = be16_to_cpu(linfo.num);

>          ret = virtio_ccw_set_vqs(sch, NULL, &linfo);

>      } else {

>          ccw_dstream_read(&sch->cds, info);

> -        be64_to_cpus(&info.desc);

> -        be16_to_cpus(&info.index);

> -        be16_to_cpus(&info.num);

> -        be64_to_cpus(&info.avail);

> -        be64_to_cpus(&info.used);

> +        info.desc = be64_to_cpu(info.desc);

> +        info.index = be16_to_cpu(info.index);

> +        info.num = be16_to_cpu(info.num);

> +        info.avail = be64_to_cpu(info.avail);

> +        info.used = be64_to_cpu(info.used);

>          ret = virtio_ccw_set_vqs(sch, &info, NULL);

>      }

>      sch->curr_status.scsw.count = 0;

> @@ -382,7 +382,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>                  features.features = 0;

>              }

>              ccw_dstream_rewind(&sch->cds);

> -            cpu_to_le32s(&features.features);

> +            features.features = cpu_to_le32(features.features);

>              ccw_dstream_write(&sch->cds, features.features);

>              sch->curr_status.scsw.count = ccw.count - sizeof(features);

>              ret = 0;

> @@ -403,7 +403,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>              ret = -EFAULT;

>          } else {

>              ccw_dstream_read(&sch->cds, features);

> -            le32_to_cpus(&features.features);

> +            features.features = le32_to_cpu(features.features);

>              if (features.index == 0) {

>                  virtio_set_features(vdev,

>                                      (vdev->guest_features & 0xffffffff00000000ULL) |

> @@ -546,7 +546,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>              ret = -EFAULT;

>          } else {

>              ccw_dstream_read(&sch->cds, indicators);

> -            be64_to_cpus(&indicators);

> +            indicators = be64_to_cpu(indicators);

>              dev->indicators = get_indicator(indicators, sizeof(uint64_t));

>              sch->curr_status.scsw.count = ccw.count - sizeof(indicators);

>              ret = 0;

> @@ -567,7 +567,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>              ret = -EFAULT;

>          } else {

>              ccw_dstream_read(&sch->cds, indicators);

> -            be64_to_cpus(&indicators);

> +            indicators = be64_to_cpu(indicators);

>              dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));

>              sch->curr_status.scsw.count = ccw.count - sizeof(indicators);

>              ret = 0;

> @@ -588,14 +588,14 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>              ret = -EFAULT;

>          } else {

>              ccw_dstream_read(&sch->cds, vq_config.index);

> -            be16_to_cpus(&vq_config.index);

> +            vq_config.index = be16_to_cpu(vq_config.index);

>              if (vq_config.index >= VIRTIO_QUEUE_MAX) {

>                  ret = -EINVAL;

>                  break;

>              }

>              vq_config.num_max = virtio_queue_get_num(vdev,

>                                                       vq_config.index);

> -            cpu_to_be16s(&vq_config.num_max);

> +            vq_config.num_max = cpu_to_be16(vq_config.num_max);

>              ccw_dstream_write(&sch->cds, vq_config.num_max);

>              sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);

>              ret = 0;

> @@ -621,9 +621,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>              if (ccw_dstream_read(&sch->cds, thinint)) {

>                  ret = -EFAULT;

>              } else {

> -                be64_to_cpus(&thinint.ind_bit);

> -                be64_to_cpus(&thinint.summary_indicator);

> -                be64_to_cpus(&thinint.device_indicator);

> +                thinint.ind_bit = be64_to_cpu(thinint.ind_bit);

> +                thinint.summary_indicator =

> +                    be64_to_cpu(thinint.summary_indicator);

> +                thinint.device_indicator =

> +                    be64_to_cpu(thinint.device_indicator);

>  

>                  dev->summary_indicator =

>                      get_indicator(thinint.summary_indicator, sizeof(uint8_t));

> @@ -654,8 +656,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>              break;

>          }

>          ccw_dstream_read_buf(&sch->cds, &revinfo, 4);

> -        be16_to_cpus(&revinfo.revision);

> -        be16_to_cpus(&revinfo.length);

> +        revinfo.revision = be16_to_cpu(revinfo.revision);

> +        revinfo.length = be16_to_cpu(revinfo.length);

>          if (ccw.count < len + revinfo.length ||

>              (check_len && ccw.count > len + revinfo.length)) {

>              ret = -EINVAL;

>
Halil Pasic Dec. 10, 2018, 1:06 p.m. UTC | #2 
On Mon, 10 Dec 2018 12:04:36 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> Taking the address of a field in a packed struct is a bad idea, because

> it might not be actually aligned enough for that pointer type (and

> thus cause a crash on dereference on some host architectures). Newer

> versions of clang warn about this. Avoid the bug by not using the

> "modify in place" byte swapping functions.

> 

> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci

> (with a couple of long lines manually wrapped).

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Halil Pasic <pasic@linux.ibm.com>


I wonder if packed was a good idea in the first place. @Connie: Do you
have an opinion on this?

Regards,
Halil

> ---

> s390 also has some warnings in hw/s390x/css.c which are going to

> be harder to fix, relating to taking the address of the 'pmcw' and

> 'scsw' fields in the SCHUB struct...

> 

>  hw/s390x/virtio-ccw.c | 42 ++++++++++++++++++++++--------------------

>  1 file changed, 22 insertions(+), 20 deletions(-)

> 

> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c

> index 212b3d3dead..c2b78c8e9b1 100644

> --- a/hw/s390x/virtio-ccw.c

> +++ b/hw/s390x/virtio-ccw.c

> @@ -287,18 +287,18 @@ static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,

>      }

>      if (is_legacy) {

>          ccw_dstream_read(&sch->cds, linfo);

> -        be64_to_cpus(&linfo.queue);

> -        be32_to_cpus(&linfo.align);

> -        be16_to_cpus(&linfo.index);

> -        be16_to_cpus(&linfo.num);

> +        linfo.queue = be64_to_cpu(linfo.queue);

> +        linfo.align = be32_to_cpu(linfo.align);

> +        linfo.index = be16_to_cpu(linfo.index);

> +        linfo.num = be16_to_cpu(linfo.num);

>          ret = virtio_ccw_set_vqs(sch, NULL, &linfo);

>      } else {

>          ccw_dstream_read(&sch->cds, info);

> -        be64_to_cpus(&info.desc);

> -        be16_to_cpus(&info.index);

> -        be16_to_cpus(&info.num);

> -        be64_to_cpus(&info.avail);

> -        be64_to_cpus(&info.used);

> +        info.desc = be64_to_cpu(info.desc);

> +        info.index = be16_to_cpu(info.index);

> +        info.num = be16_to_cpu(info.num);

> +        info.avail = be64_to_cpu(info.avail);

> +        info.used = be64_to_cpu(info.used);

>          ret = virtio_ccw_set_vqs(sch, &info, NULL);

>      }

>      sch->curr_status.scsw.count = 0;

> @@ -382,7 +382,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>                  features.features = 0;

>              }

>              ccw_dstream_rewind(&sch->cds);

> -            cpu_to_le32s(&features.features);

> +            features.features = cpu_to_le32(features.features);

>              ccw_dstream_write(&sch->cds, features.features);

>              sch->curr_status.scsw.count = ccw.count - sizeof(features);

>              ret = 0;

> @@ -403,7 +403,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>              ret = -EFAULT;

>          } else {

>              ccw_dstream_read(&sch->cds, features);

> -            le32_to_cpus(&features.features);

> +            features.features = le32_to_cpu(features.features);

>              if (features.index == 0) {

>                  virtio_set_features(vdev,

>                                      (vdev->guest_features & 0xffffffff00000000ULL) |

> @@ -546,7 +546,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>              ret = -EFAULT;

>          } else {

>              ccw_dstream_read(&sch->cds, indicators);

> -            be64_to_cpus(&indicators);

> +            indicators = be64_to_cpu(indicators);

>              dev->indicators = get_indicator(indicators, sizeof(uint64_t));

>              sch->curr_status.scsw.count = ccw.count - sizeof(indicators);

>              ret = 0;

> @@ -567,7 +567,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>              ret = -EFAULT;

>          } else {

>              ccw_dstream_read(&sch->cds, indicators);

> -            be64_to_cpus(&indicators);

> +            indicators = be64_to_cpu(indicators);

>              dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));

>              sch->curr_status.scsw.count = ccw.count - sizeof(indicators);

>              ret = 0;

> @@ -588,14 +588,14 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>              ret = -EFAULT;

>          } else {

>              ccw_dstream_read(&sch->cds, vq_config.index);

> -            be16_to_cpus(&vq_config.index);

> +            vq_config.index = be16_to_cpu(vq_config.index);

>              if (vq_config.index >= VIRTIO_QUEUE_MAX) {

>                  ret = -EINVAL;

>                  break;

>              }

>              vq_config.num_max = virtio_queue_get_num(vdev,

>                                                       vq_config.index);

> -            cpu_to_be16s(&vq_config.num_max);

> +            vq_config.num_max = cpu_to_be16(vq_config.num_max);

>              ccw_dstream_write(&sch->cds, vq_config.num_max);

>              sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);

>              ret = 0;

> @@ -621,9 +621,11 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>              if (ccw_dstream_read(&sch->cds, thinint)) {

>                  ret = -EFAULT;

>              } else {

> -                be64_to_cpus(&thinint.ind_bit);

> -                be64_to_cpus(&thinint.summary_indicator);

> -                be64_to_cpus(&thinint.device_indicator);

> +                thinint.ind_bit = be64_to_cpu(thinint.ind_bit);

> +                thinint.summary_indicator =

> +                    be64_to_cpu(thinint.summary_indicator);

> +                thinint.device_indicator =

> +                    be64_to_cpu(thinint.device_indicator);

>  

>                  dev->summary_indicator =

>                      get_indicator(thinint.summary_indicator, sizeof(uint8_t));

> @@ -654,8 +656,8 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)

>              break;

>          }

>          ccw_dstream_read_buf(&sch->cds, &revinfo, 4);

> -        be16_to_cpus(&revinfo.revision);

> -        be16_to_cpus(&revinfo.length);

> +        revinfo.revision = be16_to_cpu(revinfo.revision);

> +        revinfo.length = be16_to_cpu(revinfo.length);

>          if (ccw.count < len + revinfo.length ||

>              (check_len && ccw.count > len + revinfo.length)) {

>              ret = -EINVAL;
Cornelia Huck Dec. 10, 2018, 1:39 p.m. UTC | #3 
On Mon, 10 Dec 2018 14:06:42 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Mon, 10 Dec 2018 12:04:36 +0000

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

> 

> > Taking the address of a field in a packed struct is a bad idea, because

> > it might not be actually aligned enough for that pointer type (and

> > thus cause a crash on dereference on some host architectures). Newer

> > versions of clang warn about this. Avoid the bug by not using the

> > "modify in place" byte swapping functions.

> > 

> > Patch produced with scripts/coccinelle/inplace-byteswaps.cocci

> > (with a couple of long lines manually wrapped).

> > 

> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>  

> 

> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

> 

> I wonder if packed was a good idea in the first place. @Connie: Do you

> have an opinion on this?


Some of the structures probably don't need the packed attribute; but
there are two structures that have a trailing 8 bit value. The problem
is that the structures are a a guest<->host interface as defined in the
virtio standard, so we can't e.g. add padding to the structures with
the isc at the end.

If I had to re-define the interface, I'd probably add tail padding; but
I was not aware of the problems the packing might cause when I defined
them, and we can't get rid of them without also getting rid of support
for older virtio versions.

Simply moving away from the modify in place byteswaps is the easiest
solution.
Cornelia Huck Dec. 10, 2018, 3:22 p.m. UTC | #4 
On Mon, 10 Dec 2018 12:04:36 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> Taking the address of a field in a packed struct is a bad idea, because

> it might not be actually aligned enough for that pointer type (and

> thus cause a crash on dereference on some host architectures). Newer

> versions of clang warn about this. Avoid the bug by not using the

> "modify in place" byte swapping functions.

> 

> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci

> (with a couple of long lines manually wrapped).

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Thanks, applied.
diff mbox series

Patch

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 212b3d3dead..c2b78c8e9b1 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -287,18 +287,18 @@  static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
     }
     if (is_legacy) {
         ccw_dstream_read(&sch->cds, linfo);
-        be64_to_cpus(&linfo.queue);
-        be32_to_cpus(&linfo.align);
-        be16_to_cpus(&linfo.index);
-        be16_to_cpus(&linfo.num);
+        linfo.queue = be64_to_cpu(linfo.queue);
+        linfo.align = be32_to_cpu(linfo.align);
+        linfo.index = be16_to_cpu(linfo.index);
+        linfo.num = be16_to_cpu(linfo.num);
         ret = virtio_ccw_set_vqs(sch, NULL, &linfo);
     } else {
         ccw_dstream_read(&sch->cds, info);
-        be64_to_cpus(&info.desc);
-        be16_to_cpus(&info.index);
-        be16_to_cpus(&info.num);
-        be64_to_cpus(&info.avail);
-        be64_to_cpus(&info.used);
+        info.desc = be64_to_cpu(info.desc);
+        info.index = be16_to_cpu(info.index);
+        info.num = be16_to_cpu(info.num);
+        info.avail = be64_to_cpu(info.avail);
+        info.used = be64_to_cpu(info.used);
         ret = virtio_ccw_set_vqs(sch, &info, NULL);
     }
     sch->curr_status.scsw.count = 0;
@@ -382,7 +382,7 @@  static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                 features.features = 0;
             }
             ccw_dstream_rewind(&sch->cds);
-            cpu_to_le32s(&features.features);
+            features.features = cpu_to_le32(features.features);
             ccw_dstream_write(&sch->cds, features.features);
             sch->curr_status.scsw.count = ccw.count - sizeof(features);
             ret = 0;
@@ -403,7 +403,7 @@  static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EFAULT;
         } else {
             ccw_dstream_read(&sch->cds, features);
-            le32_to_cpus(&features.features);
+            features.features = le32_to_cpu(features.features);
             if (features.index == 0) {
                 virtio_set_features(vdev,
                                     (vdev->guest_features & 0xffffffff00000000ULL) |
@@ -546,7 +546,7 @@  static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EFAULT;
         } else {
             ccw_dstream_read(&sch->cds, indicators);
-            be64_to_cpus(&indicators);
+            indicators = be64_to_cpu(indicators);
             dev->indicators = get_indicator(indicators, sizeof(uint64_t));
             sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
             ret = 0;
@@ -567,7 +567,7 @@  static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EFAULT;
         } else {
             ccw_dstream_read(&sch->cds, indicators);
-            be64_to_cpus(&indicators);
+            indicators = be64_to_cpu(indicators);
             dev->indicators2 = get_indicator(indicators, sizeof(uint64_t));
             sch->curr_status.scsw.count = ccw.count - sizeof(indicators);
             ret = 0;
@@ -588,14 +588,14 @@  static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             ret = -EFAULT;
         } else {
             ccw_dstream_read(&sch->cds, vq_config.index);
-            be16_to_cpus(&vq_config.index);
+            vq_config.index = be16_to_cpu(vq_config.index);
             if (vq_config.index >= VIRTIO_QUEUE_MAX) {
                 ret = -EINVAL;
                 break;
             }
             vq_config.num_max = virtio_queue_get_num(vdev,
                                                      vq_config.index);
-            cpu_to_be16s(&vq_config.num_max);
+            vq_config.num_max = cpu_to_be16(vq_config.num_max);
             ccw_dstream_write(&sch->cds, vq_config.num_max);
             sch->curr_status.scsw.count = ccw.count - sizeof(vq_config);
             ret = 0;
@@ -621,9 +621,11 @@  static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             if (ccw_dstream_read(&sch->cds, thinint)) {
                 ret = -EFAULT;
             } else {
-                be64_to_cpus(&thinint.ind_bit);
-                be64_to_cpus(&thinint.summary_indicator);
-                be64_to_cpus(&thinint.device_indicator);
+                thinint.ind_bit = be64_to_cpu(thinint.ind_bit);
+                thinint.summary_indicator =
+                    be64_to_cpu(thinint.summary_indicator);
+                thinint.device_indicator =
+                    be64_to_cpu(thinint.device_indicator);
 
                 dev->summary_indicator =
                     get_indicator(thinint.summary_indicator, sizeof(uint8_t));
@@ -654,8 +656,8 @@  static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             break;
         }
         ccw_dstream_read_buf(&sch->cds, &revinfo, 4);
-        be16_to_cpus(&revinfo.revision);
-        be16_to_cpus(&revinfo.length);
+        revinfo.revision = be16_to_cpu(revinfo.revision);
+        revinfo.length = be16_to_cpu(revinfo.length);
         if (ccw.count < len + revinfo.length ||
             (check_len && ccw.count > len + revinfo.length)) {
             ret = -EINVAL;