[apparmor] [PATCH v2 18/42] parser: Begin to flesh out library interface for features

John Johansen john.johansen at canonical.com
Tue Mar 10 16:55:51 UTC 2015


On 03/06/2015 01:48 PM, Tyler Hicks wrote:
> The aa_features_new_*() functions create an aa_features object. They can
> be thought of as the constructor of aa_features objects. A number of
> constructors are available depending on whether the features are coming
> from a file in the policy cache, a string specified on the command line,
> or from apparmorfs.
> 
> The aa_features_ref() and aa_features_unref() functions are used to grab
> and give up references to an aa_features. When the ref count hits zero,
> all allocated memory is freed. Like with free(), aa_features_unref() can
> be called with a NULL pointer for convenience.
> 
> Pre-processor macros are hidden behind functions so that they don't
> become part of our ABI when we move this code into libapparmor later on.
> 
> A temporary convenience function, aa_features_get_string(), is provided
> while code that uses aa_features is migrated from expecting raw features
> string access to something more abstract. The function will be removed
> in an upcoming patch.
> 
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
Acked-by: John Johansen <john.johansen at canonical.com>

> ---
>  parser/features.c     | 170 ++++++++++++++++++++++++++++++++++++++++----------
>  parser/features.h     |  15 ++---
>  parser/parser_main.c  |  36 ++++++-----
>  parser/policy_cache.c |  23 ++++---
>  parser/policy_cache.h |   4 +-
>  5 files changed, 183 insertions(+), 65 deletions(-)
> 
> diff --git a/parser/features.c b/parser/features.c
> index a2c25c2..ccc238d 100644
> --- a/parser/features.c
> +++ b/parser/features.c
> @@ -33,11 +33,17 @@
>  #include "lib.h"
>  #include "parser.h"
>  
> -#define FEATURES_STRING_SIZE 8192
> -char *features_string = NULL;
> +#define FEATURES_FILE "/sys/kernel/security/" MODULE_NAME "/features"
> +
> +#define STRING_SIZE 8192
> +
> +struct aa_features {
> +	unsigned int ref_count;
> +	char string[STRING_SIZE];
> +};
>  
>  struct features_struct {
> -	char **buffer;
> +	char *buffer;
>  	int size;
>  	char *pos;
>  };
> @@ -45,7 +51,7 @@ struct features_struct {
>  static int features_snprintf(struct features_struct *fst, const char *fmt, ...)
>  {
>  	va_list args;
> -	int i, remaining = fst->size - (fst->pos - *fst->buffer);
> +	int i, remaining = fst->size - (fst->pos - fst->buffer);
>  
>  	if (remaining < 0) {
>  		errno = EINVAL;
> @@ -86,7 +92,7 @@ static int features_dir_cb(DIR *dir, const char *name, struct stat *st,
>  	if (S_ISREG(st->st_mode)) {
>  		autoclose int file = -1;
>  		int len;
> -		int remaining = fst->size - (fst->pos - *fst->buffer);
> +		int remaining = fst->size - (fst->pos - fst->buffer);
>  
>  		file = openat(dirfd(dir), name, O_RDONLY);
>  		if (file == -1) {
> @@ -122,7 +128,7 @@ static int features_dir_cb(DIR *dir, const char *name, struct stat *st,
>  	return 0;
>  }
>  
> -static char *handle_features_dir(const char *filename, char **buffer, int size,
> +static char *handle_features_dir(const char *filename, char *buffer, int size,
>  				 char *pos)
>  {
>  	struct features_struct fst = { buffer, size, pos };
> @@ -135,49 +141,145 @@ static char *handle_features_dir(const char *filename, char **buffer, int size,
>  	return fst.pos;
>  }
>  
> -char *load_features_file(const char *name) {
> -	char *buffer;
> +static int load_features_file(const char *name, char *buffer, size_t size)
> +{
>  	autofclose FILE *f = NULL;
> -	size_t size;
> +	size_t end;
>  
>  	f = fopen(name, "r");
>  	if (!f)
> -		return NULL;
> -
> -	buffer = (char *) malloc(FEATURES_STRING_SIZE);
> -	if (!buffer)
> -		goto fail;
> -
> -	size = fread(buffer, 1, FEATURES_STRING_SIZE - 1, f);
> -	if (!size || ferror(f))
> -		goto fail;
> -	buffer[size] = 0;
> +		return -1;
>  
> -	return buffer;
> +	errno = 0;
> +	end = fread(buffer, 1, size - 1, f);
> +	if (ferror(f)) {
> +		if (!errno)
> +			errno = EIO;
> +		return -1;
> +	}
> +	buffer[end] = 0;
>  
> -fail:
> -	int save = errno;
> -	free(buffer);
> -	errno = save;
> -	return NULL;
> +	return 0;
>  }
>  
> -int load_features(const char *name)
> +/**
> + * aa_features_new - create a new features based on a path
> + * @features: will point to the address of an allocated and initialized
> + *            aa_features object upon success
> + * @path: path to a features file or directory
> + *
> + * Returns: 0 on success, -1 on error with errno set and *@features pointing to
> + *          NULL
> + */
> +int aa_features_new(aa_features **features, const char *path)
>  {
>  	struct stat stat_file;
> +	aa_features *f;
>  
> -	if (stat(name, &stat_file) == -1)
> +	*features = NULL;
> +
> +	if (stat(path, &stat_file) == -1)
> +		return -1;
> +
> +	f = (aa_features *) calloc(1, sizeof(*f));
> +	if (!f) {
> +		errno = ENOMEM;
>  		return -1;
> +	}
> +	aa_features_ref(f);
>  
>  	if (S_ISDIR(stat_file.st_mode)) {
> -		/* if we have a features directory default to */
> -		features_string = (char *) malloc(FEATURES_STRING_SIZE);
> -		handle_features_dir(name, &features_string, FEATURES_STRING_SIZE, features_string);
> -	} else {
> -		features_string = load_features_file(name);
> -		if (!features_string)
> -			return -1;
> +		handle_features_dir(path, f->string, STRING_SIZE, f->string);
> +	} else if (load_features_file(path, f->string, STRING_SIZE)) {
> +		int save = errno;
> +
> +		aa_features_unref(f);
> +		errno = save;
> +		return -1;
> +	}
> +
> +	*features = f;
> +
> +	return 0;
> +}
> +
> +/**
> + * aa_features_new_from_string - create a new features based on a string
> + * @features: will point to the address of an allocated and initialized
> + *            aa_features object upon success
> + * @string: a NUL-terminated string representation of features
> + * @size: the size of @string, not counting the NUL-terminator
> + *
> + * Returns: 0 on success, -1 on error with errno set and *@features pointing to
> + *          NULL
> + */
> +int aa_features_new_from_string(aa_features **features,
> +				const char *string, size_t size)
> +{
> +	aa_features *f;
> +
> +	*features = NULL;
> +
> +	/* Require size to be less than STRING_SIZE so there's room for a NUL */
> +	if (size >= STRING_SIZE)
> +		return ENOBUFS;
> +
> +	f = (aa_features *) calloc(1, sizeof(*f));
> +	if (!f) {
> +		errno = ENOMEM;
> +		return -1;
>  	}
> +	aa_features_ref(f);
> +
> +	memcpy(f->string, string, size);
> +	f->string[size] = '\0';
> +	*features = f;
>  
>  	return 0;
>  }
> +
> +/**
> + * aa_features_new_from_kernel - create a new features based on the current kernel
> + * @features: will point to the address of an allocated and initialized
> + *            aa_features object upon success
> + *
> + * Returns: 0 on success, -1 on error with errno set and *@features pointing to
> + *          NULL
> + */
> +int aa_features_new_from_kernel(aa_features **features)
> +{
> +	return aa_features_new(features, FEATURES_FILE);
> +}
> +
> +/**
> + * aa_features_ref - increments the ref count of a features
> + * @features: the features
> + *
> + * Returns: the features
> + */
> +aa_features *aa_features_ref(aa_features *features)
> +{
> +	atomic_inc(&features->ref_count);
> +	return features;
> +}
> +
> +/**
> + * aa_features_unref - decrements the ref count and frees the features when 0
> + * @features: the features (can be NULL)
> + */
> +void aa_features_unref(aa_features *features)
> +{
> +	if (features && atomic_dec_and_test(&features->ref_count))
> +		free(features);
> +}
> +
> +/**
> + * aa_features_get_string - provides immutable string representation of features
> + * @features: the features
> + *
> + * Returns: an immutable string representation of features
> + */
> +const char *aa_features_get_string(aa_features *features)
> +{
> +	return features->string;
> +}
> diff --git a/parser/features.h b/parser/features.h
> index 201d8fc..100f460 100644
> --- a/parser/features.h
> +++ b/parser/features.h
> @@ -19,13 +19,14 @@
>  #ifndef __AA_FEATURES_H
>  #define __AA_FEATURES_H
>  
> -#include "parser.h"
> +typedef struct aa_features aa_features;
>  
> -#define FEATURES_FILE "/sys/kernel/security/" MODULE_NAME "/features"
> -
> -extern char *features_string;
> -
> -char *load_features_file(const char *name);
> -int load_features(const char *name);
> +int aa_features_new(aa_features **features, const char *path);
> +int aa_features_new_from_string(aa_features **features,
> +				const char *string, size_t size);
> +int aa_features_new_from_kernel(aa_features **features);
> +aa_features *aa_features_ref(aa_features *features);
> +void aa_features_unref(aa_features *features);
> +const char *aa_features_get_string(aa_features *features);
>  
>  #endif /* __AA_FEATURES_H */
> diff --git a/parser/parser_main.c b/parser/parser_main.c
> index 2455103..3f8ca6a 100644
> --- a/parser/parser_main.c
> +++ b/parser/parser_main.c
> @@ -82,6 +82,8 @@ struct timespec mru_tstamp;
>  
>  static char *cacheloc = NULL;
>  
> +static aa_features *features = NULL;
> +
>  /* Make sure to update BOTH the short and long_options */
>  static const char *short_options = "adf:h::rRVvI:b:BCD:NSm:M:qQn:XKTWkL:O:po:";
>  struct option long_options[] = {
> @@ -389,11 +391,17 @@ static int process_arg(int c, char *optarg)
>  		}
>  		break;
>  	case 'm':
> -		features_string = strdup(optarg);
> +		if (aa_features_new_from_string(&features,
> +						optarg, strlen(optarg))) {
> +			fprintf(stderr,
> +				"Failed to parse features string: %m\n");
> +			exit(1);
> +		}
>  		break;
>  	case 'M':
> -		if (load_features(optarg) == -1) {
> -			fprintf(stderr, "Failed to load features from '%s'\n",
> +		if (aa_features_new(&features, optarg)) {
> +			fprintf(stderr,
> +				"Failed to load features from '%s': %m\n",
>  				optarg);
>  			exit(1);
>  		}
> @@ -564,16 +572,17 @@ no_match:
>  	perms_create = 1;
>  }
>  
> -static void set_supported_features(void) {
> +static void set_supported_features(void)
> +{
> +	const char *features_string;
>  
>  	/* has process_args() already assigned a match string? */
> -	if (!features_string) {
> -		if (load_features(FEATURES_FILE) == -1) {
> -			set_features_by_match_file();
> -			return;
> -		}
> +	if (!features && aa_features_new_from_kernel(&features) == -1) {
> +		set_features_by_match_file();
> +		return;
>  	}
>  
> +	features_string = aa_features_get_string(features);
>  	perms_create = 1;
>  
>  	/* TODO: make this real parsing and config setting */
> @@ -865,10 +874,9 @@ static void setup_flags(void)
>  	set_supported_features();
>  
>  	/* Gracefully handle AppArmor kernel without compatibility patch */
> -	if (!features_string) {
> -		PERROR("Cache read/write disabled: %s interface file missing. "
> -			"(Kernel needs AppArmor 2.4 compatibility patch.)\n",
> -			FEATURES_FILE);
> +	if (!features) {
> +		PERROR("Cache read/write disabled: interface file missing. "
> +			"(Kernel needs AppArmor 2.4 compatibility patch.)\n");
>  		write_cache = 0;
>  		skip_read_cache = 1;
>  		return;
> @@ -924,7 +932,7 @@ int main(int argc, char *argv[])
>  		return 0;
>  	}
>  
> -	retval = setup_cache(cacheloc);
> +	retval = setup_cache(features, cacheloc);
>  	if (retval) {
>  		PERROR(_("Failed setting up policy cache (%s): %s\n"),
>  		       cacheloc, strerror(errno));
> diff --git a/parser/policy_cache.c b/parser/policy_cache.c
> index 469d884..8d34f34 100644
> --- a/parser/policy_cache.c
> +++ b/parser/policy_cache.c
> @@ -30,7 +30,6 @@
>  #define _(s) gettext(s)
>  
>  #include "lib.h"
> -#include "features.h"
>  #include "parser.h"
>  #include "policy_cache.h"
>  
> @@ -228,10 +227,11 @@ void install_cache(const char *cachetmpname, const char *cachename)
>  	}
>  }
>  
> -int setup_cache(const char *cacheloc)
> +int setup_cache(aa_features *kernel_features, const char *cacheloc)
>  {
>  	autofree char *cache_features_path = NULL;
> -	autofree char *cache_flags = NULL;
> +	aa_features *cache_features;
> +	const char *kernel_features_string;
>  
>  	if (!cacheloc) {
>  		errno = EINVAL;
> @@ -250,22 +250,27 @@ int setup_cache(const char *cacheloc)
>  		return -1;
>  	}
>  
> -	cache_flags = load_features_file(cache_features_path);
> -	if (cache_flags) {
> -		if (strcmp(features_string, cache_flags) != 0) {
> +	kernel_features_string = aa_features_get_string(kernel_features);
> +	if (!aa_features_new(&cache_features, cache_features_path)) {
> +		const char *cache_features_string;
> +
> +		cache_features_string = aa_features_get_string(cache_features);
> +		if (strcmp(kernel_features_string, cache_features_string) != 0) {
>  			if (write_cache && cond_clear_cache) {
>  				if (create_cache(cacheloc, cache_features_path,
> -						 features_string))
> +						 kernel_features_string))
>  					skip_read_cache = 1;
>  			} else {
>  				if (show_cache)
> -					PERROR("Cache read/write disabled: %s does not match %s\n", FEATURES_FILE, cache_features_path);
> +					PERROR("Cache read/write disabled: Police cache is invalid\n");
>  				write_cache = 0;
>  				skip_read_cache = 1;
>  			}
>  		}
> +		aa_features_unref(cache_features);
>  	} else if (write_cache) {
> -		create_cache(cacheloc, cache_features_path, features_string);
> +		create_cache(cacheloc, cache_features_path,
> +			     kernel_features_string);
>  	}
>  
>  	return 0;
> diff --git a/parser/policy_cache.h b/parser/policy_cache.h
> index 68d45a4..728de57 100644
> --- a/parser/policy_cache.h
> +++ b/parser/policy_cache.h
> @@ -19,6 +19,8 @@
>  #ifndef __AA_POLICY_CACHE_H
>  #define __AA_POLICY_CACHE_H
>  
> +#include "features.h"
> +
>  extern struct timespec mru_tstamp;
>  
>  /* returns true if time is more recent than mru_tstamp */
> @@ -46,6 +48,6 @@ void valid_read_cache(const char *cachename);
>  int cache_hit(const char *cachename);
>  int setup_cache_tmp(const char **cachetmpname, const char *cachename);
>  void install_cache(const char *cachetmpname, const char *cachename);
> -int setup_cache(const char *cacheloc);
> +int setup_cache(aa_features *kernel_features, const char *cacheloc);
>  
>  #endif /* __AA_POLICY_CACHE_H */
> 




More information about the AppArmor mailing list