Re: [AppArmor #6 0/13] AppArmor security module

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

On 07/27/2010 11:48 PM, Tetsuo Handa wrote:
> John Johansen wrote:
>> With this submission we believe AppArmor is ready for inclusion into
>> the kernel.
> If nobody has objection, I think it is time to add AppArmor for Linux 2.6.36.
> 
> 
> 
> LXR as of 9788eb59 "AppArmor: Remove delegate information from permission struct"
> is at http://tomoyo.sourceforge.jp/cgi-bin/lxr/source/?v=apparmor-dev .
> 
> Comments listed below. All trivial.
> 
> 
> 
> Regarding apparmorfs.c
> 
> 142 static struct dentry *aa_fs_dentry;
> 
> You can add "__initdata".

done

> 
> static void aafs_remove(const char *name)
> 
> You can add "__init".
> 

done

> 163 static int aafs_create(const char *name, int mask,
> 164                        const struct file_operations *fops)
> 
> You can add "__init".
> 
done

> 179 void aa_destroy_aafs(void)
> 
> You can add "__init".
> 
done

> 198 int aa_create_aafs(void)
> 
> You can add "static" and "__init".
> 
done

> 
> 
> Regarding audit.c
> 
> 179 int aa_audit(int type, struct aa_profile *profile, gfp_t gfp,
> 180              struct common_audit_data *sa,
> 181              void (*cb) (struct audit_buffer *, void *))
> 182 {
> 183         BUG_ON(!profile);
> (...snipped...)
> 200         if (profile && KILL_MODE(profile) && type == AUDIT_APPARMOR_DENIED)
> 201                 type = AUDIT_APPARMOR_KILL;
> 202 
> 203         if (profile && !unconfined(profile))
> 204                 sa->aad.profile = profile;
> 
> profile != NULL already chedked.

done

> 

> 
> 
> Regarding capability.c
> 
>  59  * Returns: 0 or sa->error on succes,  error code on failure
> 
> s/succes/success/

done + spell checked the rest of the comments

> 
> 
> 
> Regarding domain.c
> 
> 201  * Either the profile or namespace name may be optional but if the namespace
> 202  * is specified the profile name termination must be present.  This results
> 203  * in the following possible encodings:
> 204  * profile_name\0
> 205  * :ns_name\0profile_name\0
> 206  * :ns_name\0\0
> 207  *
> 208  * NOTE: the xtable fqname is prevalidated at load time in unpack_trans_table
> 209  *
> 210  * Returns: profile name if it is specified else NULL
> 211  */
> 212 static const char *separate_fqname(const char *fqname, const char **ns_name)
> 213 {
> 214         const char *name;
> 215 
> 216         if (fqname[0] == ':') {
> 217                 *ns_name = fqname + 1;          /* skip : */
> 
> Maybe
> 
> 	BUG_ON(!*ns_name);
> 
> or something is wanted in case fqname by error received ':' + '\0' rather than
> ':' + '\0' + '\0'.
> 
Hrrm, it really shouldn't be necessary.  As mentioned in the comments this is verified
at load time.  I have added an extra comment in the code regarding this, and also
updated the comments in unpack_trans_table to highlight the null terminator checking
for both the single case and for the leading ':' case which requires two.

> 218                 name = *ns_name + strlen(*ns_name) + 1;
> 219                 if (!*name)
> 220                         name = NULL;
> 
> 
> 
> Regarding lib.c
> 
>  61 void aa_info_message(const char *str)
>  62 {
>  63         if (audit_enabled) {
>  64                 struct common_audit_data sa;
>  65                 COMMON_AUDIT_DATA_INIT(&sa, NONE);
>  66                 sa.aad.info = str;
>  67                 printk(KERN_INFO "AppArmor: %s\n", str);
> 
> You want to skip printk() if !audit_enabled?
> 
No, it should be outside the if thanks

>  68                 aa_audit_msg(AUDIT_APPARMOR_STATUS, &sa, NULL);
>  69         }
>  70 }
> 
>  81 void *kvmalloc(size_t size)
>  82 {
>  83         void *buffer = NULL;
>  84 
>  85         if (size == 0)
>  86                 return NULL;
>  87 
>  88         /* do not attempt kmalloc if we need more than 16 pages at once */
>  89         if (size <= (16*PAGE_SIZE))
>  90                 buffer = kmalloc(size, GFP_NOIO | __GFP_NOWARN);
>  91         if (!buffer) {
> 
> Please add "/* See kvfree() for reason to round up. */" or something.
> 
done

>  92                 if (size < sizeof(struct work_struct))
>  93                         size = sizeof(struct work_struct);
>  94                 buffer = vmalloc(size);
> 
> 
> 
> Regarding lsm.c
> 
>  39 int apparmor_initialized;
> 
> You can add "__initdata".
> 
done

> 
> 
> Regarding policy_unpack.c
> 
> 361  * Returns: 1 if table succesfully unpacked
> 
> s/succesfully/successfully/
> 
done

> 
> 
> Regarding include/apparmor.h
> 
>  50 extern int apparmor_initialized;
> 
> You can add "__initdata".
> 
done

> 
> 
> Regarding include/apparmorfs.h
> 
>  18 extern void aa_destroy_aafs(void);
> 
> You can add "__init".
> 
done

> 
> 
> Regarding include/policy.h
> 
>  71 #define AA_NEW_SID 0
> 
> This symbol is not used.
> 
removed

> 254  * @profile: the profile to check for newer versions of (NOT NULL)
> (...snipped...)
> 263 static inline struct aa_profile *aa_newest_version(struct aa_profile *profile)
> 264 {
> 265         if (unlikely(profile && profile->replacedby))
> 266                 for (; profile->replacedby; profile = profile->replacedby)
> 
> Comment says profile != NULL. Maybe
> 
> static inline struct aa_profile *aa_newest_version(struct aa_profile *profile)
> {
> 	while (profile->replacedby)
> 		profile = profile->replacedby;
> }
> 
done