| Message ID | AANLkTik-S2V3B-rs37eTPumc8A8spvaCBk6Y3SqBHWEX@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Blue Swirl <blauwirbel@gmail.com> writes: > Fix a buffer overflow, reported by cppcheck: > [/src/qemu/hw/lan9118.c:849]: (error) Buffer access out-of-bounds: s.eeprom > > All eeprom handling code assumes that the size of eeprom is 128. > > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > --- > hw/lan9118.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/hw/lan9118.c b/hw/lan9118.c > index a988664..1bb829e 100644 > --- a/hw/lan9118.c > +++ b/hw/lan9118.c > @@ -187,7 +187,7 @@ typedef struct { > uint32_t phy_int_mask; > > int eeprom_writable; > - uint8_t eeprom[8]; > + uint8_t eeprom[128]; > > int tx_fifo_size; > LAN9118Packet *txp; Covers all the obvious accesses except for a couple of s->eeprom[addr] in lan9118_eeprom_cmd(). addr is a parameter there, and the actual argument is val & 0xff, in lan9118_writel(). What if val & 0xff >= 128?
On Mon, Jan 10, 2011 at 12:45 PM, Markus Armbruster <armbru@redhat.com> wrote: > Blue Swirl <blauwirbel@gmail.com> writes: > >> Fix a buffer overflow, reported by cppcheck: >> [/src/qemu/hw/lan9118.c:849]: (error) Buffer access out-of-bounds: s.eeprom >> >> All eeprom handling code assumes that the size of eeprom is 128. >> >> Signed-off-by: Blue Swirl <blauwirbel@gmail.com> >> --- >> hw/lan9118.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/hw/lan9118.c b/hw/lan9118.c >> index a988664..1bb829e 100644 >> --- a/hw/lan9118.c >> +++ b/hw/lan9118.c >> @@ -187,7 +187,7 @@ typedef struct { >> uint32_t phy_int_mask; >> >> int eeprom_writable; >> - uint8_t eeprom[8]; >> + uint8_t eeprom[128]; >> >> int tx_fifo_size; >> LAN9118Packet *txp; > > Covers all the obvious accesses except for a couple of s->eeprom[addr] > in lan9118_eeprom_cmd(). addr is a parameter there, and the actual > argument is val & 0xff, in lan9118_writel(). What if val & 0xff >= 128? Should the size be 256 and cases with 128 changed accordingly? Or mask changed to 0x7f?
On 10 January 2011 15:50, Blue Swirl <blauwirbel@gmail.com> wrote: > On Mon, Jan 10, 2011 at 12:45 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Covers all the obvious accesses except for a couple of s->eeprom[addr] >> in lan9118_eeprom_cmd(). addr is a parameter there, and the actual >> argument is val & 0xff, in lan9118_writel(). What if val & 0xff >= 128? > > Should the size be 256 and cases with 128 changed accordingly? Or mask > changed to 0x7f? Size should be 128, I think. The SMSC 9118 datasheet: http://www.smsc.com/media/Downloads_Public/Data_Sheets/9118.pdf says it supports "most “93C46” type EEPROMs configured for 128 x 8-bit operation", and if you look at the timing diagram in figure 3.8 EEDIO is outputting address bits A0 to A6. The data sheet doesn't say what the actual effect of writing a bit-8-set value to E2P_CMD's address field is, but "ignore the high bit" seems like a good guess. -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 10 January 2011 15:50, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Mon, Jan 10, 2011 at 12:45 PM, Markus Armbruster <armbru@redhat.com> wrote: >>> Covers all the obvious accesses except for a couple of s->eeprom[addr] >>> in lan9118_eeprom_cmd(). addr is a parameter there, and the actual >>> argument is val & 0xff, in lan9118_writel(). What if val & 0xff >= 128? >> >> Should the size be 256 and cases with 128 changed accordingly? Or mask >> changed to 0x7f? > > Size should be 128, I think. The SMSC 9118 datasheet: > http://www.smsc.com/media/Downloads_Public/Data_Sheets/9118.pdf > says it supports "most “93C46” type EEPROMs configured for > 128 x 8-bit operation", and if you look at the timing diagram in > figure 3.8 EEDIO is outputting address bits A0 to A6. > The data sheet doesn't say what the actual effect of writing a > bit-8-set value to E2P_CMD's address field is, but "ignore the > high bit" seems like a good guess. That answers the question I was about to ask: how large is the real hardware's EEPROM. Let's mask with 0x7f.
diff --git a/hw/lan9118.c b/hw/lan9118.c index a988664..1bb829e 100644 --- a/hw/lan9118.c +++ b/hw/lan9118.c @@ -187,7 +187,7 @@ typedef struct { uint32_t phy_int_mask; int eeprom_writable; - uint8_t eeprom[8]; + uint8_t eeprom[128]; int tx_fifo_size; LAN9118Packet *txp;
Fix a buffer overflow, reported by cppcheck: [/src/qemu/hw/lan9118.c:849]: (error) Buffer access out-of-bounds: s.eeprom All eeprom handling code assumes that the size of eeprom is 128. Signed-off-by: Blue Swirl <blauwirbel@gmail.com> --- hw/lan9118.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)