[Qemu-devel] [PATCH 0/4] ds1338 I2C RTC+NVRAM: various fixes

[Qemu-devel] [PATCH 0/4] ds1338 I2C RTC+NVRAM: various fixes

[Peter Maydell]

Clang's static analyzer drew my attention to the mishandling of the
register pointer in ds1338_send(); one thing led to another and I fixed
a few other things while I was there.

There seems a reasonable chance that the overrun of nvram[] is
guest-exploitable, but I assume nobody treats realview or versatilepb
models as a security boundary...

Peter Maydell (4):
  hw/ds1338: Fix mishandling of register pointer
  hw/ds1338: Recapture current time when register pointer wraps around
  hw/ds1338: Remove 'now' field from state struct
  hw/ds1338: Implement state save/restore

 hw/ds1338.c |  123 +++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 86 insertions(+), 37 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/4] hw/ds1338: Fix mishandling of register pointer

[Peter Maydell]

Correct several deficiencies in the handling of the register pointer:
 * it should wrap around after 0x3f, not 0xff
 * guard against the caller handing us an out of range pointer
   (on h/w this can never happen, because only a 7 bit value is
   transferred over the I2C bus)
 * there was confusion over whether nvram[] holds only the 56 bytes
   of guest-accessible NVRAM, or also the secondary registers
   which hold the value of the clock captured at the start of a
   multibyte read. Correct to consistently be the latter, by fixing
   the array size and the offset used for NVRAM writes.
 * ds1338_send was attempting to use 'data' as both the data and
   the register offset simultaneously, which meant that writes to
   any register were broken; fix to use the register pointer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ds1338.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/ds1338.c b/hw/ds1338.c
index d590d9c..be68140 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -12,11 +12,16 @@
 
 #include "i2c.h"
 
+/* Size of NVRAM including both the user-accessible area and the
+ * secondary register area.
+ */
+#define NVRAM_SIZE 64
+
 typedef struct {
     I2CSlave i2c;
     time_t offset;
     struct tm now;
-    uint8_t nvram[56];
+    uint8_t nvram[NVRAM_SIZE];
     int ptr;
     int addr_byte;
 } DS1338State;
@@ -57,7 +62,7 @@ static int ds1338_recv(I2CSlave *i2c)
     uint8_t res;
 
     res  = s->nvram[s->ptr];
-    s->ptr = (s->ptr + 1) & 0xff;
+    s->ptr = (s->ptr + 1) & (NVRAM_SIZE - 1);
     return res;
 }
 
