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.mayd...@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
