lkml.org 
[lkml]   [2010]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v3 06/11] rlimits: do security check under task_lock
    (add selinux maintainers)

    First of all, my apologies for the huge delay. And I still didn't
    read the whole series, sorry.

    On 06/06, Jiri Slaby wrote:
    >
    > +static int check_security_task_setrlimit_unlocked(struct task_struct *tsk,
    > + unsigned int resource, struct rlimit *new_rlim,
    > + struct rlimit *old_rlim)
    > +{
    > + struct rlimit rlim;
    > + int ret;
    > +
    > + memcpy(&rlim, old_rlim, sizeof(rlim));
    > +
    > + task_unlock(tsk->group_leader);
    > + ret = security_task_setrlimit(tsk, resource, new_rlim);
    > + task_lock(tsk->group_leader);
    > +
    > + if (!ret && memcmp(&rlim, old_rlim, sizeof(rlim)))
    > + return -EAGAIN;
    > + return ret;
    > +}
    > +
    > /* make sure you are allowed to change @tsk limits before calling this */
    > int do_prlimit(struct task_struct *tsk, unsigned int resource,
    > struct rlimit *new_rlim, struct rlimit *old_rlim)
    > {
    > struct rlimit *rlim;
    > - int retval = 0;
    > + int retval;
    >
    > if (resource >= RLIM_NLIMITS)
    > return -EINVAL;
    > @@ -1339,13 +1364,19 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource,
    >
    > rlim = tsk->signal->rlim + resource;
    > task_lock(tsk->group_leader);
    > +again:
    > + retval = 0;
    > if (new_rlim) {
    > if ((new_rlim->rlim_max > rlim->rlim_max) &&
    > !capable(CAP_SYS_RESOURCE))
    > retval = -EPERM;
    > - if (!retval)
    > - retval = security_task_setrlimit(tsk, resource,
    > - new_rlim);
    > + if (!retval) {
    > + retval = check_security_task_setrlimit_unlocked(tsk,
    > + resource, new_rlim, rlim);
    > + if (retval == -EAGAIN) {
    > + goto again;
    > + }
    > + }

    Oh. Can't we just ignore this (imho minor) race ? Or just verify/document that
    current_has_perm() can be called under task_lock. Actually, I do not think
    we have a race, selinux_task_setrlimit() only checks that the caller has
    rights to change the rlimits.

    And. Given that avc_has_perm() can be called from irq context (say,
    selinux_file_send_sigiotask or selinux_task_kill), we can assume it is safe
    to call it under task_lock() which is not irq-safe.

    But. OTOH, if we are really worried about security_ ops, then we have another
    reason to call this hook under task_lock(), and we probably want to modify
    selinux_bprm_committing_creds() to take this lock too:

    --- security/selinux/hooks.c
    +++ security/selinux/hooks.c
    @@ -2333,11 +2333,14 @@ static void selinux_bprm_committing_cred
    rc = avc_has_perm(new_tsec->osid, new_tsec->sid, SECCLASS_PROCESS,
    PROCESS__RLIMITINH, NULL);
    if (rc) {
    + /* protects against do_prlimit() */
    + task_lock(current);
    for (i = 0; i < RLIM_NLIMITS; i++) {
    rlim = current->signal->rlim + i;
    initrlim = init_task.signal->rlim + i;
    rlim->rlim_cur = min(rlim->rlim_max, initrlim->rlim_cur);
    }
    + task_unlock(current);
    update_rlimit_cpu(current->signal->rlim[RLIMIT_CPU].rlim_cur);
    }
    }

    Finally. selinux_task_setrlimit(p) uses __task_cred(p) for the check.
    This looks a bit strange, different threads can have different creds
    but obviously rlimits are per-process.

    Perhaps it makes sense to do selinux_task_setrlimit(p->group_leader)?
    At least in this case the result should be "consistent".

    Oleg.



    \
     
     \ /
      Last update: 2010-06-07 20:13    [W:0.026 / U:65.720 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site