smbus_eeprom: Limit data writes to 255 bytes

[Qemu-devel] [PATCH] smbus_eeprom: Limit data writes to 255 bytes

Posted by Michael Hanselmann 6 years, 10 months ago

The "eeprom_write_data" function in "smbus_eeprom.c" had no provisions
to limit the length of data written. If a caller were able to manipulate
the "len" parameter they could potentially write before or after the
target buffer.
---
 hw/i2c/smbus_eeprom.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index f18aa3de35..74fa1c328c 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -76,6 +76,7 @@ static void eeprom_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int l
        It is a block write without a length byte.  Fortunately we
        get the full block anyway.  */
     /* TODO: Should this set the current location?  */
+    len &= 0xff;
     if (cmd + len > 256)
         n = 256 - cmd;
     else
-- 
2.11.0

Re: [Qemu-devel] [PATCH] smbus_eeprom: Limit data writes to 255 bytes

Posted by Philippe Mathieu-Daudé 6 years, 10 months ago

Hi Michael,

On Thu, Dec 27, 2018 at 12:53 PM Michael Hanselmann <public@hansmi.ch> wrote:
&gt; The "eeprom_write_data" function in "smbus_eeprom.c" had no provisions
&gt; to limit the length of data written. If a caller were able to manipulate
&gt; the "len" parameter they could potentially write before or after the
&gt; target buffer.

You forgot to sign your commit:
"Signed-off-by: Michael Hanselmann <public@hansmi.ch>"

(See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297)

> ---
>  hw/i2c/smbus_eeprom.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index f18aa3de35..74fa1c328c 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -76,6 +76,7 @@ static void eeprom_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int l
>         It is a block write without a length byte.  Fortunately we
>         get the full block anyway.  */
>      /* TODO: Should this set the current location?  */
> +    len &= 0xff;
>      if (cmd + len > 256)

Corey Minyard sent a cleanup series [1] because this device model is
known to be unsafe and need rewrite.
There is a particular patch [2] which add the SMBUS_EEPROM_SIZE definition.
He also provided a intent at cleaning this problem here [3] where
Peter suggested to split it in fewer patches.

Regards,

Phil.

[1] https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg05293.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg05295.html
[3] https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg05298.html

>          n = 256 - cmd;
>      else
> --
> 2.11.0
>
>

Re: [Qemu-devel] [PATCH] smbus_eeprom: Limit data writes to 255 bytes

Posted by Michael Hanselmann 6 years, 10 months ago

Hi Philippe

On 27.12.18 20:03, Philippe Mathieu-Daudé wrote:
> On Thu, Dec 27, 2018 at 12:53 PM Michael Hanselmann <public@hansmi.ch> wrote:
> &gt; The "eeprom_write_data" function in "smbus_eeprom.c" had no provisions
> &gt; to limit the length of data written. If a caller were able to manipulate
> &gt; the "len" parameter they could potentially write before or after the
> &gt; target buffer.
> 
> You forgot to sign your commit:
> "Signed-off-by: Michael Hanselmann <public@hansmi.ch>"

Indeed I did and I'm sorry.

Signed-off-by: Michael Hanselmann <public@hansmi.ch>

>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> index f18aa3de35..74fa1c328c 100644
>> --- a/hw/i2c/smbus_eeprom.c
>> +++ b/hw/i2c/smbus_eeprom.c
>> @@ -76,6 +76,7 @@ static void eeprom_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int l
>>         It is a block write without a length byte.  Fortunately we
>>         get the full block anyway.  */
>>      /* TODO: Should this set the current location?  */
>> +    len &= 0xff;
>>      if (cmd + len > 256)
> 
> Corey Minyard sent a cleanup series [1] because this device model is
> known to be unsafe and need rewrite.
> There is a particular patch [2] which add the SMBUS_EEPROM_SIZE definition.
> He also provided a intent at cleaning this problem here [3] where
> Peter suggested to split it in fewer patches.

I agree with the assessment that the code as-is has room for
improvement, especially when it comes to the hardcoded sizes. My patch
is purely on top of the master branch (ca. QEMU 3.1.0).

Best regards,
Michael

Re: [Qemu-devel] [PATCH] smbus_eeprom: Limit data writes to 255 bytes

Posted by Paolo Bonzini 6 years, 10 months ago

On 27/12/18 12:51, Michael Hanselmann wrote:
> The "eeprom_write_data" function in "smbus_eeprom.c" had no provisions
> to limit the length of data written. If a caller were able to manipulate
> the "len" parameter they could potentially write before or after the
> target buffer.
> ---
>  hw/i2c/smbus_eeprom.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index f18aa3de35..74fa1c328c 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -76,6 +76,7 @@ static void eeprom_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int l
>         It is a block write without a length byte.  Fortunately we
>         get the full block anyway.  */
>      /* TODO: Should this set the current location?  */
> +    len &= 0xff;
>      if (cmd + len > 256)
>          n = 256 - cmd;
>      else
> 

Note that len is limited to 33 bytes (smbus_do_write and smbus_i2c_send).

Paolo

Re: [Qemu-devel] [PATCH] smbus_eeprom: Limit data writes to 255 bytes

Posted by Michael Hanselmann 6 years, 10 months ago

Hi Paolo

On 28.12.18 14:52, Paolo Bonzini wrote:
> On 27/12/18 12:51, Michael Hanselmann wrote:
>> The "eeprom_write_data" function in "smbus_eeprom.c" had no provisions
>> to limit the length of data written. If a caller were able to manipulate
>> the "len" parameter they could potentially write before or after the
>> target buffer.
>> ---
>>  hw/i2c/smbus_eeprom.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>> index f18aa3de35..74fa1c328c 100644
>> --- a/hw/i2c/smbus_eeprom.c
>> +++ b/hw/i2c/smbus_eeprom.c
>> @@ -76,6 +76,7 @@ static void eeprom_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int l
>>         It is a block write without a length byte.  Fortunately we
>>         get the full block anyway.  */
>>      /* TODO: Should this set the current location?  */
>> +    len &= 0xff;
>>      if (cmd + len > 256)
>>          n = 256 - cmd;
>>      else
>>
> 
> Note that len is limited to 33 bytes (smbus_do_write and smbus_i2c_send).

In practice it turns out to be the case. I thought I had discovered an
out-of-bounds write because hw/i2c/smbus.c:smbus_i2c_recv increases
dev->data_len unconditionally. The I2C controller implemented in
hw/i2c/aspeed_i2c.c and used by certain ARM board emulations allows
fine-grained control of the communication which allowed me to increase
data_len easily (up to and beyond an overflow if intended). It was only
the state machine in smbus.c which made it impossible to actually get to
a usable point in my experiment (increasing data_len requires
SMBUS_WRITE_DATA->SMBUS_READ_DATA, then the communication must be
stopped via NACK to avoid resetting data_len in I2C_FINISH, but there's
no way from SMBUS_DONE to SMBUS_WRITE_DATA).

Adding bitwise-and for 0xff defuses this particular situation regardless
of what state an attacker can bring the emulated devices into.

Best regards,
Michael