Messages in this thread |  | | Date | Mon, 23 Nov 2009 02:11:16 -0800 | | From | John Johansen <> | | Subject | Re: [AppArmor #3 0/12] AppArmor security module |
| |
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
|  |