From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52355) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tqqsi-0005T7-6m for qemu-devel@nongnu.org; Thu, 03 Jan 2013 14:55:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tqqsf-0000NV-7D for qemu-devel@nongnu.org; Thu, 03 Jan 2013 14:55:12 -0500 Date: Thu, 3 Jan 2013 13:54:55 -0600 From: Scott Wood In-Reply-To: <2E33728A-086A-494D-98DE-D43F94D25E23@suse.de> (from agraf@suse.de on Thu Jan 3 12:55:26 2013) Message-ID: <1357242895.22404.3@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 12:55:26 PM, Alexander Graf wrote: >=20 > On 22.12.2012, at 03:15, Scott Wood wrote: >=20 > > The two checks with abort() guard against potential QEMU-internal > > problems, but the EOI check stops the guest from causing updates to =20 > queue > > position -1 and other havoc if it writes EOI with no interrupt in > > service. > > > > Signed-off-by: Scott Wood >=20 > Did you ever actually experience this? Which one? EOI with no interrupt in service can be triggered by bad =20 guest behavior, and I did see it happen when the guest was confused by =20 another bug in QEMU's openpic (which is fixed elsewhere), resulting in =20 an IRQ number of -1 being thrown around. The other checks were to try =20 to be more robust against bad IRQ numbers in general. > MAX_IRQ should match the memory region size, so we shouldn't be able =20 > to receive any interrupt above it. Right, that's why I didn't add checking to the MMIO code. In IRQ_check =20 it could happen due to bad bitmap contents (e.g. after a checkpoint =20 restore), and in openpic_set_irq() it could happen if some device =20 raises an IRQ that is out of bounds. > I might be inclined to accept an assert() there for internal sanity =20 > 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. -Scott=