[Qemu-devel] [PATCH 0/2] fix two overflow problems

[Qemu-devel] [PATCH 0/2] fix two overflow problems

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>


Gonglei (2):
  virito-pci: fix OVERRUN problem
  qdev: fix OVERFLOW_BEFORE_WIDEN

 hw/core/qdev-properties.c | 2 +-
 hw/virtio/virtio-pci.c    | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/2] virito-pci: fix OVERRUN problem

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Overrunning array "proxy->guest_features" of 2 4-byte
elements at element index 2 (byte offset 8) using index
"proxy->gfselect" (which evaluates to 2). Normally, the
Linux kernel driver just read/write '0' or '1' as the
"proxy->gfselect" values, so using '<' instead of '=<' to
make coverity happy and avoid potential harm.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/virtio/virtio-pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index d7cf34c..ce1c46e 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -977,7 +977,7 @@ static uint64_t virtio_pci_common_read(void *opaque, hwaddr addr,
         val = proxy->gfselect;
         break;
     case VIRTIO_PCI_COMMON_GF:
-        if (proxy->gfselect <= ARRAY_SIZE(proxy->guest_features)) {
+        if (proxy->gfselect < ARRAY_SIZE(proxy->guest_features)) {
             val = proxy->guest_features[proxy->gfselect];
         }
         break;
@@ -1052,7 +1052,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
         proxy->gfselect = val;
         break;
     case VIRTIO_PCI_COMMON_GF:
-        if (proxy->gfselect <= ARRAY_SIZE(proxy->guest_features)) {
+        if (proxy->gfselect < ARRAY_SIZE(proxy->guest_features)) {
             proxy->guest_features[proxy->gfselect] = val;
             virtio_set_features(vdev,
                                 (((uint64_t)proxy->guest_features[1]) << 32) |
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/2] qdev: fix OVERFLOW_BEFORE_WIDEN

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Potentially overflowing expression "1 << prop->bitnr" with
type "int" (32 bits, signed) is evaluated using 32-bit arithmetic,
and then used in a context that expects an expression of type
"uint64_t" (64 bits, unsigned).

Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/core/qdev-properties.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index a1606de..f78b335 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -130,7 +130,7 @@ PropertyInfo qdev_prop_bit = {
 static uint64_t qdev_get_prop_mask64(Property *prop)
 {
     assert(prop->info == &qdev_prop_bit);
-    return 0x1 << prop->bitnr;
+    return 0x1ull << prop->bitnr;
 }
 
 static void bit64_prop_set(DeviceState *dev, Property *props, bool val)
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 0/2] fix two overflow problems

[Michael S. Tsirkin]

On Tue, Jun 23, 2015 at 09:53:03AM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> 
> Gonglei (2):
>   virito-pci: fix OVERRUN problem
>   qdev: fix OVERFLOW_BEFORE_WIDEN


Applied, thanks a lot.

>  hw/core/qdev-properties.c | 2 +-
>  hw/virtio/virtio-pci.c    | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH 1/2] virito-pci: fix OVERRUN problem

[Michael S. Tsirkin]

Although the subject is virtio-pci. Fixed up.

Re: [Qemu-devel] [PATCH 1/2] virito-pci: fix OVERRUN problem

[Gonglei]

On 2015/6/23 14:32, Michael S. Tsirkin wrote:
> Although the subject is virtio-pci. Fixed up.
> 
Yes, thanks.

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH 2/2] qdev: fix OVERFLOW_BEFORE_WIDEN

[Markus Armbruster]

<arei.gonglei@huawei.com> writes:

> From: Gonglei <arei.gonglei@huawei.com>
>
> Potentially overflowing expression "1 << prop->bitnr" with
> type "int" (32 bits, signed) is evaluated using 32-bit arithmetic,
> and then used in a context that expects an expression of type
> "uint64_t" (64 bits, unsigned).
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/core/qdev-properties.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index a1606de..f78b335 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -130,7 +130,7 @@ PropertyInfo qdev_prop_bit = {
>  static uint64_t qdev_get_prop_mask64(Property *prop)
>  {
>      assert(prop->info == &qdev_prop_bit);
> -    return 0x1 << prop->bitnr;
> +    return 0x1ull << prop->bitnr;
>  }
>  
>  static void bit64_prop_set(DeviceState *dev, Property *props, bool val)

In my opionion, the 0x in 0x1ull is pure noise :)

Re: [Qemu-devel] [PATCH 0/2] fix two overflow problems

[Paolo Bonzini]

On 23/06/2015 03:53, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> 
> Gonglei (2):
>   virito-pci: fix OVERRUN problem
>   qdev: fix OVERFLOW_BEFORE_WIDEN
> 
>  hw/core/qdev-properties.c | 2 +-
>  hw/virtio/virtio-pci.c    | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 

I queued these two patches in case the maintainers do not pick them up
quickly.

Paolo

Re: [Qemu-devel] [PATCH 2/2] qdev: fix OVERFLOW_BEFORE_WIDEN

[Gerd Hoffmann]

On Di, 2015-06-23 at 09:53 +0800, arei.gonglei@huawei.com wrote:
>  static uint64_t qdev_get_prop_mask64(Property *prop)
>  {
>      assert(prop->info == &qdev_prop_bit);
> -    return 0x1 << prop->bitnr;
> +    return 0x1ull << prop->bitnr;
>  }

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>