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

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

Tetsuo Handa wrote:
> Regarding file.c ipc.c lib.c lsm.c
> 
> 
> 
> You can use container_of() inside callback functions to avoid "void *".
> 
yeah that is cleaner, will do

>> int aa_audit(int type, struct aa_profile *profile, struct aa_audit *sa,
>> 	     void (*cb) (struct audit_buffer *, void *))
> 
> int aa_audit(int type, struct aa_profile *profile, struct aa_audit *sa,
> 	     void (*cb) (struct audit_buffer *, struct aa_audit *))
> 
>> static int aa_audit_base(int type, struct aa_profile *profile,
>> 			 struct aa_audit *sa, struct audit_context *audit_cxt,
>> 			 void (*cb) (struct audit_buffer *, void *))
> 
> static int aa_audit_base(int type, struct aa_profile *profile,
> 			 struct aa_audit *sa, struct audit_context *audit_cxt,
> 			 void (*cb) (struct audit_buffer *, struct aa_audit *))
> 
>> void file_audit_cb(struct audit_buffer *ab, void *va)
>> {
>> 	struct aa_audit_file *sa = va;
> 
> void file_audit_cb(struct audit_buffer *ab, struct aa_audit *va)
> {
> 	struct aa_audit_file *sa = container_of(va, struct aa_audit_file, base);
> 
>> int aa_audit_file(struct aa_profile *profile, struct aa_audit_file *sa)
>> (...snipped...)
>> 	return aa_audit(type, profile, (struct aa_audit *)sa, file_audit_cb);
> 
> 	return aa_audit(type, profile, &sa->base, file_audit_cb);
> 
>> }
> 
>> static void audit_cb(struct audit_buffer *ab, void *va)
>> {
>> 	struct aa_audit_ptrace *sa = va;
> 
> static void audit_cb(struct audit_buffer *ab, struct aa_audit *va)
> {
> 	struct aa_audit_ptrace *sa = container_of(va, struct aa_audit_ptrace,
> 		base);
> 
>> static int aa_audit_ptrace(struct aa_profile *profile,
>> 			   struct aa_audit_ptrace *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);
> 
> 
> 
>> int aa_path_link(struct aa_profile *profile, struct dentry *old_dentry,
>> 		 struct path *new_dir, struct dentry *new_dentry)
> (.,..snipped...)
>> 	unsigned int state;
> (.,..snipped...)
>> 	sa.perms = aa_str_perms(profile->file.dfa, DFA_START, sa.name, &cond,
>> 				&state);
> 
> "state" remains uninitialized if profile->file.dfa == NULL.
> Are you sure profile->file.dfa != NULL ?
> 
No this needs to be fixed.

> 
> 
>> char *aa_strchrnul(const char *s, int c)
>> {
>> 	for (; *s != (char)c && *s != '\0'; ++s)
>> 		;
>> 	return (char *)s;
>> }
> 
> Only fqname_subname() calls aa_strchrnul() and
> fqname_subname() returns NULL if aa_strchrnul() returns '\0'.
> You can use strchr() instead.
> 
hrmm right thanks

>> static const char *fqname_subname(const char *name)
>> {
>> 	char *split;
>> 	/* check for namespace which begins with a : and ends with : or \0 */
>> 	name = strstrip((char *)name);
>> 	if (*name == ':') {
>> 		split = aa_strchrnul(name + 1, ':');
>> 		if (*split == '\0')
>> 			return NULL;
> 
> 		split = strchr(name + 1, ':');
> 		if (!split)
> 			return NULL;
> 
yep

>> 		name = strstrip(split + 1);
>> 	}
>> 	for (split = strstr(name, "//"); split; split = strstr(name, "//"))
>> 	name = split + 2;
>>
>> 	return name;
>> }
> 
> 
> 
>> char *aa_split_name_from_ns(char *args, char **ns_name)
>> {
>> 	char *name = strstrip(args);
>>
>> 	*ns_name = NULL;
>> 	if (args[0] == ':') {
>> 		char *split = strstrip(strchr(&args[1], ':'));
>>
>> 		if (!split)
>> 			return NULL;
> 
> 
> strchr() returns NULL if not found, and strstrip(NULL) will do strlen(NULL).
> strstrip() never returns NULL. Did you mean
> 
> 		char *split = strchr(&args[1], ':');
> 
> 		if (!split)
> 			return NULL;
> 		split = strstrip(split);
> 
> ?
yes, thanks

> 
>> 		*split = 0;
>> 		*ns_name = &args[1];
>> 		name = strstrip(split + 1);
>> 	}
>> 	if (*name == 0)
>> 		name = NULL;
>>
>> 	return name;
>> }
> 
> 
> 
>> static int apparmor_sysctl(struct ctl_table *table, int op)
> This hook will be removed.
> 
>> char *sysctl_pathname(struct ctl_table *table, char *buffer, int buflen)
> This function will no longer be needed.
> 
>> int aa_pathstr_perm(struct aa_profile *profile, const char *op,
>> 		    const char *name, u16 request, struct path_cond *cond)
> This function will no longer be needed.
> 
yep

> 
> 
>> static int apparmor_file_permission(struct file *file, int mask)
>> {
>> 	/*
>> 	 * TODO: cache profiles that have revalidated?
>> 	 */
>> 	struct aa_file_cxt *fcxt = file->f_security;
>> 	struct aa_profile *profile, *fprofile = fcxt->profile;
>> 	int error = 0;
>>
>> 	if (!fprofile || !file->f_path.mnt ||
>> 	    !mediated_filesystem(file->f_path.dentry->d_inode))
>> 		return 0;
>>
>> 	profile = aa_current_profile();
>>
>> #ifdef CONFIG_SECURITY_APPARMOR_COMPAT_24
>> 	/*
>> 	 * AppArmor <= 2.4 revalidates files at access time instead
>> 	 * of at exec.
>> 	 */
>> 	if (profile && ((fprofile != profile) || (mask & ~fcxt->allowed)))
>> 	    error = aa_file_perm(profile, "file_perm", file, mask);
>> #endif
>>
>> 	return error;
>> }
> 
> Why need to call this function if CONFIG_SECURITY_APPARMOR_COMPAT_24=n ?
> I think we can do
> 
>> static struct security_operations apparmor_ops = {
> (...snipped...)
> 
> #ifdef CONFIG_SECURITY_APPARMOR_COMPAT_24
> 
>> 	.file_permission =              apparmor_file_permission,
> 
> #endif
> 
yes we can currently.  Though this will change in the future, but for now
we should got with the cleaner switch.

> (...snipped...)
>> }
> 
> 
> 
>> int aa_alloc_default_namespace(void)
> 
> This function could be declared with __init attribute.
> 
yep, thanks

> 
> 
>> static int __init apparmor_init(void)
> (...snipped...)
>> 	error = set_init_cxt();
>> 	if (error) {
>> 		AA_ERROR("Failed to set context on init task\n");
>> 		goto alloc_out;
> 
> This should be
> 
> 		goto register_security_out;
> 
> in order to call aa_free_default_namespace().
> 
>> 	}

indeed

thanks again for taking the time to review
john