Re: [PATCH] hw/net/i82596.c: Avoid reading off end of buffer in i82596_receive()

[Helge Deller <deller@gmx.de>]

On 17.03.20 07:13, Jason Wang wrote:
> 
> On 2020/3/13 上午4:16, Peter Maydell wrote:
>> The i82596_receive() function attempts to pass the guest a buffer
>> which is effectively the concatenation of the data it is passed and a
>> 4 byte CRC value.  However, rather than implementing this as "write
>> the data; then write the CRC" it instead bumps the length value of
>> the data by 4, and writes 4 extra bytes from beyond the end of the
>> buffer, which it then overwrites with the CRC.  It also assumed that
>> we could always fit all four bytes of the CRC into the final receive
>> buffer, which might not be true if the CRC needs to be split over two
>> receive buffers.
>>
>> Calculate separately how many bytes we need to transfer into the
>> guest's receive buffer from the source buffer, and how many we need
>> to transfer from the CRC work.
>>
>> We add a count 'bufsz' of the number of bytes left in the source
>> buffer, which we use purely to assert() that we don't overrun.
>>
>> Spotted by Coverity (CID 1419396) for the specific case when we end
>> up using a local array as the source buffer.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> I know Helge has some significant rework of this device planned, but
>> for 5.0 we need to fix the buffer overrun.
>>
>> Tested with 'make check' only.
>> ---
>>   hw/net/i82596.c | 44 +++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 35 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/net/i82596.c b/hw/net/i82596.c
>> index fe9f2390a94..2bd5d310367 100644
>> --- a/hw/net/i82596.c
>> +++ b/hw/net/i82596.c
>> @@ -501,7 +501,8 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
>>       uint32_t rfd_p;
>>       uint32_t rbd;
>>       uint16_t is_broadcast = 0;
>> -    size_t len = sz;
>> +    size_t len = sz; /* length of data for guest (including CRC) */
>> +    size_t bufsz = sz; /* length of data in buf */
>>       uint32_t crc;
>>       uint8_t *crc_ptr;
>>       uint8_t buf1[MIN_BUF_SIZE + VLAN_HLEN];
>> @@ -595,6 +596,7 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
>>           if (len < MIN_BUF_SIZE) {
>>               len = MIN_BUF_SIZE;
>>           }
>> +        bufsz = len;
>>       }
>>         /* Calculate the ethernet checksum (4 bytes) */
>> @@ -627,6 +629,7 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
>>           while (len) {
>>               uint16_t buffer_size, num;
>>               uint32_t rba;
>> +            size_t bufcount, crccount;
>>                 /* printf("Receive: rbd is %08x\n", rbd); */
>>               buffer_size = get_uint16(rbd + 12);
>> @@ -639,14 +642,37 @@ ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
>>               }
>>               rba = get_uint32(rbd + 8);
>>               /* printf("rba is 0x%x\n", rba); */
>> -            address_space_write(&address_space_memory, rba,
>> -                                MEMTXATTRS_UNSPECIFIED, buf, num);
>> -            rba += num;
>> -            buf += num;
>> -            len -= num;
>> -            if (len == 0) { /* copy crc */
>> -                address_space_write(&address_space_memory, rba - 4,
>> -                                    MEMTXATTRS_UNSPECIFIED, crc_ptr, 4);
>> +            /*
>> +             * Calculate how many bytes we want from buf[] and how many
>> +             * from the CRC.
>> +             */
>> +            if ((len - num) >= 4) {
>> +                /* The whole guest buffer, we haven't hit the CRC yet */
>> +                bufcount = num;
>> +            } else {
>> +                /* All that's left of buf[] */
>> +                bufcount = len - 4;
>> +            }
>> +            crccount = num - bufcount;
>> +
>> +            if (bufcount > 0) {
>> +                /* Still some of the actual data buffer to transfer */
>> +                bufsz -= bufcount;
>> +                assert(bufsz >= 0);
>> +                address_space_write(&address_space_memory, rba,
>> +                                    MEMTXATTRS_UNSPECIFIED, buf, bufcount);
>> +                rba += bufcount;
>> +                buf += bufcount;
>> +                len -= bufcount;
>> +            }
>> +
>> +            /* Write as much of the CRC as fits */
>> +            if (crccount > 0) {
>> +                address_space_write(&address_space_memory, rba,
>> +                                    MEMTXATTRS_UNSPECIFIED, crc_ptr, crccount);
>> +                rba += crccount;
>> +                crc_ptr += crccount;
>> +                len -= crccount;
>>               }
>>                 num |= 0x4000; /* set F BIT */
> 
> 
> Applied.

Thank you, Peter and Jason!

Helge