| Message ID | 57386830befff359aa6eac1610816eb9c853a05d.1318843693.git.jan.kiszka@siemens.com |
|---|---|
| State | New |
| Headers | show |
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.kiszka@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
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.kiszka@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
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.kiszka@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
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.kiszka@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
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.kiszka@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
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.kiszka@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
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.kiszka@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.
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; 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)); } - 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 = {
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.kiszka@siemens.com> --- hw/msix.c | 16 ++++++++++------ 1 files changed, 10 insertions(+), 6 deletions(-)