diff mbox series

smbus_eeprom: Limit data writes to 255 bytes

Message ID e0d6e64a5f4b3ed436bb91aa3428986a06a0f1f1.1545911443.git.public@hansmi.ch (mailing list archive)
State New, archived
Headers show
Series smbus_eeprom: Limit data writes to 255 bytes | expand

Commit Message

Michael Hanselmann Dec. 27, 2018, 11:51 a.m. UTC 
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(+)

Comments

Philippe Mathieu-Daudé Dec. 27, 2018, 7:03 p.m. UTC | #1 
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
>
>
Paolo Bonzini Dec. 28, 2018, 1:52 p.m. UTC | #2 
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
Michael Hanselmann Dec. 28, 2018, 4:38 p.m. UTC | #3 
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
Michael Hanselmann Dec. 28, 2018, 4:46 p.m. UTC | #4 
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
diff mbox series

Patch

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