Prev: [PATCH 0/2] blktrace: startup / shutdown race fixes
Next: [PATCH 1/2] blktrace: use cmpxchg
From: Serge E. Hallyn on 15 Jul 2010 13:30
Quoting John Johansen (john.johansen(a)canonical.com):
> AppArmor hooks to interface with the LSM, module parameters and module
> initialization.
>
> Signed-off-by: John Johansen <john.johansen(a)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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [PATCH 0/2] blktrace: startup / shutdown race fixes
Next: [PATCH 1/2] blktrace: use cmpxchg