From owner-freebsd-audit@FreeBSD.ORG Thu Oct 9 04:09:52 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 58F0416A4B3 for ; Thu, 9 Oct 2003 04:09:52 -0700 (PDT) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id 72BCA43FBF for ; Thu, 9 Oct 2003 04:09:49 -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 VAA25190; Thu, 9 Oct 2003 21:09:30 +1000 Date: Thu, 9 Oct 2003 21:08:30 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Mark Murray In-Reply-To: <200310090748.h997m4YJ018758@grimreaper.grondar.org> Message-ID: <20031009200740.R9199@gamplex.bde.org> References: <200310090748.h997m4YJ018758@grimreaper.grondar.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 11:09:52 -0000 On Thu, 9 Oct 2003, Mark Murray wrote: > Hiroki Sato writes: > > Could anyone please review a patch in bin/56502? That fixes a bug > > in random.c that causes memory corruption. > > It looks fine to me. Has it been tested on all platforms? It changes far too much and increases type mismatch problems. 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).] * cleanups have been made as well. A warning for both initstate and * setstate has been inserted to the effect that on Sparc platforms * the 'arg_state' variable must be forced to begin on word boundaries. [I can't see the warning, except in this comment.] * This can be easily done by casting a long integer array to char *. [This is an (undocumented except here :-() requirement on the caller of initstate().] ... /* * 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'.] char * initstate(seed, arg_state, n) unsigned long seed; /* seed for R.N.G. */ char *arg_state; /* pointer to state array */ long n; /* # bytes of state info */ { char *ostate = (char *)(&state[-1]); long *long_arg_state = (long *) arg_state; [This bogus casts hides the bug.] if (rand_type == TYPE_0) state[-1] = rand_type; else state[-1] = MAX_TYPES * (rptr - state) + rand_type; if (n < BREAK_0) { (void)fprintf(stderr, "random: not enough state (%ld bytes); ignored.\n", n); return(0); } ... [Similarly for setstate().] %%% The "28 December 1994" changes were made in rev.1.5 (Lite2 merge). The patch in the PR uses a different bogus cast: uint32_t *int_arg_state = (uint32_t *)(void *)arg_state; This apparently works by reducing the alignment requirements a little. Howver, I'm surprised that aligning `b' actually clobbers `c' in the test program: static int *a; static char b[256]; static int *c; I would have thought that this gave 'a', b and c following each other in memory, with all of them very aligned because things are naturally aligned following 'a' and 256 is a large enough power of 2. The following would force misalignement on most machines: struct { long align; char misalign[sizeof(long) - 1]; char misaligned_state[256]; char always_clobbered; }; For a quick fix, I would print a message and return 0 if (void *)long_arg_state != (void *)arg_state. Bruce