@@ -65,14 +70,13 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
 {
     DS1338State *s = FROM_I2C_SLAVE(DS1338State, i2c);
     if (s->addr_byte) {
-        s->ptr = data;
+        s->ptr = data & (NVRAM_SIZE - 1);
         s->addr_byte = 0;
         return 0;
     }
-    s->nvram[s->ptr - 8] = data;
-    if (data < 8) {
+    if (s->ptr < 8) {
         qemu_get_timedate(&s->now, s->offset);
-        switch(data) {
+        switch(s->ptr) {
         case 0:
             /* TODO: Implement CH (stop) bit.  */
             s->now.tm_sec = from_bcd(data & 0x7f);
@@ -109,8 +113,10 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
             break;
         }
         s->offset = qemu_timedate_diff(&s->now);
+    } else {
+        s->nvram[s->ptr] = data;
     }
-    s->ptr = (s->ptr + 1) & 0xff;
+    s->ptr = (s->ptr + 1) & (NVRAM_SIZE - 1);
     return 0;
 }
 
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/4] hw/ds1338: Recapture current time when register pointer wraps around

[Peter Maydell]

The DS1338 datasheet documents that the current time is captured into
the secondary registers when the register pointer wraps round to zero
as well as at a START condition. Implement this.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ds1338.c |   59 ++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 17 deletions(-)

diff --git a/hw/ds1338.c b/hw/ds1338.c
index be68140..842d2de 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -26,27 +26,52 @@ typedef struct {
     int addr_byte;
 } DS1338State;
 
+static void capture_current_time(DS1338State *s)
+{
+    /* Capture the current time into the secondary registers
+     * which will be actually read by the data transfer operation.
+     */
+    qemu_get_timedate(&s->now, s->offset);
+    s->nvram[0] = to_bcd(s->now.tm_sec);
+    s->nvram[1] = to_bcd(s->now.tm_min);
+    if (s->nvram[2] & 0x40) {
+        s->nvram[2] = (to_bcd((s->now.tm_hour % 12)) + 1) | 0x40;
+        if (s->now.tm_hour >= 12) {
+            s->nvram[2] |= 0x20;
+        }
+    } else {
+        s->nvram[2] = to_bcd(s->now.tm_hour);
+    }
+    s->nvram[3] = to_bcd(s->now.tm_wday) + 1;
+    s->nvram[4] = to_bcd(s->now.tm_mday);
+    s->nvram[5] = to_bcd(s->now.tm_mon) + 1;
+    s->nvram[6] = to_bcd(s->now.tm_year - 100);
+}
+
+static void inc_regptr(DS1338State *s)
+{
+    /* The register pointer wraps around after 0x3F; wraparound
+     * causes the current time/date to be retransferred into
+     * the secondary registers.
+     */
+    s->ptr = (s->ptr + 1) & (NVRAM_SIZE - 1);
+    if (!s->ptr) {
+        capture_current_time(s);
+    }
+}
+
 static void ds1338_event(I2CSlave *i2c, enum i2c_event event)
 {
     DS1338State *s = FROM_I2C_SLAVE(DS1338State, i2c);
 
     switch (event) {
     case I2C_START_RECV:
-        qemu_get_timedate(&s->now, s->offset);
-        s->nvram[0] = to_bcd(s->now.tm_sec);
-        s->nvram[1] = to_bcd(s->now.tm_min);
-        if (s->nvram[2] & 0x40) {
-            s->nvram[2] = (to_bcd((s->now.tm_hour % 12)) + 1) | 0x40;
-            if (s->now.tm_hour >= 12) {
-                s->nvram[2] |= 0x20;
-            }
-        } else {
-            s->nvram[2] = to_bcd(s->now.tm_hour);
-        }
-        s->nvram[3] = to_bcd(s->now.tm_wday) + 1;
-        s->nvram[4] = to_bcd(s->now.tm_mday);
-        s->nvram[5] = to_bcd(s->now.tm_mon) + 1;
-        s->nvram[6] = to_bcd(s->now.tm_year - 100);
+        /* In h/w, capture happens on any START condition, not just a
+         * START_RECV, but there is no need to actually capture on
+         * START_SEND, because the guest can't get at that data
+         * without going through a START_RECV which would overwrite it.
+         */
+        capture_current_time(s);
         break;
     case I2C_START_SEND:
         s->addr_byte = 1;
@@ -62,7 +87,7 @@ static int ds1338_recv(I2CSlave *i2c)
     uint8_t res;
 
     res  = s->nvram[s->ptr];
-    s->ptr = (s->ptr + 1) & (NVRAM_SIZE - 1);
+    inc_regptr(s);
     return res;
 }
 
@@ -116,7 +141,7 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
     } else {
         s->nvram[s->ptr] = data;
     }
-    s->ptr = (s->ptr + 1) & (NVRAM_SIZE - 1);
+    inc_regptr(s);
     return 0;
 }
 
-- 
1.7.9.5

[Qemu-devel] [PATCH 3/4] hw/ds1338: Remove 'now' field from state struct

[Peter Maydell]

The 'struct tm now' field in the state structure is in fact only
ever used as a temporary (the actual RTC state is held in 'offset').
Remove it from the state structure in favour of using local variables
to avoid confusion about whether it needs to be saved on migration.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ds1338.c |   41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/hw/ds1338.c b/hw/ds1338.c
index 842d2de..16aba4b 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -20,7 +20,6 @@
 typedef struct {
     I2CSlave i2c;
     time_t offset;
-    struct tm now;
     uint8_t nvram[NVRAM_SIZE];
     int ptr;
     int addr_byte;
@@ -31,21 +30,22 @@ static void capture_current_time(DS1338State *s)
     /* Capture the current time into the secondary registers
      * which will be actually read by the data transfer operation.
      */
-    qemu_get_timedate(&s->now, s->offset);
-    s->nvram[0] = to_bcd(s->now.tm_sec);
-    s->nvram[1] = to_bcd(s->now.tm_min);
+    struct tm now;
+    qemu_get_timedate(&now, s->offset);
+    s->nvram[0] = to_bcd(now.tm_sec);
+    s->nvram[1] = to_bcd(now.tm_min);
     if (s->nvram[2] & 0x40) {
-        s->nvram[2] = (to_bcd((s->now.tm_hour % 12)) + 1) | 0x40;
-        if (s->now.tm_hour >= 12) {
+        s->nvram[2] = (to_bcd((now.tm_hour % 12)) + 1) | 0x40;
+        if (now.tm_hour >= 12) {
             s->nvram[2] |= 0x20;
         }
     } else {
-        s->nvram[2] = to_bcd(s->now.tm_hour);
+        s->nvram[2] = to_bcd(now.tm_hour);
     }
-    s->nvram[3] = to_bcd(s->now.tm_wday) + 1;
-    s->nvram[4] = to_bcd(s->now.tm_mday);
-    s->nvram[5] = to_bcd(s->now.tm_mon) + 1;
-    s->nvram[6] = to_bcd(s->now.tm_year - 100);
+    s->nvram[3] = to_bcd(now.tm_wday) + 1;
+    s->nvram[4] = to_bcd(now.tm_mday);
+    s->nvram[5] = to_bcd(now.tm_mon) + 1;
+    s->nvram[6] = to_bcd(now.tm_year - 100);
 }
 
 static void inc_regptr(DS1338State *s)
@@ -100,14 +100,15 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
         return 0;
     }
     if (s->ptr < 8) {
-        qemu_get_timedate(&s->now, s->offset);
+        struct tm now;
+        qemu_get_timedate(&now, s->offset);
         switch(s->ptr) {
         case 0:
             /* TODO: Implement CH (stop) bit.  */
-            s->now.tm_sec = from_bcd(data & 0x7f);
+            now.tm_sec = from_bcd(data & 0x7f);
             break;
         case 1:
-            s->now.tm_min = from_bcd(data & 0x7f);
+            now.tm_min = from_bcd(data & 0x7f);
             break;
         case 2:
             if (data & 0x40) {
@@ -119,25 +120,25 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
             } else {
                 data = from_bcd(data);
             }
-            s->now.tm_hour = data;
+            now.tm_hour = data;
             break;
         case 3:
-            s->now.tm_wday = from_bcd(data & 7) - 1;
+            now.tm_wday = from_bcd(data & 7) - 1;
             break;
         case 4:
-            s->now.tm_mday = from_bcd(data & 0x3f);
+            now.tm_mday = from_bcd(data & 0x3f);
             break;
         case 5:
-            s->now.tm_mon = from_bcd(data & 0x1f) - 1;
+            now.tm_mon = from_bcd(data & 0x1f) - 1;
             break;
         case 6:
-            s->now.tm_year = from_bcd(data) + 100;
+            now.tm_year = from_bcd(data) + 100;
             break;
         case 7:
             /* Control register. Currently ignored.  */
             break;
         }
-        s->offset = qemu_timedate_diff(&s->now);
+        s->offset = qemu_timedate_diff(&now);
     } else {
         s->nvram[s->ptr] = data;
     }
