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

[Jason Wang <jasowang@redhat.com>]

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.

Thanks