Re: [PATCH 1/3] fork: assign refed credentials earlier

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 <mjg_at_freebsd.org>
> > > > > > 
> > > > > > 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:
> > > > > > _at__at_ -870,7 +867,7 _at__at_ 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:
> > > 
> > > _at__at_ -410,9 +410,6 _at__at_ 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 <mjguzik gmail.com>