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

Tetsuo Handa
2009-11-21T05:28:17Z

Regarding file.c ipc.c lib.c lsm.c

You can use container_of() inside callback functions to avoid "void *".

> 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 ?

> 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.

> 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;

> 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);

?

>
> *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.

> 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

(...snipped...)
> }

> int aa_alloc_default_namespace(void)

This function could be declared with __init attribute.

> 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().

> }