Re: [Qemu-devel] [Qemu-ppc] [PATCH 13/15] openpic: add some bounds checking for IRQ numbers

[Scott Wood <scottwood@freescale.com>]

On 01/03/2013 12:55:26 PM, Alexander Graf wrote:
> 
> On 22.12.2012, at 03:15, Scott Wood wrote:
> 
> > The two checks with abort() guard against potential QEMU-internal
> > problems, but the EOI check stops the guest from causing updates to  
> queue
> > position -1 and other havoc if it writes EOI with no interrupt in
> > service.
> >
> > Signed-off-by: Scott Wood <scottwood@freescale.com>
> 
> Did you ever actually experience this?

Which one?  EOI with no interrupt in service can be triggered by bad  
guest behavior, and I did see it happen when the guest was confused by  
another bug in QEMU's openpic (which is fixed elsewhere), resulting in  
an IRQ number of -1 being thrown around.  The other checks were to try  
to be more robust against bad IRQ numbers in general.

> MAX_IRQ should match the memory region size, so we shouldn't be able  
> to receive any interrupt above it.

Right, that's why I didn't add checking to the MMIO code.  In IRQ_check  
it could happen due to bad bitmap contents (e.g. after a checkpoint  
restore), and in openpic_set_irq() it could happen if some device  
raises an IRQ that is out of bounds.

> I might be inclined to accept an assert() there for internal sanity  
> checking though. The last hunk looks fine.

Assert instead of abort is fine (there seem to be plenty of uses of  
both in QEMU), though for the openpic_set_irq() case it would be nice  
to be able to print the bad IRQ number before dying.

-Scott