From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37645) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WVkAd-0005gE-Uc for qemu-devel@nongnu.org; Thu, 03 Apr 2014 12:07:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WVkAW-0000Cx-Rt for qemu-devel@nongnu.org; Thu, 03 Apr 2014 12:07:15 -0400 Date: Thu, 3 Apr 2014 19:07:37 +0300 From: "Michael S. Tsirkin" Message-ID: <20140403160737.GA18418@redhat.com> 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> <20140401130752.GL2411@work-vm> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140401130752.GL2411@work-vm> 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: "Dr. David Alan Gilbert" Cc: Anthony Liguori , qemu-stable@nongnu.org, qemu-devel@nongnu.org, Stefan Hajnoczi , Dmitry Fleytman , Paolo Bonzini , mdroth@linux.vnet.ibm.com On Tue, Apr 01, 2014 at 02:07:52PM +0100, Dr. David Alan Gilbert wrote: > * 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 OK Dmitry, can you please resend patches with suggested comment changes? > > > > > > > > > >> + > > >> /* 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