Re: [Qemu-devel] [RFC][PATCH 06/45] msix: Prevent bogus mask updates on MMIO accesses
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 jan.kis...@siemens.com
Why would msix_mmio_write be called on an access
outside the table?
---
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.
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.
For example, are you worried about kvm_msix_update calls with
a sensible mask?
-if (was_masked != msix_is_masked(dev, vector)
dev-msix_mask_notifier) {
-int r = dev-msix_mask_notifier(dev, vector,
- msix_is_masked(dev, vector));
-assert(r = 0);
+
+if (vector dev-msix_entries_nr) {
+if (was_masked != msix_is_masked(dev, vector)
+dev-msix_mask_notifier) {
+int r = dev-msix_mask_notifier(dev, vector,
+msix_is_masked(dev, vector));
+assert(r = 0);
+}
+msix_handle_mask_update(dev, vector);
}
-msix_handle_mask_update(dev, vector);
}
static const MemoryRegionOps msix_mmio_ops = {
--
1.7.3.4
Re: [Qemu-devel] [RFC][PATCH 06/45] msix: Prevent bogus mask updates on MMIO accesses
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 jan.kis...@siemens.com
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
Re: [Qemu-devel] [RFC][PATCH 06/45] msix: Prevent bogus mask updates on MMIO accesses
On Mon, Oct 17, 2011 at 01:23:46PM +0200, Jan Kiszka wrote:
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 jan.kis...@siemens.com
Why would msix_mmio_write be called on an access
outside the table?
Because it handles both the table and the PBA.
Hmm. Interesting. Is there a bug in how we handle PBA
updates then? If yes I'd like a separate patch for that
to apply to the stable tree.
BTW, this code will go away if PBA can get stored separately?
---
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
Not sure why, but if you want to rework this we
would be better off doing the conversion in one go. Making half the code
use unsigned and half signed is way worse.
as we are at it.
Should be a separate patch.
Even if the overflow is practically impossible, this
remains cleaner.
I have to say this change if done throughout would introduce
a lot of code churn. The potential of introducing bugs
seems higher than the potential to find/fix them.
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.
But then do we need this patch at all?
For example, are you worried about kvm_msix_update calls with
a sensible mask?
No, that kvm code will die anyway.
Yes but we care about stable too, if there's a bug there
we need to fix it.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 06/45] msix: Prevent bogus mask updates on MMIO accesses
On 2011-10-17 13:57, Michael S. Tsirkin wrote: On Mon, Oct 17, 2011 at 01:23:46PM +0200, Jan Kiszka wrote: 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 jan.kis...@siemens.com Why would msix_mmio_write be called on an access outside the table? Because it handles both the table and the PBA. Hmm. Interesting. Is there a bug in how we handle PBA updates then? If yes I'd like a separate patch for that to apply to the stable tree. I first thought it was a serious bug, but it just triggers if the guest write to PBA (which is very uncommon) and that actually triggers any spurious out-of-bounds vector injection. Highly unlikely. BTW, this code will go away if PBA can get stored separately? Hmm - yeah, true. Likely it's moot to discuss this change then. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 06/45] msix: Prevent bogus mask updates on MMIO accesses
On Mon, Oct 17, 2011 at 02:07:10PM +0200, Jan Kiszka wrote: On 2011-10-17 13:57, Michael S. Tsirkin wrote: On Mon, Oct 17, 2011 at 01:23:46PM +0200, Jan Kiszka wrote: 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 jan.kis...@siemens.com Why would msix_mmio_write be called on an access outside the table? Because it handles both the table and the PBA. Hmm. Interesting. Is there a bug in how we handle PBA updates then? If yes I'd like a separate patch for that to apply to the stable tree. I first thought it was a serious bug, but it just triggers if the guest write to PBA (which is very uncommon) and that actually triggers any spurious out-of-bounds vector injection. Highly unlikely. Yes guests don't really use PBA ATM. But is there something bad a malicious guest can do? For example, what if msix_clr_pending gets invoked with this huge vector value? It does seem serious ... BTW, this code will go away if PBA can get stored separately? Hmm - yeah, true. Likely it's moot to discuss this change then. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [RFC][PATCH 06/45] msix: Prevent bogus mask updates on MMIO accesses
On Mon, Oct 17, 2011 at 09:11:29PM +0200, Jan Kiszka wrote: On 2011-10-17 14:50, Michael S. Tsirkin wrote: On Mon, Oct 17, 2011 at 02:07:10PM +0200, Jan Kiszka wrote: On 2011-10-17 13:57, Michael S. Tsirkin wrote: On Mon, Oct 17, 2011 at 01:23:46PM +0200, Jan Kiszka wrote: 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 jan.kis...@siemens.com Why would msix_mmio_write be called on an access outside the table? Because it handles both the table and the PBA. Hmm. Interesting. Is there a bug in how we handle PBA updates then? If yes I'd like a separate patch for that to apply to the stable tree. I first thought it was a serious bug, but it just triggers if the guest write to PBA (which is very uncommon) and that actually triggers any spurious out-of-bounds vector injection. Highly unlikely. Yes guests don't really use PBA ATM. But is there something bad a malicious guest can do? For example, what if msix_clr_pending gets invoked with this huge vector value? It does seem serious ... I checked it before and I think it is harmless. The largest vector that can be miscalculated is 255. But bit 255 in the PBA is still safe inside our MMIO page. Jan you are right. we got lucky.
Re: [Qemu-devel] [RFC][PATCH 06/45] msix: Prevent bogus mask updates on MMIO accesses
On 2011-10-17 14:50, Michael S. Tsirkin wrote: On Mon, Oct 17, 2011 at 02:07:10PM +0200, Jan Kiszka wrote: On 2011-10-17 13:57, Michael S. Tsirkin wrote: On Mon, Oct 17, 2011 at 01:23:46PM +0200, Jan Kiszka wrote: 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 jan.kis...@siemens.com Why would msix_mmio_write be called on an access outside the table? Because it handles both the table and the PBA. Hmm. Interesting. Is there a bug in how we handle PBA updates then? If yes I'd like a separate patch for that to apply to the stable tree. I first thought it was a serious bug, but it just triggers if the guest write to PBA (which is very uncommon) and that actually triggers any spurious out-of-bounds vector injection. Highly unlikely. Yes guests don't really use PBA ATM. But is there something bad a malicious guest can do? For example, what if msix_clr_pending gets invoked with this huge vector value? It does seem serious ... I checked it before and I think it is harmless. The largest vector that can be miscalculated is 255. But bit 255 in the PBA is still safe inside our MMIO page. Jan signature.asc Description: OpenPGP digital signature
