diff mbox series

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

Message ID 20200312201638.6375-1-peter.maydell@linaro.org
State Superseded
Headers show
Series hw/net/i82596.c: Avoid reading off end of buffer in i82596_receive() | expand

Commit Message

Peter Maydell March 12, 2020, 8:16 p.m. UTC 
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(-)

-- 
2.20.1

Comments

Jason Wang March 17, 2020, 6:13 a.m. UTC | #1 
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
Helge Deller March 17, 2020, 7:06 p.m. UTC | #2 
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
Peter Maydell March 26, 2020, 9:11 p.m. UTC | #3 
On Tue, 17 Mar 2020 at 06:13, Jason Wang <jasowang@redhat.com> 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.


> Applied.


Hi Jason -- this doesn't seem to have reached master yet.
Has it gotten lost somewhere along the line?

thanks
-- PMM
Jason Wang March 27, 2020, 2:10 a.m. UTC | #4 
On 2020/3/27 上午5:11, Peter Maydell wrote:
> On Tue, 17 Mar 2020 at 06:13, Jason Wang <jasowang@redhat.com> 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.

>> Applied.

> Hi Jason -- this doesn't seem to have reached master yet.

> Has it gotten lost somewhere along the line?

>

> thanks

> -- PMM



Nope, it's in my queue.

I will send a pull request shortly.

Thanks
diff mbox series

Patch

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 */