Re: [PATCH v5 5/5] IMA: introduce a new policy option func=SETXATTR_CHECK

[Mimi Zohar <zohar@linux.ibm.com>]

Hi Simon,

On Wed, 2021-07-28 at 13:21 +0000, THOBY Simon wrote:
 
> @@ -914,6 +918,42 @@ int ima_check_policy(void)
>  	return 0;
>  }
>  
> +/** update_allowed_hash_algorithms - update the hash algorithms allowed

The first line of kernel-doc is just "/**" by itself, followed by the
function name and a brief description.  The brief description should
not wrap to the next line.  Refer to Documentation/doc-guide/kernel-
doc.rst.

> + * for setxattr writes
> + *
> + * Update the atomic variable holding the set of allowed hash algorithms
> + * that can be used to update the security.ima xattr of a file.
> + *
> + * Context: called when updating the IMA policy.
> + *
> + * SETXATTR_CHECK rules do not implement a full policy check because of
> + * the performance impact performing rules checking on setxattr() would
> + * have. The consequence is that only one SETXATTR_CHECK can be active at
> + * a time.
> + */
> +static void update_allowed_hash_algorithms(void)
> +{
> +	struct ima_rule_entry *entry;
> +
> +	/*
> +	 * We scan in reverse order because only the last entry with the
> +	 * 'func=SETXATTR_CHECK' apply: this allows runtime upgrades of the
> +	 * digest algorithm policy, unlike the other IMA rules that are
> +	 * usually append-only. Old rules will still be present in the
> +	 * ruleset, but inactive.
> +	 */

Oh, my!  I really hope this won't be used as precedent.  Before
agreeing to this, the existing policy rules must require loading of
only signed IMA policies.

thanks,

Mimi
  
> +	rcu_read_lock();
> +	list_for_each_entry_reverse(entry, ima_rules, list) {
> +		if (entry->func != SETXATTR_CHECK)
> +			continue;
> +
> +		atomic_xchg(&ima_setxattr_allowed_hash_algorithms,
> +			    entry->allowed_hashes);
> +		break;
> +	}
> +	rcu_read_unlock();
> +}
> +