[apparmor] [PATCH 3/3] Add mount rules

[John Johansen]

On 02/22/2012 07:46 PM, James Troup wrote:
> John Johansen <john.johansen at canonical.com> writes:
> 
> a) What is it with you and Kees and not trimming replies? :-P
> 
> b) Most of my comments are not substantive and limited to trivial issues
>    as I'm only reading the patch for educational purposes and not really
>    qualified to review the code.  If it's not of interest, I'm happy to
>    do any read further in silence, just let me know.
> 
not at all thanks for the review, I have made changes for most of your
comments, a couple code changes I haven't made at this time but do agree
with you about the changes being cleaner.

>> +++ b/parser/mount.c
> 
> [...]
> 
>> + *   Copyright (c) 2010
>> + *   Canonical, Ltd. (All rights reserved)
> 
> As Kees said, 2012 - but also s/Canonical, Ltd./Canonical Ltd./
> 
> Same for all files. 
> 
>> + * -F fork for similtaneous mount
> 
> s/similtaneous/simultaneous/
> 
>> + * The linux kernel has 32 fs - independent mount flags, that mount command
> 
> s/linux/Linux/
> 
>> + * is responsible for stripping out and mapping to a 32 bit flags field.
>> + * The mount commands mapping is documented below.
>> + *
>> + * Unfortunately we can not directly use this mapping as we need to be able
>> + * represent, whether none, 1 or both options of a flag can be present for
>> + *
> 
> for ... what?
> 
the examples below, that is ro, and rw.  But a single bit match is not enough
we need to store trianary of information in the match rule.  A write up of
how we actually handle and store mount rules will follow later

>> + * eg.
> 
> s/eg./e.g./
> 
>> + *    ro, and rw information is stored in a single bit.  But we need 2 bits
>> + *    of information.
>> + *    ro - the mount can only be readonly
>> + *    rw - the mount can only be rw
>> + *    ro/rw - the mount can be either ro/rw
>> + *    the forth state of neither ro/rw does not exist, but still we need
> 
> s/forth/fourth/ (presumably?)
> 
>> + * Note: leading owner option applies owner condition to both sours and dest
> 
> s/sours/source/
> 
>> + * If a condition is not specified then it is assumed to match all possible
>> + * entries for it.  ie. a missing fstype means all fstypes are matced.
> 
> s/ie./i.e./, s/matced/matched/
> 
>> + * However if a condition is specified then the rule only grants permission
>> + * for mounts matching the specified pattern.
>> + *
>> + * Egs.
> 
> Perhaps: 'Examples:' or just straight up 'e.g.'
> 
yeah

>> + *   to catch mount options, that can't be covered.
> 
> s/, that/that/
> 
>> +			PDEBUG("  extracting fstype ");
> 
> Missing \n ?
> 
>> +	ent = (struct mnt_entry *) calloc(1, sizeof(struct mnt_entry));
>> +	if (ent) {
> 
> Maybe this is simply project style and/or a personal preference, but
> wouldn't it be cleaner to do:
> 
>   ent = (struct mnt_entry *) calloc(1, sizeof(struct mnt_entry));
>     if (!ent)
>       return ent
> 
> Which would allow the rest of the function to be deindented?
> 
agreed but I left it for now, as there is a bigger rework of this
coming

>> +		ent->dev_type = extract_fstype(&sconds);
> 
> It may just be me, but my brain automatically expands that to 'seconds'.
> It might be nice to use src_conds and dst_conds like you did elsewhere
> here too.
> 
>> +		fprintf(stderr, "error: unknonw mount perm");
> 
> s/unknonw/unknown/
> 
>> +++ b/parser/mount.h
> 
> 
>> +#define AA_MAY_REMOUNT 8	/* dump perm for remount rule */
> 
> What does this comment mean?
> 
err it should have said dummy its a fake permission that gets remapped
to a mount flag
  remount X -> Y,

uses it.  It was done instead of adding an extra parameter to the common
mount function but I may revisit that choice

>> +++ b/parser/parser_policy.c
> 
>> +				PERROR("Profile %s has to many specified profile transitions.\n", cod->name);
> 
> s/to/too/
> 
>> +++ b/parser/parser_regex.c
> 
>> +		if (ent->next && size - (p -buffer) > 1) {
> 
> s/-buffer/- buffer/
> 
>> +		/* remount is can't be conditional on device and type */
> 
> s/is //
> 
>> +		pwarn("profile %s mount rules not enforce\n", cod->name);
> 
> not enforced?  not enforceable?

it should be enforced

> 
>> +++ b/parser/parser_variable.c
> 
>> +			if (!dup_and_chain(entry)) {
>>  				PERROR("Memory allocaton error while handling set variable %s\n",
> 
> It's pre-existing, but: s/allocaton/allocation/
> 
>>  static int process_variables_in_entries(struct cod_entry *entry_list)
>>  {
>>  	int ret = TRUE, rc;
>>  	struct cod_entry *entry;
>>  
>>  	list_for_each(entry_list, entry) {
>> -		rc = expand_entry_variables(entry);
>> +		rc = expand_entry_variables(&entry->name, entry,
>> +					    clone_and_chain_cod);
>>  		if (!rc)
>> -			ret = FALSE;
>> +			return FALSE;
>> +	}
>> +
>> +	return ret;
>> +}
>>
> 
> Couldn't you just 'return TRUE;' here and drop the 'ret' variable altogether?
> 

yeah, copied and wasted from another function

>> +static int process_variables_in_mnt_entries(struct mnt_entry *entry_list)
> 
> [...]
> 
>>  	return ret;
> 
> Same here.
> 
>> +++ b/parser/parser_yacc.y
> 
>> +		//		val->next = $1;
> 
oops, it got replaced by the call to append, the line should have been deleted

> ?
>