[Qemu-devel] [PATCH 6/7] lan9118: fix a buffer overflow

[Qemu-devel] [PATCH 6/7] lan9118: fix a buffer overflow

[Blue Swirl]

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;
-- 
1.6.2.4

Re: [Qemu-devel] [PATCH 6/7] lan9118: fix a buffer overflow

[Markus Armbruster]

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?

Re: [Qemu-devel] [PATCH 6/7] lan9118: fix a buffer overflow

[Blue Swirl]

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?

Re: [Qemu-devel] [PATCH 6/7] lan9118: fix a buffer overflow

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 6/7] lan9118: fix a buffer overflow

[Markus Armbruster]

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.