Re: [Qemu-devel] [PATCH 1/1] tmp105: Fix I2C protocol bug

[Blue Swirl]

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