| Message ID | 1354736883-22417-1-git-send-email-alex.horn@cs.ox.ac.uk |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 5, 2012 at 7:48 PM, Alex Horn <alex.horn@cs.ox.ac.uk> wrote: > The private buffer length field must only be incremented after the I2C > frame has been transmitted. > > To expose this bug, assume the temperature in the TMP105 hardware model > is +0.125 C (e.g. snow slush). Note that eleven bit precision is required > to read this value; otherwise the reading is equal to zero centigrade (ice). > > Continue by considering the following I2C protocol steps: > > 1) Start transfer with I2C_START_SEND > 2) Send byte 0x01 (i.e. configuration register) > 3) Send byte 0x40 (i.e. eleven bit precision) > 4) End transfer with I2C_FINISH > > 5) Start transfer with I2C_START_SEND > 6) Send byte 0x00 (i.e. temperature register) > 7) End transfer I2C_FINISH > > 8) Start transfer with I2C_START_RECV > 9) Receive high-order byte of temperature > ... > > In step (1), the function tmp105_tx() is called. By the conditional > check !s->len and the side effect with ++, s->len is equal to 1 when > step (2) begins. Thus, 0x40 is written to s->buf[1] in step (3). > By definition of tmp105_write(), s->config is set to zero in step (3). > Thus, when we read the higher-order byte in step (9), it is zero! > > In other words, the TMP105 hardware model allows us to measure 0 C (ice) > even with eleven bit precision when, in fact, it should be 0.125 C (slush)! > > Signed-off-by: Alex Horn <alex.horn@cs.ox.ac.uk> > --- > hw/tmp105.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/hw/tmp105.c b/hw/tmp105.c > index 8e8dbd9..5f41a3f 100644 > --- a/hw/tmp105.c > +++ b/hw/tmp105.c > @@ -152,7 +152,7 @@ static int tmp105_tx(I2CSlave *i2c, uint8_t data) > { > TMP105State *s = (TMP105State *) i2c; > > - if (!s->len ++) > + if (s->len == 0) Please fix coding style (add braces) while touching this line. QEMU code is not yet consistent with our CODING_STYLE, but changes should make progress towards that. > s->pointer = data; > else { > if (s->len <= 2) > @@ -160,6 +160,7 @@ static int tmp105_tx(I2CSlave *i2c, uint8_t data) > tmp105_write(s); > } > > + s->len ++; Please remove the space between s->len and ++. However, I don't think the change is entirely correct since the 'else' clause currently seems to take the increment into account: if (s->len <= 2) s->buf[s->len - 1] = data; tmp105_write(s); Shouldn't the '- 1' in the middle line be removed? > return 0; > } > > -- > 1.7.6.5 > >
> [T]he change is entirely correct since the 'else' clause currently seems > to take the increment into account: > if (s->len <= 2) > s->buf[s->len - 1] = data; > tmp105_write(s); > > Shouldn't the '- 1' in the middle line be removed? Several clarifications follow. The bug seems to occur because the else clause does not correctly account for the increment (i.e. !s->len ++). If this increment operation in the conditional statement should remain there, the assignment "s->buf[s->len - 1] = data" would appear to have to be changed to "s->buf[s->len - 2] = data". However, this offset adjustment would only shadow the root cause of the bug. To further clarify this bug, I have also created a standalone unit test: https://github.com/ahorn/benchmarks/blob/master/qemu-hw/tmp105/tmp105-test.c#L130 This test case would fail in the unmodified tmp105.c hardware model. That is, you can produce the runtime assertion failure by reverting the fix I have implemented in tmp105_tx() in the file https://github.com/ahorn/benchmarks/blob/master/qemu-hw/tmp105/tmp105.c#L140 There is also one correction about the bug description itself (see below). >> 8) Start transfer with I2C_START_RECV >> 9) Receive high-order byte of temperature >> ... >> Thus, when we read the higher-order byte in step (9), it is zero! This description should read instead: [as before] ... 8) Start transfer with I2C_START_RECV 9) Receive high-order byte of temperature 10) Receive low-order byte of temperature Thus, when we read the low-order byte in step (10), it is zero. The rest of the argument remains the same. I hope this correction and the unit test help in clarifying the source of the error. I would greatly appreciate your thoughts on this. With best regards, Alex On 6 December 2012 21:21, Blue Swirl <blauwirbel@gmail.com> wrote: > On Wed, Dec 5, 2012 at 7:48 PM, Alex Horn <alex.horn@cs.ox.ac.uk> wrote: >> The private buffer length field must only be incremented after the I2C >> frame has been transmitted. >> >> To expose this bug, assume the temperature in the TMP105 hardware model >> is +0.125 C (e.g. snow slush). Note that eleven bit precision is required >> to read this value; otherwise the reading is equal to zero centigrade (ice). >> >> Continue by considering the following I2C protocol steps: >> >> 1) Start transfer with I2C_START_SEND >> 2) Send byte 0x01 (i.e. configuration register) >> 3) Send byte 0x40 (i.e. eleven bit precision) >> 4) End transfer with I2C_FINISH >> >> 5) Start transfer with I2C_START_SEND >> 6) Send byte 0x00 (i.e. temperature register) >> 7) End transfer I2C_FINISH >> >> 8) Start transfer with I2C_START_RECV >> 9) Receive high-order byte of temperature >> ... >> >> In step (1), the function tmp105_tx() is called. By the conditional >> check !s->len and the side effect with ++, s->len is equal to 1 when >> step (2) begins. Thus, 0x40 is written to s->buf[1] in step (3). >> By definition of tmp105_write(), s->config is set to zero in step (3). >> Thus, when we read the higher-order byte in step (9), it is zero! >> >> In other words, the TMP105 hardware model allows us to measure 0 C (ice) >> even with eleven bit precision when, in fact, it should be 0.125 C (slush)! >> >> Signed-off-by: Alex Horn <alex.horn@cs.ox.ac.uk> >> --- >> hw/tmp105.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/hw/tmp105.c b/hw/tmp105.c >> index 8e8dbd9..5f41a3f 100644 >> --- a/hw/tmp105.c >> +++ b/hw/tmp105.c >> @@ -152,7 +152,7 @@ static int tmp105_tx(I2CSlave *i2c, uint8_t data) >> { >> TMP105State *s = (TMP105State *) i2c; >> >> - if (!s->len ++) >> + if (s->len == 0) > > Please fix coding style (add braces) while touching this line. > > QEMU code is not yet consistent with our CODING_STYLE, but changes > should make progress towards that. > >> s->pointer = data; >> else { >> if (s->len <= 2) >> @@ -160,6 +160,7 @@ static int tmp105_tx(I2CSlave *i2c, uint8_t data) >> tmp105_write(s); >> } >> >> + s->len ++; > > Please remove the space between s->len and ++. However, I don't think > the change is entirely correct since the 'else' clause currently seems > to take the increment into account: > if (s->len <= 2) > s->buf[s->len - 1] = data; > tmp105_write(s); > > Shouldn't the '- 1' in the middle line be removed? > >> return 0; >> } >> >> -- >> 1.7.6.5 >> >>
diff --git a/hw/tmp105.c b/hw/tmp105.c index 8e8dbd9..5f41a3f 100644 --- a/hw/tmp105.c +++ b/hw/tmp105.c @@ -152,7 +152,7 @@ static int tmp105_tx(I2CSlave *i2c, uint8_t data) { TMP105State *s = (TMP105State *) i2c; - if (!s->len ++) + if (s->len == 0) s->pointer = data; else { if (s->len <= 2) @@ -160,6 +160,7 @@ static int tmp105_tx(I2CSlave *i2c, uint8_t data) tmp105_write(s); } + s->len ++; return 0; }
The private buffer length field must only be incremented after the I2C frame has been transmitted. To expose this bug, assume the temperature in the TMP105 hardware model is +0.125 C (e.g. snow slush). Note that eleven bit precision is required to read this value; otherwise the reading is equal to zero centigrade (ice). Continue by considering the following I2C protocol steps: 1) Start transfer with I2C_START_SEND 2) Send byte 0x01 (i.e. configuration register) 3) Send byte 0x40 (i.e. eleven bit precision) 4) End transfer with I2C_FINISH 5) Start transfer with I2C_START_SEND 6) Send byte 0x00 (i.e. temperature register) 7) End transfer I2C_FINISH 8) Start transfer with I2C_START_RECV 9) Receive high-order byte of temperature ... In step (1), the function tmp105_tx() is called. By the conditional check !s->len and the side effect with ++, s->len is equal to 1 when step (2) begins. Thus, 0x40 is written to s->buf[1] in step (3). By definition of tmp105_write(), s->config is set to zero in step (3). Thus, when we read the higher-order byte in step (9), it is zero! In other words, the TMP105 hardware model allows us to measure 0 C (ice) even with eleven bit precision when, in fact, it should be 0.125 C (slush)! Signed-off-by: Alex Horn <alex.horn@cs.ox.ac.uk> --- hw/tmp105.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)