Messages in this thread | | | Date | Wed, 1 Apr 2009 12:18:11 +0100 (BST) | From | Hugh Dickins <> | Subject | Re: Q: check_unsafe_exec() races (Was: [PATCH 2/4] fix setuid sometimes doesn't) |
| |
On Wed, 1 Apr 2009, Al Viro wrote: > On Wed, Apr 01, 2009 at 01:28:01AM +0100, Hugh Dickins wrote: > > > Otherwise it looks good to me, except I keep worrying about those > > EAGAINs. The more so once I noticed current->cred_exec_mutex is > > already being used to handle a similar issue with ptrace. What > > do you think of this rather smaller patch? which I'd much rather > > send after having slept on it, since it may be embarrassingly and > > obviously wrong, but tomorrow may be too late ... > > Eh... I'm not particulary happy with fork() growing heavier and heavier.
I don't see it as making fork() any heavier, but never mind. The important thing is to get a fix out.
> Besides, there's a subtle problem avoided by another variant - think what > happens if past the point of no return execve() will unshare fs_struct > (e.g. by explicit unshare() from dynamic linker).
You're too far ahead of me there.
> > Frankly, -EAGAIN in situation when we have userland race is fine. And > we *do* have a userland race here - execve() will kill -9 those threads > in case of success, so if they'd been doing something useful, they are > about to be suddenly screwed.
Good point. I found it quite odd the way the awkward case (shared beyond the threadgroup) is allowed to go forward (with possibility that setuid will be undone), but the easy case is -EAGAINed. (And I gave up on trying to find a better name for your "in_exec" flag, which is rather more subtle than just that!) But odd as it is, there's good reason for doing it that way.
> > So I stand by my variant.
Fair enough.
> Note that if we have *other* tasks sharing > fs_struct, your variant will block their clone() for the duration of > execve() while mine will simply leave them alone (and accept that we > have unsafe sharing).
Yes, intentional, consistent with the existing cred_exec_mutex technique.
Hugh
| |