linux-kernel - Re: [Patch 0/12] AppArmor security module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4AF4B636.9010107@canonical.com>
Date:	Fri, 06 Nov 2009 15:50:14 -0800
From:	John Johansen <john.johansen@...onical.com>
To:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
CC:	john.johansen@...onical.com, linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [Patch 0/12] AppArmor security module

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/