[Qemu-devel] [PATCH v2] vt82c686: fix coverity warning about out-of-bounds write

[Qemu-devel] [PATCH v2] vt82c686: fix coverity warning about out-of-bounds write

[zhanghailiang]

Refactor superio_ioport_writeb to fix the out of bounds write warning.

In addition, fix two typos: s/chage/change/

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 - using bool instead of int for can_write as suggested by Stefan Weil
 - fix two typos: s/chage/change
---
 hw/isa/vt82c686.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index e0c235c..6c00f22 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -50,13 +50,13 @@ typedef struct VT82C686BState {
 static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
                                   unsigned size)
 {
-    int can_write;
     SuperIOConfig *superio_conf = opaque;
 
     DPRINTF("superio_ioport_writeb  address 0x%x  val 0x%x\n", addr, data);
     if (addr == 0x3f0) {
         superio_conf->index = data & 0xff;
     } else {
+        bool can_write = true;
         /* 0x3f1 */
         switch (superio_conf->index) {
         case 0x00 ... 0xdf:
@@ -68,30 +68,27 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
         case 0xf7:
         case 0xf9 ... 0xfb:
         case 0xfd ... 0xff:
-            can_write = 0;
+            can_write = false;
             break;
-        default:
-            can_write = 1;
-
-            if (can_write) {
-                switch (superio_conf->index) {
-                case 0xe7:
-                    if ((data & 0xff) != 0xfe) {
-                        DPRINTF("chage uart 1 base. unsupported yet\n");
-                    }
-                    break;
-                case 0xe8:
-                    if ((data & 0xff) != 0xbe) {
-                        DPRINTF("chage uart 2 base. unsupported yet\n");
-                    }
-                    break;
-
-                default:
-                    superio_conf->config[superio_conf->index] = data & 0xff;
-                }
+        case 0xe7:
+            if ((data & 0xff) != 0xfe) {
+                DPRINTF("change uart 1 base. unsupported yet\n");
+                can_write = false;
+            }
+            break;
+        case 0xe8:
+            if ((data & 0xff) != 0xbe) {
+                DPRINTF("change uart 2 base. unsupported yet\n");
+                can_write = false;
             }
+            break;
+        default:
+            break;
+
+        }
+        if (can_write == true) {
+            superio_conf->config[superio_conf->index] = data & 0xff;
         }
-        superio_conf->config[superio_conf->index] = data & 0xff;
     }
 }
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2] vt82c686: fix coverity warning about out-of-bounds write

[Paolo Bonzini]

On 09/12/2014 08:15, zhanghailiang wrote:
> Refactor superio_ioport_writeb to fix the out of bounds write warning.
> 
> In addition, fix two typos: s/chage/change/
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  - using bool instead of int for can_write as suggested by Stefan Weil
>  - fix two typos: s/chage/change
> ---
>  hw/isa/vt82c686.c | 41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index e0c235c..6c00f22 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -50,13 +50,13 @@ typedef struct VT82C686BState {
>  static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
>                                    unsigned size)
>  {
> -    int can_write;
>      SuperIOConfig *superio_conf = opaque;
>  
>      DPRINTF("superio_ioport_writeb  address 0x%x  val 0x%x\n", addr, data);
>      if (addr == 0x3f0) {
>          superio_conf->index = data & 0xff;
>      } else {
> +        bool can_write = true;
>          /* 0x3f1 */
>          switch (superio_conf->index) {
>          case 0x00 ... 0xdf:
> @@ -68,30 +68,27 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
>          case 0xf7:
>          case 0xf9 ... 0xfb:
>          case 0xfd ... 0xff:
> -            can_write = 0;
> +            can_write = false;
>              break;
> -        default:
> -            can_write = 1;
> -
> -            if (can_write) {
> -                switch (superio_conf->index) {
> -                case 0xe7:
> -                    if ((data & 0xff) != 0xfe) {
> -                        DPRINTF("chage uart 1 base. unsupported yet\n");
> -                    }
> -                    break;
> -                case 0xe8:
> -                    if ((data & 0xff) != 0xbe) {
> -                        DPRINTF("chage uart 2 base. unsupported yet\n");
> -                    }
> -                    break;
> -
> -                default:
> -                    superio_conf->config[superio_conf->index] = data & 0xff;
> -                }
> +        case 0xe7:
> +            if ((data & 0xff) != 0xfe) {
> +                DPRINTF("change uart 1 base. unsupported yet\n");
> +                can_write = false;
> +            }
> +            break;
> +        case 0xe8:
> +            if ((data & 0xff) != 0xbe) {
> +                DPRINTF("change uart 2 base. unsupported yet\n");
> +                can_write = false;
>              }
> +            break;
> +        default:
> +            break;
> +
> +        }
> +        if (can_write == true) {

Michael, can you remove "== true" when applying this patch?

Thanks,

Paolo

> +            superio_conf->config[superio_conf->index] = data & 0xff;
>          }
> -        superio_conf->config[superio_conf->index] = data & 0xff;
>      }
>  }
>  
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2] vt82c686: fix coverity warning about out-of-bounds write

