Re: [RFC][PATCH 2/11] security: AppArmor - Core headers

[Arjan van de Ven <arjan@infradead.org>]

On Wed, 2006-04-19 at 10:49 -0700, Tony Jones wrote:
> This patch provides the various common headerfiles used by the AppArmor module.
> 
> apparmor.h contains the core data structures.
> shared.h contains definitions that are common to the userspace policy loader.
> inline.h implements various inline utility functions
> 
> 
> Signed-off-by: Tony Jones <tonyj@suse.de>
> 
> ---
>  security/apparmor/apparmor.h |  325 +++++++++++++++++++++++++++++++++++++++++
>  security/apparmor/inline.h   |  333 +++++++++++++++++++++++++++++++++++++++++++
>  security/apparmor/shared.h   |   41 +++++
>  3 files changed, 699 insertions(+)
> 
> --- /dev/null
> +++ linux-2.6.17-rc1/security/apparmor/apparmor.h
> @@ -0,0 +1,325 @@
> +/*
> + *	Copyright (C) 1998-2005 Novell/SUSE
> + *
> + *	This program is free software; you can redistribute it and/or
> + *	modify it under the terms of the GNU General Public License as
> + *	published by the Free Software Foundation, version 2 of the
> + *	License.
> + *
> + *	AppArmor internal prototypes
> + */
> +
> +#ifndef __SUBDOMAIN_H
> +#define __SUBDOMAIN_H

this is an odd include guard for a file called apparmor.h

> +#include "shared.h"
> +
> +/* Control parameters (0 or 1), settable thru module/boot flags or
> + * via /sys/kernel/security/apparmor/control */
> +extern int apparmor_complain;
> +extern int apparmor_debug;
> +extern int apparmor_audit;
> +extern int apparmor_logsyscall;

looks like these should be in a header too

> +
> +/* PIPEFS_MAGIC */
> +#include <linux/pipe_fs_i.h>
> +/* from net/socket.c */
> +#define SOCKFS_MAGIC 0x534F434B
> +/* from inotify.c  */
> +#define INOTIFYFS_MAGIC 0xBAD1DEA

> +
> +#define VALID_FSTYPE(inode) ((inode)->i_sb->s_magic != PIPEFS_MAGIC && \
> +                             (inode)->i_sb->s_magic != SOCKFS_MAGIC && \
> +                             (inode)->i_sb->s_magic != INOTIFYFS_MAGIC)

ehhhh what is this about? Isn't this highly fragile???

> +
> +/*
> + * DEBUG remains global (no per profile flag) since it is mostly used in sysctl
> + * which is not related to profile accesses.
> + */
> +
> +#define AA_DEBUG(fmt, args...)						\
> +	do {								\
> +		if (apparmor_debug)					\
> +			printk(KERN_DEBUG "AppArmor: " fmt, ##args);	\
> +	} while (0)
> +#define AA_INFO(fmt, args...)	printk(KERN_INFO "AppArmor: " fmt, ##args)
> +#define AA_WARN(fmt, args...)	printk(KERN_WARNING "AppArmor: " fmt, ##args)
> +#define AA_ERROR(fmt, args...)	printk(KERN_ERR "AppArmor: " fmt, ##args)
> +

eh why? at least use prdebug and the like, but don't do your own

> +/* aa_audit - AppArmor auditing structure
> + * Structure is populated by access control code and passed to aa_audit which
> + * provides for a single point of logging.

why duplicate the audit infrastructure??

(and it's not possible to use LSM for auditing really; so it's not just
duplication, it's a bad idea)


> +/** aa_path_getname
> + * @data: data object previously initialized by aa_path_begin
> + *
> + * Return the next mountpoint which has the same root dentry as data->root.
> + * If no more mount points exist (or in case of error) NULL is returned
> + * (caller should call aa_path_end() and inspect return code to differentiate)
> + */
> +static inline char *aa_path_getname(struct aa_path_data *data)
> +{
> +	char *name = NULL;
> +	struct vfsmount *mnt;
> +
> +	while (data->pos != data->head) {
> +		mnt = list_entry(data->pos, struct vfsmount, mnt_list);
> +
> +		/* advance to next -- so that it is done before we break */
> +		data->pos = data->pos->next;
> +		prefetch(data->pos->next);
> +
> +		if (mnt->mnt_root == data->root) {
> +			name = aa_get_name(data->dentry, mnt);
> +			if (!name)
> +				data->errno = -ENOMEM;
> +			break;
> +		}
> +	}
> +
> +	return name;

what's the locking rules and refcounting rules for this stuff ??