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
signature.asc
Description: OpenPGP digital signature