[Michael Tokarev]

09.12.2014 10:15, zhanghailiang wrote:
> Refactor superio_ioport_writeb to fix the out of bounds write warning.

Is it just a warning, or real oob write?
>From the code it looks like it's just a warning...

[]
> +
> +        }
> +        if (can_write == true) {

09.12.2014 17:08, Paolo Bonzini wrote:
> Michael, can you remove "== true" when applying this patch?

Sure, just did.  Does it mean I can add your R-b too? ;)

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2] vt82c686: fix coverity warning about out-of-bounds write

[Paolo Bonzini]

On 10/12/2014 09:57, Michael Tokarev wrote:
> 09.12.2014 10:15, zhanghailiang wrote:
>> Refactor superio_ioport_writeb to fix the out of bounds write warning.
> 
> Is it just a warning, or real oob write?
> From the code it looks like it's just a warning...

It's a bug.

The simpler patch would have been just to remove the useless assignment

        superio_conf->config[superio_conf->index] = data & 0xff;

that is *outside* the switch and not protected by if (can_write).  Apart
from this, there is an off-by-one that I'll send a patch for right away.

Paolo

> 
> []
>> +
>> +        }
>> +        if (can_write == true) {
> 
> 09.12.2014 17:08, Paolo Bonzini wrote:
>> Michael, can you remove "== true" when applying this patch?
> 
> Sure, just did.  Does it mean I can add your R-b too? ;)

Sure.

Paolo

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2] vt82c686: fix coverity warning about out-of-bounds write

[zhanghailiang]

On 2014/12/10 16:57, Michael Tokarev wrote:
> 09.12.2014 10:15, zhanghailiang wrote:
>> Refactor superio_ioport_writeb to fix the out of bounds write warning.
>
> Is it just a warning, or real oob write?
>>From the code it looks like it's just a warning...
>

Er, i don't know when input 'data' will be oxff,
if this happens, it will access superio_conf->config[0xff] which is out of bounds
for superio_conf->config.

Actually, we should check the 'can_write' when access superio_conf->config[] in the follow codes,
but these codes seem a little odd, so refactor it is better choice. ;)


> []
>> +
>> +        }
>> +        if (can_write == true) {
>
> 09.12.2014 17:08, Paolo Bonzini wrote:
>> Michael, can you remove "== true" when applying this patch?
>
> Sure, just did.  Does it mean I can add your R-b too? ;)
>

cc: Paolo

Thanks.
zhanghailiang

Re: [Qemu-devel] [Qemu-trivial] [PATCH v2] vt82c686: fix coverity warning about out-of-bounds write

[Paolo Bonzini]

On 10/12/2014 09:57, Michael Tokarev wrote:
> 09.12.2014 10:15, zhanghailiang wrote:
>> Refactor superio_ioport_writeb to fix the out of bounds write warning.
> 
> Is it just a warning, or real oob write?
> From the code it looks like it's just a warning...
> 
> []
>> +
>> +        }
>> +        if (can_write == true) {
> 
> 09.12.2014 17:08, Paolo Bonzini wrote:
>> Michael, can you remove "== true" when applying this patch?
> 
> Sure, just did.  Does it mean I can add your R-b too? ;)

Yes, please do.

Paolo