Re: [Qemu-devel] [PATCH v4 29/30] vmxnet3: validate queues configuration coming from quest

[Michael S. Tsirkin]

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 <address@hidden>
> wrote:
> 
> 
>     * Michael S. Tsirkin (address@hidden) wrote:
> 
>         From: Dmitry Fleytman <address@hidden>
> 
>         CVE-2013-4544
> 
>         Signed-off-by: Dmitry Fleytman <address@hidden>
>         Reported-by: Michael S. Tsirkin <address@hidden>
>         Signed-off-by: Michael S. Tsirkin <address@hidden>
>         ---
>         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 / address@hidden / Manchester, UK
> 
>