From owner-freebsd-audit@FreeBSD.ORG Thu Oct 9 05:14:06 2003 Return-Path: Delivered-To: freebsd-audit@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 0793916A4BF for ; Thu, 9 Oct 2003 05:14:06 -0700 (PDT) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id 3C9A743FCB for ; Thu, 9 Oct 2003 05:14:04 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246]) by mailman.zeta.org.au (8.9.3p2/8.8.7) with ESMTP id WAA32389; Thu, 9 Oct 2003 22:13:54 +1000 Date: Thu, 9 Oct 2003 22:12:30 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Mark Murray In-Reply-To: <20031009200740.R9199@gamplex.bde.org> Message-ID: <20031009213822.L9492@gamplex.bde.org> References: <200310090748.h997m4YJ018758@grimreaper.grondar.org> <20031009200740.R9199@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: audit@freebsd.org cc: Hiroki Sato Subject: Re: bin/56502 X-BeenThere: freebsd-audit@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: FreeBSD Security Audit List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Oct 2003 12:14:06 -0000 On Thu, 9 Oct 2003, Bruce Evans wrote: > > Hiroki Sato writes: > > > Could anyone please review a patch in bin/56502? That fixes a bug > > > in random.c that causes memory corruption. > The test program gives undefined behaviour. From random.c: > > %%% > * Modified 28 December 1994 by Jacob S. Rosenberg. > * The following changes have been made: > * All references to the type u_int have been changed to unsigned long. > * All references to type int have been changed to type long. Other > > [The proposed fix sort of reverts this, except it mostly uses uint32_t, > which may be better but may cause sign extension problems. Places that > use plain int are broken on machines where sizeof(int) < sizeof(int32_t).] > ... > /* > * initstate: > ... > * Note: The Sparc platform requires that arg_state begin on a long > * word boundary; otherwise a bus error will occur. Even so, lint will > * complain about mis-alignment, but you should disregard these messages. > */ > > [The test program doesn't do this. It has 'static char b[256]' followed > by 'static int *c' and passes &b[0]. Alignment of the pointer apparently > increases it, so using all of the 256 bytes following the aligned pointer > overruns `b'. The increase is apparently enough for the overrun to reach > 'c'.] > %%% > > The "28 December 1994" changes were made in rev.1.5 (Lite2 merge). Bah, the alignment problem is secondary. I naively assumed that the bug wasn't related to longs being larger than ints since it hasn't been reported for other machines, but in fact it causes buffer overrun by a factor of sizeof(long)/4 on all machines where this factor is > 1 (I tested on alphas). This is fixed in NetBSD. Our rev.1.5 is mostly wrong. NetBSD obtained the same bug by importing Lite2 in their rev.1.7, but fixed it in their rev.1.20. Some diffs between NetBSD 1.7 and 1.22: 112a119,123 > * > * Modified 07 January 2002 by Jason R. Thorpe. > * The following changes have been made: > * All the references to "long" have been changed back to "int". This > * fixes memory corruption problems on LP64 platforms. This basically moves the bug from the large set of machines where sizeof(long) != 4 to the small set of machines where sizeof(int) != 4. s/int/int32_t/ not quite as in the PR would be a more complete fix for the main problem, but I don't like it since it puts magic 32's all over the code. The bug is caused by magic 4's not even literally present: % if (n < BREAK_1) { % rand_type = TYPE_0; % rand_deg = DEG_0; % rand_sep = SEP_0; % } else if (n < BREAK_2) { % rand_type = TYPE_1; % rand_deg = DEG_1; % rand_sep = SEP_1; % } else if (n < BREAK_3) { % rand_type = TYPE_2; % rand_deg = DEG_2; % rand_sep = SEP_2; % } else if (n < BREAK_4) { % rand_type = TYPE_3; % rand_deg = DEG_3; % rand_sep = SEP_3; % } else { % rand_type = TYPE_4; % rand_deg = DEG_4; % rand_sep = SEP_4; % } % state = (long *) (long_arg_state + 1); /* first location */ % end_ptr = &state[rand_deg]; /* must set end_ptr before srandom */ Here BREAK_N are literal constsants, but they should be related to sizeof(state[0]). E.g., BREAK_4 is 256, which is enough for 64 32-bit longs, but it needs to be enough for 64 elements in the state array, so it isn't enough for 64-bit longs. Bruce