From owner-p4-projects@FreeBSD.ORG Thu May 29 17:10:56 2003 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id B2A4F37B404; Thu, 29 May 2003 17:10:55 -0700 (PDT) Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 4B9DD37B401 for ; Thu, 29 May 2003 17:10:55 -0700 (PDT) Received: from fledge.watson.org (fledge.watson.org [204.156.12.50]) by mx1.FreeBSD.org (Postfix) with ESMTP id 8D9B443F75 for ; Thu, 29 May 2003 17:10:52 -0700 (PDT) (envelope-from robert@fledge.watson.org) Received: from fledge.watson.org (localhost [127.0.0.1]) by fledge.watson.org (8.12.9/8.12.9) with ESMTP id h4U09xOn043875; Thu, 29 May 2003 20:09:59 -0400 (EDT) (envelope-from robert@fledge.watson.org) Received: from localhost (robert@localhost)h4U09xXh043872; Thu, 29 May 2003 20:09:59 -0400 (EDT) (envelope-from robert@fledge.watson.org) Date: Thu, 29 May 2003 20:09:59 -0400 (EDT) From: Robert Watson X-Sender: robert@fledge.watson.org To: Adam Migus In-Reply-To: <45980.205.227.136.1.1054252023.squirrel@mail.migus.org> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: perforce@freebsd.org Subject: Re: PERFORCE change 32072 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 May 2003 00:10:56 -0000 On Thu, 29 May 2003, Adam Migus wrote: > Sorry I failed to address a point. I assumed when writing the code that > the caller would pass in a large enough string, as that assumption is > made elsewhere in code called at this layer in the framework. To be > explicit, I think this change is therefor rendundant. Also I am > curious, asside from intentionally passing in a string that's too short > to hold the result can you make this code panic? By passing too small a buffer in from user space test program, I am able to trigger kernel memory corruption resulting in a panic. To do this, I set the buffer in user space to an artificially small value after setting the Biba or MLS label to be something that renders to quite a large string. It appears that the '\0' generally gets plopped in recently free'd memory, and so I pick it up with INVARIANTS on, since under INVARIANTS, the kernel malloc code checks that memory hasn't been modified after a free(). Obviously, there's not a 100% certainty that this particular program is triggering the panic, it could be triggered by another bug along the same code path, but after my change, I'm unable to reproduce the panic after about three hours of running the test program and exercising the kernel memory allocator. Robert N M Watson FreeBSD Core Team, TrustedBSD Projects robert@fledge.watson.org Network Associates Laboratories > > Adam > > > > Robert, > > len can never exceed size which is passed in as the > > size of the string. Therefor the NULL will always > > land inside the string. > > How exactly does snprintf() fail? > > > > Adam > > > > > >> http://perforce.freebsd.org/chv.cgi?CH=32072 > >> > >> Change 32072 by rwatson@rwatson_tislabs on > >> 2003/05/29 16:14:02 > >> > >> Temporary work-around for overflows in > >> externalization of > >> compartment strings in the Biba and MLS policies. > >> Validate > >> that the nul we slap down in fact lands inside > >> the > >> string. > >> This code generally needs cleaning up, since it > >> fails to > >> handle failures by snprintf(). If the provided > >> string is > >> too short, this result is preferable to kernel > >> panics, etc. > >> > >> Affected files ... > >> > >> .. > >> //depot/projects/trustedbsd/mac/sys/security/mac_biba/mac_biba.c#209 > >> edit .. > >> //depot/projects/trustedbsd/mac/sys/security/mac_mls/mac_mls.c#167 > >> edit > >> > >> Differences ... > >> > >> ==== > >> //depot/projects/trustedbsd/mac/sys/security/mac_biba/mac_biba.c#209 > >> (text+ko) ==== > >> > >> @@ -583,7 +583,11 @@ > >> } while(bit <= MAC_BIBA_MAX_COMPARTMENTS); > >> > >> len = size - left - 1; > >> - string[len] = '\0'; > >> + if (len > 0 && len < size) > >> + string[len] = '\0'; > >> + else > >> + string[0] = '\0'; > >> + > >> return (len); > >> > >> default: > >> > >> ==== > >> //depot/projects/trustedbsd/mac/sys/security/mac_mls/mac_mls.c#167 > >> (text+ko) ==== > >> > >> @@ -547,7 +547,10 @@ > >> } while(bit <= MAC_MLS_MAX_COMPARTMENTS); > >> > >> len = size - left - 1; > >> - string[len] = '\0'; > >> + if (len > 0 && len < size) > >> + string[len] = '\0'; > >> + else > >> + string[0] = '\0'; > >> return (len); > >> > >> default: > > > > > > -- > > Adam Migus - Research Scientist > > Network Associates Laboratories > > (http://www.nailabs.com) > > FreeBSD (http://www.freebsd.org) > > God may be subtle, but He isn't plain mean. -- > > Albert > > Einstein > > > -- > Adam Migus - Research Scientist > Network Associates Laboratories (http://www.nailabs.com) > FreeBSD (http://www.freebsd.org) > God may be subtle, but He isn't plain mean. -- Albert > Einstein > > > >