| 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
| ||
|
Message-Id: <200911222049.BCG40864.HVQMOOFtLFJOSF@I-love.SAKURA.ne.jp>
Date: Sun, 22 Nov 2009 20:49:51 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: john.johansen@...onical.com
Cc: linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [AppArmor #3 0/12] AppArmor security module
And the rest of files...
Regarding match.c
Why not to start YYTD_ID_something from 0 so that we can avoid "- 1" in
dfa->tables[table->td_id - 1] ? I think you can do "- 1" at
th.td_id = be16_to_cpu(*(u16 *) (blob));
.
> int unpack_dfa(struct aa_dfa *dfa, void *blob, size_t size)
> {
(...snipped...)
> fail:
> for (i = 0; i < ARRAY_SIZE(dfa->tables); i++) {
> free_table(dfa->tables[i]);
> dfa->tables[i] = NULL;
> }
This function is called by only aa_unpack_dfa(), and aa_unpack_dfa() calls
aa_match_free(). Thus, you don't need to call free_table() here.
> return error;
> }
Regarding net.c
> static void audit_cb(struct audit_buffer *ab, void *va)
> {
> struct aa_audit_net *sa = va;
static void audit_cb(struct audit_buffer *ab, struct aa_audit *va)
{
struct aa_audit_net *sa = container_of(va, struct aa_audit_net, base);
> static int aa_audit_net(struct aa_profile *profile, struct aa_audit_net *sa)
> {
(...snipped...)
> return aa_audit(type, profile, (struct aa_audit *)sa, audit_cb);
return aa_audit(type, profile, &sa->base, audit_cb);
> }
Regarding policy.c
> struct aa_namespace *alloc_aa_namespace(const char *name)
This function could be "static". Please try "make namespacecheck".
> struct aa_namespace *aa_prepare_namespace(const char *name)
This function could be "static".
> {
> struct aa_namespace *ns;
>
> write_lock(&ns_list_lock);
> if (name)
> /* released by caller */
> ns = aa_get_namespace(__aa_find_namespace(&ns_list, name));
> else
> /* released by caller */
> ns = aa_get_namespace(default_namespace);
alloc_aa_namespace() returns NULL if name == NULL.
If it is intended behavior, you may do like
else {
/* released by caller */
ns = aa_get_namespace(default_namespace);
write_unlock(&ns_list_lock);
return ns;
}
> if (!ns) {
> struct aa_namespace *new_ns;
> write_unlock(&ns_list_lock);
> new_ns = alloc_aa_namespace(name);
> if (!new_ns)
> return NULL;
> write_lock(&ns_list_lock);
> /* test for race when new_ns was allocated */
> ns = __aa_find_namespace(&ns_list, name);
> if (!ns) {
> list_add(&new_ns->base.list, &ns_list);
> /* add list ref */
> ns = aa_get_namespace(new_ns);
> } else {
> /* raced so free the new one */
> free_aa_namespace(new_ns);
> /* get reference on namespace */
> aa_get_namespace(ns);
> }
> }
> write_unlock(&ns_list_lock);
>
> /* return ref */
> return ns;
> }
> void __aa_replace_profile(struct aa_profile *old,
> struct aa_profile *new)
This function could be "static".
> void __aa_profile_list_release(struct list_head *head)
This function could be "static".
> void __aa_remove_namespace(struct aa_namespace *ns)
This function could be "static".
> {
> struct aa_profile *unconfined = ns->unconfined;
> /* remove ns from namespace list */
> list_del_init(&ns->base.list);
>
> /*
> * break the ns, unconfined profile cyclic reference and forward
> * all new unconfined profiles requests to the default namespace
> * This will result in all confined tasks that have a profile
> * being removed inheriting the default->unconfined profile.
> */
> ns->unconfined = aa_get_profile(default_namespace->unconfined);
> __aa_profile_list_release(&ns->base.profiles);
> /* release original ns->unconfined ref */
> aa_put_profile(unconfined);
> /* release ns->base.list ref, from removal above */
> aa_put_namespace(ns);
aa_put_profile() and aa_put_namespace() may call write_lock() inside
free_aa_profile(). Are you sure that these calls do not dead lock?
> }
> ssize_t aa_interface_remove_profiles(char *name, size_t size)
(...snipped...)
> write_lock(&ns_list_lock);
> if (name[0] == ':') {
> char *ns_name;
> name = aa_split_name_from_ns(name, &ns_name);
> /* released below */
> ns = aa_get_namespace(__aa_find_namespace(&ns_list, ns_name));
aa_split_name_from_ns() may set ns_name to NULL but __aa_find_namespace() can't
handle ns_name == NULL case. I think you should check ns_name != NULL.
Regarding policy_unpack.c
Please use bool for functions that return 0 or 1.
> static void audit_cb(struct audit_buffer *ab, void *va)
> {
> struct aa_audit_iface *sa = va;
static void audit_cb(struct audit_buffer *ab, struct aa_audit *va)
{
struct aa_audit_iface *sa = container_of(va, struct aa_audit_iface,
base);
> static int aa_unpack_trans_table(struct aa_ext *e, struct aa_profile *profile)
(...snipped...)
> for (i = 0; i < size; i++) {
> char *tmp;
> if (!unpack_dynstring(e, &tmp, NULL))
> goto fail;
> /*
> * note: strings beginning with a : have an embedded
> * \0 seperating the profile ns name from the profile
> * name
> */
> profile->file.trans.table[i] = tmp;
unpack_dynstring() returns string duplicated by kstrdup(). Thus, "tmp" can't
have an embedded \0 seperating the profile ns name from the profile name
even if tmp[0] == ':' is true, can it?
> }
Regarding resource.c
> static void audit_cb(struct audit_buffer *ab, void *va)
> {
> struct aa_audit_resource *sa = va;
static void audit_cb(struct audit_buffer *ab, struct aa_audit *va)
{
struct aa_audit_resource *sa = container_of(va, struct aa_audit_resource,
base);
> static int aa_audit_resource(struct aa_profile *profile,
> struct aa_audit_resource *sa)
> {
> return aa_audit(AUDIT_APPARMOR_AUTO, profile, (struct aa_audit *)sa,
> audit_cb);
return aa_audit(AUDIT_APPARMOR_AUTO, profile, &sa->base, audit_cb);
> }
Regarding sid.c
> int aa_add_sid_profile(u32 sid, struct aa_profile *profile)
This function is not used.
> int aa_replace_sid_profile(u32 sid, struct aa_profile *profile)
This function is not used.
> struct aa_profile *aa_get_sid_profile(u32 sid)
This function is not used.
--
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/
Powered by blists - more mailing lists