-- 
1.7.9.5

[Qemu-devel] [PATCH 4/4] hw/ds1338: Implement state save/restore

[Peter Maydell]

Implement state save/restore for the DS1338. This requires
the usual minor adjustment of types in the state struct to
get fixed-width ones with vmstate macros.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ds1338.c |   27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/hw/ds1338.c b/hw/ds1338.c
index 16aba4b..b576d56 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -19,12 +19,27 @@
 
 typedef struct {
     I2CSlave i2c;
-    time_t offset;
+    int64_t offset;
     uint8_t nvram[NVRAM_SIZE];
-    int ptr;
-    int addr_byte;
+    int32_t ptr;
+    bool addr_byte;
 } DS1338State;
 
+static const VMStateDescription vmstate_ds1338 = {
+    .name = "ds1338",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_I2C_SLAVE(i2c, DS1338State),
+        VMSTATE_INT64(offset, DS1338State),
+        VMSTATE_UINT8_ARRAY(nvram, DS1338State, NVRAM_SIZE),
+        VMSTATE_INT32(ptr, DS1338State),
+        VMSTATE_BOOL(addr_byte, DS1338State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void capture_current_time(DS1338State *s)
 {
     /* Capture the current time into the secondary registers
@@ -74,7 +89,7 @@ static void ds1338_event(I2CSlave *i2c, enum i2c_event event)
         capture_current_time(s);
         break;
     case I2C_START_SEND:
-        s->addr_byte = 1;
+        s->addr_byte = true;
         break;
     default:
         break;
@@ -96,7 +111,7 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
     DS1338State *s = FROM_I2C_SLAVE(DS1338State, i2c);
     if (s->addr_byte) {
         s->ptr = data & (NVRAM_SIZE - 1);
-        s->addr_byte = 0;
+        s->addr_byte = false;
         return 0;
     }
     if (s->ptr < 8) {
@@ -153,12 +168,14 @@ static int ds1338_init(I2CSlave *i2c)
 
 static void ds1338_class_init(ObjectClass *klass, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(klass);
     I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
 
     k->init = ds1338_init;
     k->event = ds1338_event;
     k->recv = ds1338_recv;
     k->send = ds1338_send;
+    dc->vmsd = &vmstate_ds1338;
 }
 
 static TypeInfo ds1338_info = {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 0/4] ds1338 I2C RTC+NVRAM: various fixes

[Peter Maydell]

On 24 September 2012 19:33, Peter Maydell <peter.maydell@linaro.org> wrote:
> Clang's static analyzer drew my attention to the mishandling of the
> register pointer in ds1338_send(); one thing led to another and I fixed
> a few other things while I was there.
>
> There seems a reasonable chance that the overrun of nvram[] is
> guest-exploitable, but I assume nobody treats realview or versatilepb
> models as a security boundary...
>
> Peter Maydell (4):
>   hw/ds1338: Fix mishandling of register pointer
>   hw/ds1338: Recapture current time when register pointer wraps around
>   hw/ds1338: Remove 'now' field from state struct
>   hw/ds1338: Implement state save/restore

These will go into the next arm-devs pullreq unless I get any
review comments in the next few days...

-- PMM