On Fri, Feb 08, 2013 at 01:00:55PM -0800, John Johansen wrote: > signed-offby: John Johansen <john.johan...@canonical.com> > --- > security/apparmor/domain.c | 15 ++- > security/apparmor/include/apparmor.h | 6 ++ > security/apparmor/include/policy.h | 44 +++++++- > security/apparmor/policy.c | 195 > ++++++++++++++++++---------------- > security/apparmor/policy_unpack.c | 5 + > 5 files changed, 164 insertions(+), 101 deletions(-)
I'm sorry that I haven't reviewed this entire patch yet -- it's heavier
lifting than I expected. But I'll give you what I've got now, with the
hopes that it may unblock you sooner.
> @@ -641,7 +641,10 @@ int aa_change_hat(const char *hats[], int count, u64
> token, bool permtest)
> if (count) {
> /* attempting to change into a new hat or switch to a sibling */
> struct aa_profile *root;
> - root = PROFILE_IS_HAT(profile) ? profile->parent : profile;
> + if (PROFILE_IS_HAT(profile))
> + root = aa_get_profile_rcu(&profile->parent);
> + else
> + root = aa_get_profile(profile);
>
> /* find first matching hat */
> for (i = 0; i < count && !hat; i++)
'root' is get here but there's a branch where it isn't put:
} else {
target = hat->base.hname;
if (!PROFILE_IS_HAT(hat)) {
info = "target not hat";
error = -EPERM;
goto audit;
}
}
> @@ -42,6 +42,7 @@ extern const char *const profile_mode_names[];
>
> #define PROFILE_IS_HAT(_profile) ((_profile)->flags & PFLAG_HAT)
>
> +#define list_empty_rcu(X) (list_empty(X) || (X)->prev == LIST_POISON2)
This feels brittle. I haven't put the time into fully understanding the
uses of it yet, but one at least feels like an opportunity to just return
worse information to userspace (whether there are zero hats or just an
invalid hat was selected). Besides, in that case, it is racing against
replacement, perhaps just using list_empty() alone would be fine --
it might not be true by the next sync(), but it _might have been true_
at some point in the past. Learning mode is never going to be perfect.
But perhaps the other two uses are more compelling...
> +static inline struct aa_profile *aa_get_profile_rcu(struct aa_profile **p)
> +{
> + struct aa_profile *c;
> +
> + rcu_read_lock();
> + do {
> + c = rcu_dereference(*p);
> + } while (c && !kref_get_not0(&c->base.count));
> + rcu_read_unlock();
> +
> + return c;
> +}
How many iterations of this are possible, if two or more cores each
trying to aa_get_profile_rcu() the same profile... Would they step on
each other's toes in the kref_get_not0() loop, bouncing among them and
never one 'owning' it?
How long could that go on?
Wow, it seems so little after so much hair-pulling..
Thanks John :)
signature.asc
Description: Digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
