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    [W:0.023 / U:33.152 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site