lkml.org 
[lkml]   [2008]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/3] do_wait: return security_task_wait() error code in place of -ECHILD
On 03/30, Roland McGrath wrote:
>
> This reverts the effect of commit f2cc3eb133baa2e9dc8efd40f417106b2ee520f3
> "do_wait: fix security checks". That change reverted the effect of commit
> 73243284463a761e04d69d22c7516b2be7de096c. The rationale for the original
> commit still stands. The inconsistent treatment of children hidden by
> ptrace was an unintended omission in the original change and in no way
> invalidates its purpose.

OK, it turns out I misunderstood the purpose of 73243284463a761e04d69d22c7516b2be7de096c,
its changelog says:

wait* syscalls return -ECHILD even when an individual PID of a live child
was requested explicitly, when security_task_wait denies the operation.
This means that something like a broken SELinux policy can produce an
unexpected failure that looks just like a bug with wait or ptrace or
something.

I wrongly thought that "-ECHILD even when an individual PID ... was requested"
was the problem.

> This makes do_wait return the error returned by security_task_wait()
> (usually -EACCES) in place of -ECHILD when there are some children the
> caller would be able to wait for if not for the permission failure. A
> permission error will give the user a clue to look for security policy
> problems, rather than for mysterious wait bugs.

OK, thanks. Again, I was confused and thought we should "hide" -EACCES
unless the child was explicitly requested.

> @@ -1463,9 +1460,22 @@ static int wait_consider_task(struct task_struct *parent,
> int __user *stat_addr, struct rusage __user *ru)
> {
> int ret = eligible_child(type, pid, options, p);
> - if (ret <= 0)
> + if (!ret)
> return ret;
>
> + if (unlikely(ret < 0)) {
> + /*
> + * If we have not yet seen any eligible child,
> + * then let this error code replace -ECHILD.
> + * A permission error will give the user a clue
> + * to look for security policy problems, rather
> + * than for mysterious wait bugs.
> + */
> + if (*retval)
> + *retval = ret;
> + return 0;
> + }

Not that I blame this patch...

Suppose that we have 2 childs. The first one is running, the second is zombie
but nacked by security_task_wait(). Now waitpid(-1, WHOHANG|WEXITED) returns 0,
a bit strange/confusing.
Yes, we have the same behaviour before this patch, but after reading your
explanation I am starting to think this is not "optimal".

Don't get me wrong, I don't claim this should be changed, just I'd like to be
sure this didn't escape your attention.

Oleg.



\
 
 \ /
  Last update: 2008-03-31 14:05    [from the cache]
©2003-2011 Jasper Spaans