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

[zhanghailiang]

On 2014/12/9 14:04, Stefan Weil wrote:

Am 09.12.2014 um 06:44 schrieb zhanghailiang:

Refactor superio_ioport_writeb to fix the out of bounds write warning.

Signed-off-by: zhanghailiang <zhang.zhanghaili...@huawei.com>
---
  hw/isa/vt82c686.c | 39 ++++++++++++++++++---------------------
  1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index e0c235c..4516af0 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 {
+        int can_write = 1;

IMHO using bool instead of int would be better here (and false, true in
the following code).

OK, will fix in v2. Thanks.

          /* 0x3f1 */
          switch (superio_conf->index) {
          case 0x00 ... 0xdf:
@@ -70,28 +70,25 @@ static void superio_ioport_writeb(void *opaque, hwaddr 
addr, uint64_t data,
          case 0xfd ... 0xff:
              can_write = 0;
              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("chage uart 1 base. unsupported yet\n");

This text looks strange. Maybe a typo?

Ah, Good catch, a typo, should be 'change'. will fix in v2, Thanks ;)

+                can_write = 0;
+            }
+            break;
+        case 0xe8:
+            if ((data & 0xff) != 0xbe) {
+                DPRINTF("chage uart 2 base. unsupported yet\n");

This text looks strange. Maybe a typo?

+                can_write = 0;
              }
+            break;
+        default:
+            break;
+
+        }
+        if (can_write) {
+            superio_conf->config[superio_conf->index] = data & 0xff;
          }
-        superio_conf->config[superio_conf->index] = data & 0xff;
      }
  }

Regards
Stefan


.