[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] smbus_eeprom: Limit data writes to 255 bytes
From: |
Michael Hanselmann |
Subject: |
Re: [Qemu-devel] [PATCH] smbus_eeprom: Limit data writes to 255 bytes |
Date: |
Fri, 28 Dec 2018 17:38:37 +0100 |
Hi Philippe
On 27.12.18 20:03, Philippe Mathieu-Daudé wrote:
> On Thu, Dec 27, 2018 at 12:53 PM Michael Hanselmann <address@hidden> 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.
>
> You forgot to sign your commit:
> "Signed-off-by: Michael Hanselmann <address@hidden>"
Indeed I did and I'm sorry.
Signed-off-by: Michael Hanselmann <address@hidden>
>> 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
signature.asc
Description: OpenPGP digital signature