From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40181) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TqsDO-0000Lb-VM for qemu-devel@nongnu.org; Thu, 03 Jan 2013 16:20:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TqsDM-00081y-1A for qemu-devel@nongnu.org; Thu, 03 Jan 2013 16:20:38 -0500 Date: Thu, 3 Jan 2013 15:20:22 -0600 From: Scott Wood In-Reply-To: (from agraf@suse.de on Thu Jan 3 15:07:49 2013) Message-ID: <1357248022.22404.11@snotra> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 13/15] openpic: add some bounds checking for IRQ numbers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org On 01/03/2013 03:07:49 PM, Alexander Graf wrote: >=20 > On 03.01.2013, at 20:54, Scott Wood wrote: >=20 > > 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 =20 > to queue > >> > position -1 and other havoc if it writes EOI with no interrupt in > >> > service. > >> > > >> > Signed-off-by: Scott Wood > >> Did you ever actually experience this? > > > > Which one? EOI with no interrupt in service can be triggered by =20 > bad guest behavior, and I did see it happen when the guest was =20 > confused by another bug in QEMU's openpic (which is fixed elsewhere), =20 > resulting in an IRQ number of -1 being thrown around. >=20 > 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 =20 checking elsewhere, which was what led me to add it in certain places. =20 I'm not sure why I didn't add it in the place that would have helped =20 find the EOI bug, though (IRQ_resetbit). :-P > > The other checks were to try to be more robust against bad IRQ =20 > numbers in general. > > > >> MAX_IRQ should match the memory region size, so we shouldn't be =20 > able to receive any interrupt above it. > > > > Right, that's why I didn't add checking to the MMIO code. In =20 > IRQ_check it could happen due to bad bitmap contents (e.g. after a =20 > checkpoint restore), and in openpic_set_irq() it could happen if some =20 > device raises an IRQ that is out of bounds. >=20 > How would a device raise an IRQ that is out of bounds? Devices can =20 > only raise IRQs that are passed down from the init function and that =20 > only creates MAX_INT irq lines. OK, so it looks like there would need to be a bug in the qdev gpio =20 mechanism rather than the devices -- but the interface boundary of =20 openpic.c does take an int rather than a pointer. > >> I might be inclined to accept an assert() there for internal =20 > sanity checking though. The last hunk looks fine. > > > > Assert instead of abort is fine (there seem to be plenty of uses of =20 > both in QEMU), though for the openpic_set_irq() case it would be nice =20 > to be able to print the bad IRQ number before dying. >=20 > Well, that's why I was asking where you've seen this happen. It =20 > really shouldn't. Ever. :) That's why it's assert/abort and not some less severe form of error =20 handling. :-) -Scott=