Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 30 Jan 2018 16:33:53 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] faccessat: fix error code on setreXid failure

On Tue, Jan 30, 2018 at 11:32:37PM +0300, Alexander Monakov wrote:
> Commit 316d6741b68b485205d7233c98bd6c795bb80370 changed one use of
> SYS_exit in 'checker' without changing another just three lines above,
> and then commit f9fb20b42da0e755d93de229a5a737d79a0e8f60 changed the
> meaning of return value, causing EACCES to be reported instead of EBUSY
> if preparatory setregid/setreuid fail.

This looks right.

> This is the minimal fix for the issue, but it appears there's another:
> collecting checker's exit code and reaping the zombie is implemented as
> 
> 		int status;
> 		do {
> 			__syscall(SYS_wait4, pid, &status, __WCLONE, 0);
> 		} while (!WIFEXITED(status) && !WIFSIGNALED(status));
> 
> but I don't understand why this retry loop is required and correct:

AFAIK wait4 can also return due to Stopped status or trace-related
reasons, not just exit. That was the motivation I think.

> - if another thread won the race to collect the zombie by doing something
>   like waitpid(-1, 0, __WALL), it fails to check syscall's return value and
>   uses uninitialized 'status', possibly causing an infinite loop or OOB access
>   in the parent;

Yes, I think that should be fixed even though I think it's broken
usage.

> - the code seems to assume that the zombie will not be auto-collected even if
>   SIGCHLD disposition is set to SIG_IGN; this sounds logical, but not explicitly
>   documented as far as I can tell;

Indeed, I'm not sure, but I don't know any good fix.

> - if the two problems above don't arise, I don't see how the test in while ()
>   condition can fail; we have signals blocked, so waitpid can only return when
>   the child no longer exists.
> 
> Plus, using CLONE_VM | CLONE_VFORK would help conserve resources.

I'm not sure if it's safe -- having tasks sharing vm with different
permissions is usually a very bad thing. It might be ok here but I'm
not sure.

> 
> Alexander
> 
>  src/unistd/faccessat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/unistd/faccessat.c b/src/unistd/faccessat.c
> index 33478959..954cbdb4 100644
> --- a/src/unistd/faccessat.c
> +++ b/src/unistd/faccessat.c
> @@ -25,7 +25,7 @@ static int checker(void *p)
>  	int i;
>  	if (__syscall(SYS_setregid, __syscall(SYS_getegid), -1)
>  	    || __syscall(SYS_setreuid, __syscall(SYS_geteuid), -1))
> -		__syscall(SYS_exit, 1);
> +		return sizeof errors/sizeof *errors - 1;
>  	ret = __syscall(SYS_faccessat, c->fd, c->filename, c->amode, 0);
>  	for (i=0; i < sizeof errors/sizeof *errors - 1 && ret!=errors[i]; i++);
>  	return i;

Looks ok except it encodes an assumption that EBUSY is last. It might
make more sense to goto the errno-searching loop.

Rich

Powered by blists - more mailing lists

Your e-mail address:

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.