From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49408) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaCbF-0000YN-KW for qemu-devel@nongnu.org; Mon, 23 Mar 2015 20:21:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaCbA-0003UD-S9 for qemu-devel@nongnu.org; Mon, 23 Mar 2015 20:21:41 -0400 Date: Tue, 24 Mar 2015 11:22:17 +1100 From: David Gibson Message-ID: <20150324002217.GU25043@voom.fritz.box> References: <1427075508-12099-1-git-send-email-david@gibson.dropbear.id.au> <1427075508-12099-3-git-send-email-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="l/3WCmqZNZ+BnHMk" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 2/2] i6300esb: Fix signed integer overflow List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: BALATON Zoltan Cc: pbonzini@redhat.com, mst@redhat.com, qemu-ppc@nongnu.org, rjones@redhat.com, qemu-devel@nongnu.org --l/3WCmqZNZ+BnHMk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Mar 23, 2015 at 10:54:39AM +0100, BALATON Zoltan wrote: > On Mon, 23 Mar 2015, David Gibson wrote: > >If the guest programs a sufficiently large timeout value an integer > >overflow can occur in i6300esb_restart_timer(). e.g. if the maximum > >possible timer preload value of 0xfffff is programmed then we end up with > >the calculation: > > > >timeout =3D get_ticks_per_sec() * (0xfffff << 15) / 33000000; > > > >get_ticks_per_sec() returns 1000000000 (10^9) giving: > > > > 10^9 * (0xfffff * 2^15) =3D=3D 0x1dcd632329b000000 (65 bits) > > > >Obviously the division by 33MHz brings it back under 64-bits, but the > >overflow has already occurred. > > > >Since signed integer overflow has undefined behaviour in C, in theory th= is > >could be arbitrarily bad. In practice, the overflowed value wraps around > >to something negative, causing the watchdog to immediately expire, killi= ng > >the guest, which is still fairly bad. > > > >The bug can be triggered by running a Linux guest, loading the i6300esb > >driver with parameter "heartbeat=3D2046" and opening /dev/watchdog. The > >watchdog will trigger as soon as the device is opened. > > > >This patch corrects the problem by using muldiv64(), which effectively > >allows a 128-bit intermediate value between the multiplication and > >division. > > > >Signed-off-by: David Gibson > >--- > >hw/watchdog/wdt_i6300esb.c | 10 ++++++++-- > >1 file changed, 8 insertions(+), 2 deletions(-) > > > >diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c > >index e694fa9..c7316f5 100644 > >--- a/hw/watchdog/wdt_i6300esb.c > >+++ b/hw/watchdog/wdt_i6300esb.c > >@@ -125,8 +125,14 @@ static void i6300esb_restart_timer(I6300State *d, i= nt stage) > > else > > timeout <<=3D 5; > > > >- /* Get the timeout in units of ticks_per_sec. */ > >- timeout =3D get_ticks_per_sec() * timeout / 33000000; > >+ /* Get the timeout in units of ticks_per_sec. > >+ * > >+ * ticks_per_sec is typically 10^9 =3D=3D 0x3B9ACA00 (30 bits), with > >+ * 20 bits of user supplied preload, and 15 bits of scale, the > >+ * multiply here can exceed 64-bits, before we divide by 33MHz, so > >+ * we use a 128-bit temporary > >+ */ >=20 > Is the comment still correct saying "we use a 128-bit temporary" when the > code does not do that explicitely any more? Bother. I fixed the commit message, but not this comment. It's still kind of correct, in that muldiv64 does effectively have a 128-bit temporary internally. Not quite what I meant, and a little misleading though. Paolo, worth fixing? --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --l/3WCmqZNZ+BnHMk Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVEK45AAoJEGw4ysog2bOSqGMP/17ZPsNLMGNDOzhPNj2XSYSD 8sHaYilgEGhBST0MiwoqXtJBklP623gLG9xgxC3l4AxrMQe2FDuGDOcLFXp7fnfE CuUF4JgOX2uT/gdjidc9hwn3XK9yiI3HYa+aJ20UuI9EFtd4vdgDLXV7zJYEQGE6 pJt8yfMrMm056kKAQkO3YO5HxP9JVFgdToWnN8UDw60U3A/hery8DgAXjBBLrdjb AcbrMBemMLmlZHvQ+fqHgrsBmXP5STu+4kA1CuW3RiWg8Cy68ix+FMwQrbEERB3I 03+akVzALhMPobMswrGCBvZzq/+LyP2rC27NOBaKkhNmTRICNKRk1WCveNaDjylX iunxJz+S4fIUjLzQJbT0Ph8iEIsQC6jv8KMRZgUFo3ZZmdJDfCT/AwVbDWpD7wpV HQVBUBHGrhRlIJvmBMRU1goGAXjyp4W9joGNNjuNTF4bwhth5DUad2kseKafIpiK BT4lNyLXAWCrUKBd5DIUrXoJTTMS0W+6jnIYKc7B8WrX1lk2DHZg2/14+G0VuIgP 1ThGAPTCSaFK//Tl4EfyYqvX4WMCKdnOfD5AmIDyIJkBcOqdywDMVZ3UpMuauXLf NgJefFp10pvzixeRLwwGWSQ/mtcCPJE2BKyp8vp2uxiggRalKOrazysUk+FLnMrv cDRqr6fQIwxuYbsFDgGH =KVBw -----END PGP SIGNATURE----- --l/3WCmqZNZ+BnHMk--