Re: [AppArmor #3 0/12] AppArmor security module

[Tetsuo Handa]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/