From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:47427) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RFlIP-0000rJ-K8 for qemu-devel@nongnu.org; Mon, 17 Oct 2011 07:23:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RFlIL-0002Dd-1e for qemu-devel@nongnu.org; Mon, 17 Oct 2011 07:23:53 -0400 Received: from goliath.siemens.de ([192.35.17.28]:22417) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RFlIK-0002DX-OD for qemu-devel@nongnu.org; Mon, 17 Oct 2011 07:23:49 -0400 Message-ID: <4E9C1042.4040007@siemens.com> Date: Mon, 17 Oct 2011 13:23:46 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <57386830befff359aa6eac1610816eb9c853a05d.1318843693.git.jan.kiszka@siemens.com> <20111017111045.GB4537@redhat.com> In-Reply-To: <20111017111045.GB4537@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC][PATCH 06/45] msix: Prevent bogus mask updates on MMIO accesses List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Alex Williamson , Marcelo Tosatti , Avi Kivity , "kvm@vger.kernel.org" , "qemu-devel@nongnu.org" On 2011-10-17 13:10, Michael S. Tsirkin wrote: > On Mon, Oct 17, 2011 at 11:27:40AM +0200, Jan Kiszka wrote: >> Only accesses to the MSI-X table must trigger a call to >> msix_handle_mask_update or a notifier invocation. >> >> Signed-off-by: Jan Kiszka > > Why would msix_mmio_write be called on an access > outside the table? Because it handles both the table and the PBA. > >> --- >> hw/msix.c | 16 ++++++++++------ >> 1 files changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/hw/msix.c b/hw/msix.c >> index 2c4de21..33cb716 100644 >> --- a/hw/msix.c >> +++ b/hw/msix.c >> @@ -264,18 +264,22 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr, >> { >> PCIDevice *dev = opaque; >> unsigned int offset = addr & (MSIX_PAGE_SIZE - 1) & ~0x3; >> - int vector = offset / PCI_MSIX_ENTRY_SIZE; >> + unsigned int vector = offset / PCI_MSIX_ENTRY_SIZE; > > Why the int/unsigned change? this has no chance to overflow, and using > unsigned causes signed/unsigned comparison below, > and unsigned/signed conversion on calls such as msix_is_masked. Vectors should be unsigned int, this is just one step in that direction as we are at it. Even if the overflow is practically impossible, this remains cleaner. > >> int was_masked = msix_is_masked(dev, vector); >> pci_set_long(dev->msix_table_page + offset, val); >> if (kvm_enabled() && kvm_irqchip_in_kernel()) { >> kvm_msix_update(dev, vector, was_masked, msix_is_masked(dev, vector)); >> } > > I would say if we need to check the address, check it first thing > and return if the address is out of a sensible range. Will do that later when generalized MSI-X support. > For example, are you worried about kvm_msix_update calls with > a sensible mask? No, that kvm code will die anyway. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux