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 <m...@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>
pgpNt_9G4SRSZ.pgp
Description: PGP signature
