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    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site