On Wed, Dec 5, 2012 at 7:48 PM, Alex Horn <alex.h...@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.h...@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
>
>
