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

[Scott Wood]

On 01/03/2013 03:07:49 PM, Alexander Graf wrote:
> On 03.01.2013, at 20:54, Scott Wood wrote:
>
> > 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 <address@hidden>
> >> 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.
>
> That's the last hunk, which as I said is fine :).

I would have found the issue in that hunk faster if I had array bounds  
checking elsewhere, which was what led me to add it in certain places.   
I'm not sure why I didn't add it in the place that would have helped  
find the EOI bug, though (IRQ_resetbit). :-P

> > 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.
>
> How would a device raise an IRQ that is out of bounds? Devices can  
> only raise IRQs that are passed down from the init function and that  
> only creates MAX_INT irq lines.

OK, so it looks like there would need to be a bug in the qdev gpio  
mechanism rather than the devices -- but the interface boundary of  
openpic.c does take an int rather than a pointer.

> >> 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.
>
> Well, that's why I was asking where you've seen this happen. It  
> really shouldn't. Ever. :)

That's why it's assert/abort and not some less severe form of error  
handling. :-)

-Scott