From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60184) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WUyQE-0002Z1-RL for qemu-devel@nongnu.org; Tue, 01 Apr 2014 09:08:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WUyQ6-0001pB-Fs for qemu-devel@nongnu.org; Tue, 01 Apr 2014 09:08:10 -0400 Date: Tue, 1 Apr 2014 14:07:52 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20140401130752.GL2411@work-vm> References: <1396275242-10810-1-git-send-email-mst@redhat.com> <1396275242-10810-28-git-send-email-mst@redhat.com> <20140401113349.GJ2411@work-vm> <724FDAF6-88A2-49B1-B334-EFE23506923B@daynix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <724FDAF6-88A2-49B1-B334-EFE23506923B@daynix.com> Subject: Re: [Qemu-devel] [PATCH v4 27/30] vmxnet3: validate interrupt indices coming from guest List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dmitry Fleytman Cc: Anthony Liguori , "Michael S. Tsirkin" , qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com, qemu-stable@nongnu.org, Stefan Hajnoczi , Paolo Bonzini * Dmitry Fleytman (dmitry@daynix.com) wrote: > > On Apr 1, 2014, at 14:33 PM, Dr. David Alan Gilbert wrote: > > > * Michael S. Tsirkin (mst@redhat.com) wrote: > >> From: Dmitry Fleytman > >> > >> CVE-2013-4544 > >> > >> Signed-off-by: Dmitry Fleytman > >> Reported-by: Michael S. Tsirkin > >> Signed-off-by: Michael S. Tsirkin > >> --- > >> hw/net/vmxnet3.c | 36 ++++++++++++++++++++++++++++++++++-- > >> 1 file changed, 34 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > >> index 5be807c..a4b5c11 100644 > >> --- a/hw/net/vmxnet3.c > >> +++ b/hw/net/vmxnet3.c > >> @@ -52,6 +52,9 @@ > >> #define VMXNET3_DEVICE_VERSION 0x1 > >> #define VMXNET3_DEVICE_REVISION 0x1 > >> > >> +/* Number of interrupt vectors for INTx/MSI */ > >> +#define VMXNET3_MAX_NMSIX_INTRS (1) > > > > As per Dmitry's reply this is apparently the number of non-MSIX > > interrupts; can we change the comment to make this clear. > > Not sure how to change it. > There are three modes of operation: > 1. INTx - 1 interrupt is used > 2. MSI - 1 interrupt is used > 3. MSIx - up to 25 interrupts are used. > > This define covers 2 first modes of operation. > Would something like > > /* Number of interrupt vectors for non-MSIx modes */ > > be better? Yes - that's fine. Dave > > > > > >> + > >> /* Macros for rings descriptors access */ > >> #define VMXNET3_READ_TX_QUEUE_DESCR8(dpa, field) \ > >> (vmw_shmem_ld8(dpa + offsetof(struct Vmxnet3_TxQueueDesc, field))) > >> @@ -1305,6 +1308,34 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx) > >> (pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1)); > >> } > >> > >> +static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx) > >> +{ > >> + int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS; > >> + if (idx >= max_ints) { > >> + hw_error("Bad interrupt index: %d\n", idx); > >> + } > > > > Can we avoid hw_error here, we're using in this in state load, so I'm > > OK at the thought of error_report, but then we should make > > this return a bool to say it's failed and bubble this up so > > that it just fails the post_load test like any other bad state load. > > > > Dave > > > >> +} > >> + > >> +static void vmxnet3_validate_interrupts(VMXNET3State *s) > >> +{ > >> + int i; > >> + > >> + VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx); > >> + vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx); > >> + > >> + for (i = 0; i < s->txq_num; i++) { > >> + int idx = s->txq_descr[i].intr_idx; > >> + VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx); > >> + vmxnet3_validate_interrupt_idx(s->msix_used, idx); > >> + } > >> + > >> + for (i = 0; i < s->rxq_num; i++) { > >> + int idx = s->rxq_descr[i].intr_idx; > >> + VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx); > >> + vmxnet3_validate_interrupt_idx(s->msix_used, idx); > >> + } > >> +} > >> + > >> static void vmxnet3_activate_device(VMXNET3State *s) > >> { > >> int i; > >> @@ -1447,6 +1478,8 @@ static void vmxnet3_activate_device(VMXNET3State *s) > >> sizeof(s->rxq_descr[i].rxq_stats)); > >> } > >> > >> + vmxnet3_validate_interrupts(s); > >> + > >> /* Make sure everything is in place before device activation */ > >> smp_wmb(); > >> > >> @@ -2005,7 +2038,6 @@ vmxnet3_cleanup_msix(VMXNET3State *s) > >> } > >> } > >> > >> -#define VMXNET3_MSI_NUM_VECTORS (1) > >> #define VMXNET3_MSI_OFFSET (0x50) > >> #define VMXNET3_USE_64BIT (true) > >> #define VMXNET3_PER_VECTOR_MASK (false) > >> @@ -2016,7 +2048,7 @@ vmxnet3_init_msi(VMXNET3State *s) > >> PCIDevice *d = PCI_DEVICE(s); > >> int res; > >> > >> - res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MSI_NUM_VECTORS, > >> + res = msi_init(d, VMXNET3_MSI_OFFSET, VMXNET3_MAX_NMSIX_INTRS, > >> VMXNET3_USE_64BIT, VMXNET3_PER_VECTOR_MASK); > >> if (0 > res) { > >> VMW_WRPRN("Failed to initialize MSI, error %d", res); > >> -- > >> MST > >> > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK