Re: [Patch 0/12] AppArmor security module

[John Johansen <john.johansen@canonical.com>]

Tetsuo Handa wrote:
> Hello.
> 
> I browsed using lxr.
> 
> 
> 
>> static int aa_audit_caps(struct aa_profile *profile, struct aa_audit_caps *sa)
> ...snipped...
>> 	ent = &get_cpu_var(audit_cache);
>> 	if (sa->base.task == ent->task && cap_raised(ent->caps, sa->cap)) {
> 
> 		put_cpu_var(audit_cache); ?
> 
yep thanks for the catch

>> 		if (PROFILE_COMPLAIN(profile))
>> 			return 0;
>> 		return sa->base.error;
>> 	} else {
>> 		ent->task = sa->base.task;
>> 		cap_raise(ent->caps, sa->cap);
>> 	}
>> 	put_cpu_var(audit_cache);
> ...snipped...
> 
> 
> 
> Regarding unpack_*(), I'm not sure, but e seems to be no longer used after once
> unpack_*() failed. If so, we can remove
> 
>> 	void *pos = e->pos;
> 
> and
> 
>>  fail:
>>  	e->pos = pos;
> 
actually it is used sometimes for optional elements.  However this could be
cleaned up some because optional elements should only ever fail on the
name or type tags, not the actual data it self.

It is also used in reporting failure position to user space but that only
gets the tag location, it might be better to return the true location of
failure, I'll have a look.

> 
> 
> Also, please add comments regarding
> 
>   memory allocated here is released by ...
> 
>   refcount obtained here is released by ...
> 
>   the caller of this function need to hold ... lock
> 
will do

> as it is difficult for me to track memleak/refcounter/locking bugs.
> For example, in function apparmor_dentry_open(), from
> 
> 	fcxt->profile = aa_get_profile(profile);
> 
> to something like
> 
> 	/* released by ... */
> 	fcxt->profile = aa_get_profile(profile);
> 
> (Oh, is it correct to get refcount even if aa_path_perm() failed?)
> 
yes as long as the refcount is put, there are several possible reasons for 
grabbing a refcount, like passing the object to auditing, or just optimizing the success path.

Of course it could also just be a bug or code that could use some cleaning up
too.

Thanks again Tetsuo

john