From owner-freebsd-current@FreeBSD.ORG Sat Mar 21 19:57:02 2015 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 2179657C; Sat, 21 Mar 2015 19:57:02 +0000 (UTC) Received: from mail-wi0-x232.google.com (mail-wi0-x232.google.com [IPv6:2a00:1450:400c:c05::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id A066F1C0; Sat, 21 Mar 2015 19:57:01 +0000 (UTC) Received: by wibgn9 with SMTP id gn9so20318494wib.1; Sat, 21 Mar 2015 12:57:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=lXsTr0SFCtHNFW+2PuMweZLQ3V3iERMBcQXAGSdGyj0=; b=VZKOFlj6/rj0fq4MrmfaFTIr0Vxt5rEB57qE/D56S09wo28tlmaaaGOnU2WN/KGCMp NEc7x1xdrp2O/BWGXTF0RnKq0FUJ0fVxe2cXWoIllugXHkllTghH2+GNt8QUr29CNcQC j3adgewkeXPXHxpHtm92j9eb5X0s2pnp0LKv+3E1KtpXCntucHvbdc4MsPn831LtaLvi 7n6VqaMPsD8diqtGHxtHj9jacqjXYu+MsWBIBd2UIyAOTNrJJNHRPNvF4huf8Deb+zk9 rQ1wRstEVZLgg1fgs6xsNdZbGwJUIvBFK1jKNfvS4AHDfHYRn+j9kyuCydpNnzi0rofP g8OA== X-Received: by 10.180.38.1 with SMTP id c1mr6675168wik.84.1426967820086; Sat, 21 Mar 2015 12:57:00 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id fo9sm3718328wib.16.2015.03.21.12.56.58 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sat, 21 Mar 2015 12:56:59 -0700 (PDT) Date: Sat, 21 Mar 2015 20:56:56 +0100 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: [PATCH 1/3] fork: assign refed credentials earlier Message-ID: <20150321195656.GB14650@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , Konstantin Belousov , jenkins-admin@freebsd.org, freebsd-current@freebsd.org, Mateusz Guzik References: <20150320122125.GP2379@kib.kiev.ua> <1426899640-6599-1-git-send-email-mjguzik@gmail.com> <1426899640-6599-2-git-send-email-mjguzik@gmail.com> <20150321015151.GF2379@kib.kiev.ua> <20150321015722.GC27736@dft-labs.eu> <20150321141832.GH2379@kib.kiev.ua> <20150321181931.GA14650@dft-labs.eu> <20150321192904.GQ2379@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150321192904.GQ2379@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-current@freebsd.org, jenkins-admin@freebsd.org, Mateusz Guzik X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 21 Mar 2015 19:57:02 -0000 On Sat, Mar 21, 2015 at 09:29:04PM +0200, Konstantin Belousov wrote: > On Sat, Mar 21, 2015 at 07:19:31PM +0100, Mateusz Guzik wrote: > > On Sat, Mar 21, 2015 at 04:18:32PM +0200, Konstantin Belousov wrote: > > > On Sat, Mar 21, 2015 at 02:57:22AM +0100, Mateusz Guzik wrote: > > > > On Sat, Mar 21, 2015 at 03:51:51AM +0200, Konstantin Belousov wrote: > > > > > On Sat, Mar 21, 2015 at 02:00:38AM +0100, Mateusz Guzik wrote: > > > > > > From: Mateusz Guzik > > > > > > > > > > > > Prior to this change the kernel would take p1's credentials and assign > > > > > > them tempororarily to p2. But p1 could change credentials at that time > > > > > > and in effect give us a use-after-free. > > > > > In which way could it change the credentials ? The assigned credentials > > > > > are taken from td_ucred, which, I thought, are guaranteed to be stable > > > > > for the duration of a syscall. > > > > > > > > > > > > > It takes thread's credential in do_fork. But initial copy is taken > > > > unlocked from struct proc. > > > > > > > > Relevant part of the diff: > > > > > > @@ -870,7 +867,7 @@ fork1(struct thread *td, int flags, int pages, struct proc **procp, > > > > > > * XXX: This is ugly; when we copy resource usage, we need to bump > > > > > > * per-cred resource counters. > > > > > > */ > > > > > > - proc_set_cred(newproc, p1->p_ucred); > > > > > > + proc_set_cred(newproc, crhold(td->td_ucred)); > > > > > > > > > > > > I do not understand your note, nor I see the chunk above in the patches > > > you send. Below is the citation from the patch 1: > > > > > > @@ -410,9 +410,6 @@ do_fork(struct thread *td, int flags, struct proc *p2, > > > +struct thread *td2, > > > bzero(&p2->p_startzero, > > > __rangeof(struct proc, p_startzero, p_endzero)); > > > > > > - crhold(td->td_ucred); > > > - proc_set_cred(p2, td->td_ucred); > > > - > > > > fork1 does: > > > > proc_set_cred(newproc, p1->p_ucred); > > > > p1 is unlocked, so whatever memory p1->p_ucred points to may already be > > freed. > > > > /* > > * Initialize resource accounting for the child process. > > */ > > error = racct_proc_fork(p1, newproc); > > if (error != 0) { > > error = EAGAIN; > > goto fail1; > > } > > > > racct_proc_fork -> racct_add_locked results in accessing such > > now-possibly-freed credentials. > > > > do_fork which properly assigns credentials (from a stable source > > (td_ucred) + grabs a reference) is called later. > > > > The patch in question moves aforementioned assignent earlier to replace > > unsafe one with p1->p_ucred. > > It seems that I understand now. > > If you instead assign td->td_ucred for the new process p_ucred temporary, > would it allow to avoid introducing fail2 label ? I dislike even more > contrived cleanup after the patch. Yes but that seems like a hack awaiting for someone to trip over. For instance I would say it would be desirable to move stuff like freeing credentials into zone destructor handler. A hack like this would leave us with an extra crfree. Doing this work would require some care and making sure we have garbage only where we can afford it and I'm not interested in dealing with this right now. So tl;dr I strongly prefer the patch as it is. -- Mateusz Guzik