by Tetsuo Handa

[permalink] [raw]

Subject: Re: [AppArmor #4 0/12] AppArmor security module

Regarding path.c

"struct task_struct" has "struct fs_struct *fs" and "struct nsproxy *nsproxy".
The former is used for holding current process's root directory and current
directory.
The latter is used for holding current process's namespace information.

I'm not sure, but I think we can directly fetch current process's namespace's
root by dereferencing current->nsproxy->mnt_ns , and therefore simplify
d_namespace_path() as follows.

36 static int d_namespace_path(struct path *path, char *buf, int buflen,
37 char **name, int flags)
38 {
39 struct path root, tmp, ns_root = { };

/* Remove "ns_root" as only "root" and "tmp" are used. */

40 char *res;
41 int deleted, connected;
42 int error = 0;
43

if (flags & PATH_CHROOT_REL) {

44 read_lock(&current->fs->lock);
45 root = current->fs->root;
46 /* released below */
47 path_get(&root);
48 read_unlock(&current->fs->lock);
49

} else {
root.mnt = current->nsproxy->root;
root.dentry = root.mnt->mnt_root;
/* released below */
path_get(&root);
}

/*

50 spin_lock(&vfsmount_lock);
51 if (root.mnt && root.mnt->mnt_ns)
52 /* released below */
53 ns_root.mnt = mntget(root.mnt->mnt_ns->root);
54 if (ns_root.mnt)
55 /* released below */
56 ns_root.dentry = dget(ns_root.mnt->mnt_root);
57 spin_unlock(&vfsmount_lock);
58

*/

59 spin_lock(&dcache_lock);
60 /* There is a race window between path lookup here and the
61 * need to strip the " (deleted) string that __d_path applies
62 * Detect the race and relookup the path
63 *
64 * The stripping of (deleted) is a hack that could be removed
65 * with an updated __d_path
66 */
67 do {

/*

68 if (flags & PATH_CHROOT_REL)

*/

69 tmp = root;

/*

70 else
71 tmp = ns_root;

*/

72 deleted = d_unlinked(path->dentry);
73 res = __d_path(path, &tmp, buf, buflen);
74
75 } while (deleted != d_unlinked(path->dentry));
76 spin_unlock(&dcache_lock);
77
78 *name = res;
79 /* handle error conditions - and still allow a partial path to
80 * be returned.
81 */
82 if (IS_ERR(res)) {
83 error = PTR_ERR(res);
84 *name = buf;
85 goto out;
86 }
87 if (deleted) {
88 /* On some filesystems, newly allocated dentries appear to the
89 * security_path hooks as a deleted dentry except without an
90 * inode allocated.
91 *
92 * Remove the appended deleted text and return as string for
93 * normal mediation, or auditing. The (deleted) string is
94 * guarenteed to be added in this case, so just strip it.
95 */
96 buf[buflen - 11] = 0; /* - (len(" (deleted)") +\0) */
97
98 if (path->dentry->d_inode && !(flags & PATH_MEDIATE_DELETED)) {
99 error = -ENOENT;
100 goto out;
101 }
102 }
103

/*

104 if (flags & PATH_CHROOT_REL)

*/

105 connected = tmp.dentry == root.dentry && tmp.mnt == root.mnt;

/*

106 else
107 connected = tmp.dentry == ns_root.dentry &&
108 tmp.mnt == ns_root.mnt;

*/

109
110 if (!connected &&
111 !(flags & PATH_CONNECT_PATH) &&
112 !((flags & PATH_CHROOT_REL) && (flags & PATH_CHROOT_NSCONNECT) &&
113 (tmp.dentry == ns_root.dentry && tmp.mnt == ns_root.mnt))) {

/* Change from "ns_root" to "root". */

114 /* disconnected path, don't return pathname starting with '/' */
115 error = -ESTALE;
116 if (*res == '/')
117 *name = res + 1;
118 }
119
120 out:
121 path_put(&root);

/*

122 path_put(&ns_root);

*/

123
124 return error;
125 }

2010-02-26 06:31:33

by Tetsuo Handa

[permalink] [raw]

Subject: Re: [AppArmor #4 0/12] AppArmor security module

Regarding match.c

33 static struct table_header *unpack_table(void *blob, size_t bsize)
(...snipped...)
48 blob += sizeof(struct table_header);
Adding against "void *" is gcc specific.

54 tsize = table_size(th.td_lolen, th.td_flags);
Is tsize != 0 (i.e. integer overflow never happen) guaranteed?

188 struct aa_dfa *aa_dfa_unpack(void *blob, size_t size, int flags)
(...snipped...)
215 while (size > 0) {
216 struct table_header *table;
217 table = unpack_table(blob, size);
218 if (!table)
219 goto fail;
220

You need to do

if (table->td_id < YYTD_ID_TSIZE)
dfa->tables[table->td_id] = table;

now in order to let aa_dfa_free(dfa) to free_table().
You might rather want to do

if (table->td_id < YYTD_ID_TSIZE && !dfa->tables[table->td_id])
dfa->tables[table->td_id] = table;
else
goto fail;

in case duplicated entries are found.

221 switch (table->td_id) {
222 case YYTD_ID_ACCEPT:
223 if (!(table->td_flags & ACCEPT1_FLAGS(flags)))
224 goto fail;
225 break;
226 case YYTD_ID_ACCEPT2:
227 if (!(table->td_flags & ACCEPT2_FLAGS(flags)))
228 goto fail;
229 break;
230 case YYTD_ID_BASE:
231 if (table->td_flags != YYTD_DATA32)
232 goto fail;
233 break;
234 case YYTD_ID_DEF:
235 case YYTD_ID_NXT:
236 case YYTD_ID_CHK:
237 if (table->td_flags != YYTD_DATA16)
238 goto fail;
239 break;
240 case YYTD_ID_EC:
241 if (table->td_flags != YYTD_DATA8)
242 goto fail;
243 break;
244 default:
245 free_table(table);

This "free_table(table);" is not needed as aa_dfa_free(dfa) will do it.

246 goto fail;
247 }
248 dfa->tables[table->td_id] = table;

This "dfa->tables[table->td_id] = table;" is too late.

249 blob += table_size(table->td_lolen, table->td_flags);
250 size -= table_size(table->td_lolen, table->td_flags);
251 }

Regarding policy.c

606 struct aa_profile *aa_new_null_profile(struct aa_profile *parent, int hat)
607 {
608 struct aa_profile *profile = NULL;
609 char *name;
610 u32 sid = aa_alloc_sid(AA_ALLOC_SYS_SID);
611
612 /* freed below */
613 name = kmalloc(strlen(parent->base.hname) + 2 + 7 + 8, GFP_KERNEL);
614 if (!name)
615 goto fail;
616 sprintf(name, "%s//null-%x", parent->base.hname, sid);
617
618 profile = aa_alloc_profile(name);
619 kfree(name);
620 if (!profile)
621 goto fail;
622
623 profile->sid = aa_alloc_sid(AA_ALLOC_SYS_SID);

Why calling aa_alloc_sid(AA_ALLOC_SYS_SID) again rather than using sid?