svn commit: r268087 - head/sys/kern

[Konstantin Belousov]

On Tue, Jul 01, 2014 at 04:32:38PM +0200, Mateusz Guzik wrote:
> On Tue, Jul 01, 2014 at 07:07:28AM -0700, Matthew Fleming wrote:
> > On Tue, Jul 1, 2014 at 2:21 AM, Mateusz Guzik <mjg at freebsd.org> wrote:
> > > Author: mjg
> > > Date: Tue Jul  1 09:21:32 2014
> > > New Revision: 268087
> > > URL: http://svnweb.freebsd.org/changeset/base/268087
> > >
> > > Log:
> > >   Don't call crcopysafe or uifind unnecessarily in execve.
> > 
> > I'm not sure the code works.
> > 
> > It gets a copy of the pointer p_ucred under the PROC_LOCK.  The
> > PROC_LOCK is released before newcred = crdup(oldcred) is called.  Thus
> > you may be copying an old version of the credentials if any of the
> > other functions that modify them run in the meantime.
> > 
> > Maybe this can't happen because the process is single-threaded at the
> > time and all the other sets of p_ucred come via a syscall.  I didn't
> > look at all the functions in the kernel which set p_ucred.  But only
> > in the case that none of them can run during do_execve this code would
> > be safe.  In which case it at least deserves a comment indicating the
> > code is violating the normal locking and safety on p_ucred.
> > 
> 
> All other threads have to be blocked, otherwise there are more dangerous
> races - for instance we support sharing file descriptor tables, so
> execve makes sure to unshare the table (fdunshare()), which is
> especially important for suid binaries. If other threads could execute,
> they could fork off after fdunshare() and then have a table shared with
> now privileged process.
In fact, other threads can execute, just not in this process.
If rfork(2) was used, then the filedesc table can be shared, but
not the address space.

I somehow thought that you already ensured that this does not happen.
> 
> That said, I can add appropriate comment + assertion at top of do_execve
> later, just wanted to note the code was assuming that the process is left
> alone prior to my changes.
> 
> > Also, what is the motivation to avoid the crcopy?  Is there a
> > measurable performance impact?
> > 
> 
> It's just a minor cleanup.
> 
> -- 
> Mateusz Guzik <mjguzik gmail.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20140701/89c38417/attachment.sig>