diff mbox

[v4,29/30] vmxnet3: validate queues configuration coming from quest

Message ID 1396275242-10810-30-git-send-email-mst@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin March 31, 2014, 2:17 p.m. UTC 
From: Dmitry Fleytman <dmitry@daynix.com>

CVE-2013-4544

Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/vmxnet3.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Dr. David Alan Gilbert March 31, 2014, 3:48 p.m. UTC | #1 
* Michael S. Tsirkin (mst@redhat.com) wrote:
> From: Dmitry Fleytman <dmitry@daynix.com>
> 
> CVE-2013-4544
> 
> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/net/vmxnet3.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 8c6df05..0fa54e7 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts(VMXNET3State *s)
>      }
>  }
>  
> +static void vmxnet3_validate_queues(VMXNET3State *s)
> +{
> +    if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
> +        hw_error("Bad TX queues number: %d\n", s->txq_num);
> +    }
> +
> +    if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
> +        hw_error("Bad RX queues number: %d\n", s->rxq_num);
> +    }

Why isn't that >= ?
(I agree it matches the original assert).

        Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
        Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];

static void vmxnet3_fill_stats(VMXNET3State *s)
{
    int i;
    for (i = 0; i < s->txq_num; i++) {
        cpu_physical_memory_write(s->txq_descr[i].tx_stats_pa,
                                  &s->txq_descr[i].txq_stats,
                                  sizeof(s->txq_descr[i].txq_stats));
    }

so that looks like it's 0 indexed.

Dave

> +}
> +
>  static void vmxnet3_activate_device(VMXNET3State *s)
>  {
>      int i;
> @@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>          VMXNET3_READ_DRV_SHARED8(s->drv_shmem, devRead.misc.numRxQueues);
>  
>      VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
> -    assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
> +    vmxnet3_validate_queues(s);
>  
>      qdescr_table_pa =
>          VMXNET3_READ_DRV_SHARED64(s->drv_shmem, devRead.misc.queueDescPA);
> -- 
> MST
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dmitry Fleytman April 1, 2014, 10:04 a.m. UTC | #2 
On Mar 31, 2014, at 18:48 PM, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:

> * Michael S. Tsirkin (mst@redhat.com) wrote:
>> From: Dmitry Fleytman <dmitry@daynix.com>
>> 
>> CVE-2013-4544
>> 
>> Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> hw/net/vmxnet3.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 8c6df05..0fa54e7 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts(VMXNET3State *s)
>>     }
>> }
>> 
>> +static void vmxnet3_validate_queues(VMXNET3State *s)
>> +{
>> +    if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
>> +        hw_error("Bad TX queues number: %d\n", s->txq_num);
>> +    }
>> +
>> +    if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
>> +        hw_error("Bad RX queues number: %d\n", s->rxq_num);
>> +    }
> 
> Why isn't that >= ?
> (I agree it matches the original assert).
> 
>        Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
>        Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
> 
> static void vmxnet3_fill_stats(VMXNET3State *s)
> {
>    int i;
>    for (i = 0; i < s->txq_num; i++) {
>        cpu_physical_memory_write(s->txq_descr[i].tx_stats_pa,
>                                  &s->txq_descr[i].txq_stats,
>                                  sizeof(s->txq_descr[i].txq_stats));
>    }
> 
> so that looks like it's 0 indexed.
> 
> Dave

HI Dave, thanks for the review.

The verification is ok because s->txq_num and s->rxq_num are total number of queues, not a queue index.

Dmitry.


> 
>> +}
>> +
>> static void vmxnet3_activate_device(VMXNET3State *s)
>> {
>>     int i;
>> @@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>>         VMXNET3_READ_DRV_SHARED8(s->drv_shmem, devRead.misc.numRxQueues);
>> 
>>     VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
>> -    assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
>> +    vmxnet3_validate_queues(s);
>> 
>>     qdescr_table_pa =
>>         VMXNET3_READ_DRV_SHARED64(s->drv_shmem, devRead.misc.queueDescPA);
>> -- 
>> MST
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Michael S. Tsirkin April 1, 2014, 2:52 p.m. UTC | #3 
On Tue, Apr 01, 2014 at 01:04:12PM +0300, Dmitry Fleytman wrote:
> 
> On Mar 31, 2014, at 18:48 PM, Dr. David Alan Gilbert <dgilbert@redhat.com>
> wrote:
> 
> 
>     * Michael S. Tsirkin (mst@redhat.com) wrote:
> 
>         From: Dmitry Fleytman <dmitry@daynix.com>
> 
>         CVE-2013-4544
> 
>         Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>         Reported-by: Michael S. Tsirkin <mst@redhat.com>
>         Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>         ---
>         hw/net/vmxnet3.c | 13 ++++++++++++-
>         1 file changed, 12 insertions(+), 1 deletion(-)
> 
>         diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>         index 8c6df05..0fa54e7 100644
>         --- a/hw/net/vmxnet3.c
>         +++ b/hw/net/vmxnet3.c
>         @@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts
>         (VMXNET3State *s)
>             }
>         }
> 
>         +static void vmxnet3_validate_queues(VMXNET3State *s)
>         +{
>         +    if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
>         +        hw_error("Bad TX queues number: %d\n", s->txq_num);
>         +    }
>         +
>         +    if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
>         +        hw_error("Bad RX queues number: %d\n", s->rxq_num);
>         +    }
> 
> 
>     Why isn't that >= ?
>     (I agree it matches the original assert).
> 
>            Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
>            Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
> 
>     static void vmxnet3_fill_stats(VMXNET3State *s)
>     {
>        int i;
>        for (i = 0; i < s->txq_num; i++) {
>            cpu_physical_memory_write(s->txq_descr[i].tx_stats_pa,
>                                      &s->txq_descr[i].txq_stats,
>                                      sizeof(s->txq_descr[i].txq_stats));
>        }
> 
>     so that looks like it's 0 indexed.
> 
>     Dave
> 
> 
> HI Dave, thanks for the review.
> 
> The verification is ok because s->txq_num and s->rxq_num are total number of
> queues, not a queue index.
> 
> Dmitry.

A code comment here might be helpful.

> 
> 
> 
> 
>         +}
>         +
>         static void vmxnet3_activate_device(VMXNET3State *s)
>         {
>             int i;
>         @@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State
>         *s)
>                 VMXNET3_READ_DRV_SHARED8(s->drv_shmem,
>         devRead.misc.numRxQueues);
> 
>             VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
>         -    assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
>         +    vmxnet3_validate_queues(s);
> 
>             qdescr_table_pa =
>                 VMXNET3_READ_DRV_SHARED64(s->drv_shmem,
>         devRead.misc.queueDescPA);
>         -- 
>         MST
> 
> 
>     --
>     Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
>
Dmitry Fleytman April 1, 2014, 6:40 p.m. UTC | #4 
> On Apr 1, 2014, at 5:52 PM, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>> On Tue, Apr 01, 2014 at 01:04:12PM +0300, Dmitry Fleytman wrote:
>> 
>> On Mar 31, 2014, at 18:48 PM, Dr. David Alan Gilbert <dgilbert@redhat.com>
>> wrote:
>> 
>> 
>>    * Michael S. Tsirkin (mst@redhat.com) wrote:
>> 
>>        From: Dmitry Fleytman <dmitry@daynix.com>
>> 
>>        CVE-2013-4544
>> 
>>        Signed-off-by: Dmitry Fleytman <dmitry@daynix.com>
>>        Reported-by: Michael S. Tsirkin <mst@redhat.com>
>>        Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>        ---
>>        hw/net/vmxnet3.c | 13 ++++++++++++-
>>        1 file changed, 12 insertions(+), 1 deletion(-)
>> 
>>        diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>>        index 8c6df05..0fa54e7 100644
>>        --- a/hw/net/vmxnet3.c
>>        +++ b/hw/net/vmxnet3.c
>>        @@ -1336,6 +1336,17 @@ static void vmxnet3_validate_interrupts
>>        (VMXNET3State *s)
>>            }
>>        }
>> 
>>        +static void vmxnet3_validate_queues(VMXNET3State *s)
>>        +{
>>        +    if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
>>        +        hw_error("Bad TX queues number: %d\n", s->txq_num);
>>        +    }
>>        +
>>        +    if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
>>        +        hw_error("Bad RX queues number: %d\n", s->rxq_num);
>>        +    }
>> 
>> 
>>    Why isn't that >= ?
>>    (I agree it matches the original assert).
>> 
>>           Vmxnet3RxqDescr rxq_descr[VMXNET3_DEVICE_MAX_RX_QUEUES];
>>           Vmxnet3TxqDescr txq_descr[VMXNET3_DEVICE_MAX_TX_QUEUES];
>> 
>>    static void vmxnet3_fill_stats(VMXNET3State *s)
>>    {
>>       int i;
>>       for (i = 0; i < s->txq_num; i++) {
>>           cpu_physical_memory_write(s->txq_descr[i].tx_stats_pa,
>>                                     &s->txq_descr[i].txq_stats,
>>                                     sizeof(s->txq_descr[i].txq_stats));
>>       }
>> 
>>    so that looks like it's 0 indexed.
>> 
>>    Dave
>> 
>> 
>> HI Dave, thanks for the review.
>> 
>> The verification is ok because s->txq_num and s->rxq_num are total number of
>> queues, not a queue index.
>> 
>> Dmitry.
> 
> A code comment here might be helpful.

No problem. I'll resend these two patches with fixed comments.

> 
>> 
>> 
>> 
>> 
>>        +}
>>        +
>>        static void vmxnet3_activate_device(VMXNET3State *s)
>>        {
>>            int i;
>>        @@ -1382,7 +1393,7 @@ static void vmxnet3_activate_device(VMXNET3State
>>        *s)
>>                VMXNET3_READ_DRV_SHARED8(s->drv_shmem,
>>        devRead.misc.numRxQueues);
>> 
>>            VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
>>        -    assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
>>        +    vmxnet3_validate_queues(s);
>> 
>>            qdescr_table_pa =
>>                VMXNET3_READ_DRV_SHARED64(s->drv_shmem,
>>        devRead.misc.queueDescPA);
>>        -- 
>>        MST
>> 
>> 
>>    --
>>    Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>> 
>>
diff mbox

Patch

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 8c6df05..0fa54e7 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1336,6 +1336,17 @@  static void vmxnet3_validate_interrupts(VMXNET3State *s)
     }
 }
 
+static void vmxnet3_validate_queues(VMXNET3State *s)
+{
+    if (s->txq_num > VMXNET3_DEVICE_MAX_TX_QUEUES) {
+        hw_error("Bad TX queues number: %d\n", s->txq_num);
+    }
+
+    if (s->rxq_num > VMXNET3_DEVICE_MAX_RX_QUEUES) {
+        hw_error("Bad RX queues number: %d\n", s->rxq_num);
+    }
+}
+
 static void vmxnet3_activate_device(VMXNET3State *s)
 {
     int i;
@@ -1382,7 +1393,7 @@  static void vmxnet3_activate_device(VMXNET3State *s)
         VMXNET3_READ_DRV_SHARED8(s->drv_shmem, devRead.misc.numRxQueues);
 
     VMW_CFPRN("Number of TX/RX queues %u/%u", s->txq_num, s->rxq_num);
-    assert(s->txq_num <= VMXNET3_DEVICE_MAX_TX_QUEUES);
+    vmxnet3_validate_queues(s);
 
     qdescr_table_pa =
         VMXNET3_READ_DRV_SHARED64(s->drv_shmem, devRead.misc.queueDescPA);