Re: [PATCH] [ping] nptl: Fix abort in case of set*id failure

[Florian Weimer <fweimer at redhat dot com>]

On 07/09/2014 08:09 AM, Siddhesh Poyarekar wrote:
> On Wed, Jul 02, 2014 at 02:18:06PM +0200, Florian Weimer wrote:
> > If a call to the set*id functions fails in a multi-threaded program,
> > the abort introduced in commit 13f7fe35ae2b0ea55dc4b9628763aafdc8bdc30c
> > was triggered.
>
> There should be a bug report for this I think.  This is suitable for
> 2.20 since it is a regression from a patch that went in earlier to
> abort on set*id failure.

I filed bug 17135 for this.

> > We address by checking that all calls to set*id on all threads give
> > the same result, and only abort if we see success followed by failure
> > (or vice versa).
>
> The fix looks good to me barring a couple of minor issues that I have
> mentioned below.

Thanks for your review.

> The original bug report mentions a kernel failure case being the only
> case causing the setuid failure, but I think there may be another case
> worth mentioning. It goes like this:
>
> 1. A calls setuid(x) and starts signalling other threads
>
> 2. B is signalled with some other signal and its handler has a call to
>     setuid(y) - this is allowed since setuid is considered
>     async-signal-safe.
>
> 3. B signals A due to which A sets its uid to y
>
> 4. A tries calling setuid(x) on itself and fails.
>
> 3. and 4. may happen on any other thread too when they're setuid'd to
> a uid that can't setuid to the other uid.  This can result in
> partially failed setuid calls for A as well as B.

Doesn't the stack_cache_lock in __nptl_setxid prevent this?  I don't 
know what it does to async-signal-safety, and there is likely some issue 
here, but I think your A/B case is properly locked out.

> > @@ -1155,9 +1175,13 @@ __nptl_setxid (struct xid_command *cmdp)
> >   				 cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> >     if (INTERNAL_SYSCALL_ERROR_P (result, err))
> >       {
> > -      __set_errno (INTERNAL_SYSCALL_ERRNO (result, err));
> > +      int error = INTERNAL_SYSCALL_ERRNO (result, err);
> > +      __nptl_setxid_error (cmdp, error);
> > +      __set_errno (error);
> >         result = -1;
> >       }
> > +  else
> > +    __nptl_setxid_error (cmdp, 0);
>
> Get ERROR out and initialize it to 0 so that you can call
> __nptl_setxid_error just once outside the if/else.  Also, you could
> mark the error path as unlikely.  That is:
>
>      int error = 0;
>      if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result, err)))
>        {
>          error = INTERNAL_SYSCALL_ERRNO (result, err);
> 	__set_errno (error);
> 	result = -1;
>        }
>      __nptl_setxid_error (cmdp, error);

Thanks for this suggestion, applied (twice).

--
Florian Weimer / Red Hat Product Security