lkml.org 
[lkml]   [2010]   [Jul]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 11/13] AppArmor: LSM interface, and security module initialization
    Quoting John Johansen (john.johansen@canonical.com):
    > AppArmor hooks to interface with the LSM, module parameters and module
    > initialization.
    >
    > Signed-off-by: John Johansen <john.johansen@canonical.com>

    Thanks, John - looks good overall. Comments:

    ...

    > +static int apparmor_ptrace_access_check(struct task_struct *child,
    > + unsigned int mode)
    > +{
    > + int rc;
    > +
    > + rc = cap_ptrace_access_check(child, mode);
    > + if (rc)
    > + return rc;
    > +
    > + return aa_ptrace(current, child, mode);
    > +}
    > +
    > +static int apparmor_ptrace_traceme(struct task_struct *parent)
    > +{

    Just curious - why aren't you calling cap_ptrace_traceme() first here?

    > + return aa_ptrace(parent, current, PTRACE_MODE_ATTACH);
    > +}
    > +
    > +/* Derived from security/commoncap.c:cap_capget */
    > +static int apparmor_capget(struct task_struct *target, kernel_cap_t *effective,
    > + kernel_cap_t *inheritable, kernel_cap_t *permitted)
    > +{
    > + struct aa_profile *profile;
    > + const struct cred *cred;
    > +
    > + rcu_read_lock();
    > + cred = __task_cred(target);
    > + profile = aa_cred_profile(cred);
    > +
    > + *effective = cred->cap_effective;
    > + *inheritable = cred->cap_inheritable;
    > + *permitted = cred->cap_permitted;
    > +
    > + if (!unconfined(profile))
    > + *effective = cap_intersect(*effective, profile->caps.allow);

    Should you mask permitted too? Otherwise you might confuse a userspace
    lib which assumes it's caller previously culled pE, and that it can
    nwo refill it from pP.

    > + rcu_read_unlock();
    > +
    > + return 0;
    > +}
    > +
    > +static int apparmor_capable(struct task_struct *task, const struct cred *cred,
    > + int cap, int audit)
    > +{
    > + struct aa_profile *profile;
    > + /* cap_capable returns 0 on success, else -EPERM */
    > + int error = cap_capable(task, cred, cap, audit);

    jinkeys, it might be just me, but i'd have spend 2 mins less looking
    at this if you'd done

    if (error)
    return error;

    here, simplifying the condition below.

    > +
    > + profile = aa_cred_profile(cred);
    > + if (!error && !unconfined(profile))
    > + error = aa_capable(task, profile, cap, audit);
    > +
    > + return error;
    > +}

    -serge


    \
     
     \ /
      Last update: 2010-07-15 19:27    [W:0.025 / U:2.216 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site