Re: [RFC PATCH v3 03/12] security: add ipe lsm policy parser and policy loading

[Tyler Hicks <tyhicks@linux.microsoft.com>]

On 2020-04-15 09:25:41, deven.desai@linux.microsoft.com wrote:
> From: Deven Bowers <deven.desai@linux.microsoft.com>
> 
> Adds the policy parser and the policy loading to IPE, along with the
> related sysfs, securityfs entries, and audit events.
> 
> Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com>
> ---

...

> diff --git a/security/ipe/ipe-sysfs.c b/security/ipe/ipe-sysfs.c
> index 1c65185c531d..a250da29c3b5 100644
> --- a/security/ipe/ipe-sysfs.c
> +++ b/security/ipe/ipe-sysfs.c
> @@ -5,6 +5,7 @@
>  
>  #include "ipe.h"
>  #include "ipe-audit.h"
> +#include "ipe-secfs.h"
>  
>  #include <linux/sysctl.h>
>  #include <linux/fs.h>
> @@ -45,6 +46,79 @@ static int ipe_switch_mode(struct ctl_table *table, int write,
>  
>  #endif /* CONFIG_SECURITY_IPE_PERMISSIVE_SWITCH */
>  
> +#ifdef CONFIG_SECURITYFS
> +
> +/**
> + * ipe_switch_active_policy: Handler to switch the policy IPE is enforcing.
> + * @table: Sysctl table entry from the variable, sysctl_table.
> + * @write: Integer indicating whether this is a write or a read.
> + * @buffer: Data passed to sysctl. This is the policy id to activate,
> + *	    for this function.
> + * @lenp: Pointer to the size of @buffer.
> + * @ppos: Offset into @buffer.
> + *
> + * This wraps proc_dointvec_minmax, and if there's a change, emits an
> + * audit event.
> + *
> + * Return:
> + * 0 - OK
> + * -ENOMEM - Out of memory
> + * -ENOENT - Policy identified by @id does not exist
> + * Other - See proc_dostring and retrieve_backed_dentry
> + */
> +static int ipe_switch_active_policy(struct ctl_table *table, int write,
> +				    void __user *buffer, size_t *lenp,
> +				    loff_t *ppos)
> +{
> +	int rc = 0;
> +	char *id = NULL;
> +	size_t size = 0;
> +
> +	if (write) {

I see that the policy files in securityfs, such as new_policy, are
checking for CAP_MAC_ADMIN but there's no check here for CAP_MAC_ADMIN
when switching the active policy. I think we should enforce that cap
here, too.

Thinking about it some more, I find it a little odd that we're spreading
the files necessary to update a policy across both procfs (sysctl) and
securityfs. It looks like procfs will have different semantics than
securityfs after looking at proc_sys_permission(). I suggest moving
strict_parse and active_policy under securityfs for a unified experience
and common location when updating policy.

Tyler

> +		id = kzalloc((*lenp) + 1, GFP_KERNEL);
> +		if (!id)
> +			return -ENOMEM;
> +
> +		table->data = id;
> +		table->maxlen = (*lenp) + 1;
> +
> +		rc = proc_dostring(table, write, buffer, lenp, ppos);
> +		if (rc != 0)
> +			goto out;
> +
> +		rc = ipe_set_active_policy(id, strlen(id));
> +	} else {
> +		if (!rcu_access_pointer(ipe_active_policy)) {
> +			table->data = "";
> +			table->maxlen = 1;
> +			return proc_dostring(table, write, buffer, lenp, ppos);
> +		}
> +
> +		rcu_read_lock();
> +		size = strlen(rcu_dereference(ipe_active_policy)->policy_name);
> +		rcu_read_unlock();
> +
> +		id = kzalloc(size + 1, GFP_KERNEL);
> +		if (!id)
> +			return -ENOMEM;
> +
> +		rcu_read_lock();
> +		strncpy(id, rcu_dereference(ipe_active_policy)->policy_name,
> +			size);
> +		rcu_read_unlock();
> +
> +		table->data = id;
> +		table->maxlen = size;
> +
> +		rc = proc_dostring(table, write, buffer, lenp, ppos);
> +	}
> +out:
> +	kfree(id);
> +	return rc;
> +}
> +
> +#endif /* CONFIG_SECURITYFS */
> +
>  static struct ctl_table_header *sysctl_header;
>  
>  static const struct ctl_path sysctl_path[] = {
> @@ -75,6 +149,24 @@ static struct ctl_table sysctl_table[] = {
>  		.extra1 = SYSCTL_ZERO,
>  		.extra2 = SYSCTL_ONE,
>  	},
> +#ifdef CONFIG_SECURITYFS
> +	{
> +		.procname = "strict_parse",
> +		.data = &ipe_strict_parse,
> +		.maxlen = sizeof(int),
> +		.mode = 0644,
> +		.proc_handler = proc_dointvec_minmax,
> +		.extra1 = SYSCTL_ZERO,
> +		.extra2 = SYSCTL_ONE,
> +	},
> +	{
> +		.procname = "active_policy",
> +		.data = NULL,
> +		.maxlen = 0,
> +		.mode = 0644,
> +		.proc_handler = ipe_switch_active_policy,
> +	},
> +#endif /* CONFIG_SECURITYFS */
>  	{}
>  };
>  
> diff --git a/security/ipe/ipe.c b/security/ipe/ipe.c
> index b6553e370f98..07f855ffb79a 100644
> --- a/security/ipe/ipe.c
> +++ b/security/ipe/ipe.c
> @@ -6,6 +6,7 @@
>  #include "ipe.h"
>  #include "ipe-policy.h"
>  #include "ipe-hooks.h"
> +#include "ipe-secfs.h"
>  #include "ipe-sysfs.h"
>  
>  #include <linux/module.h>
> @@ -60,3 +61,4 @@ DEFINE_LSM(ipe) = {
>  
>  int ipe_enforce = 1;
>  int ipe_success_audit;
> +int ipe_strict_parse;
> diff --git a/security/ipe/ipe.h b/security/ipe/ipe.h
> index 6a47f55b05d9..bf6cf7744b0e 100644
> --- a/security/ipe/ipe.h
> +++ b/security/ipe/ipe.h
> @@ -16,5 +16,6 @@
>  
>  extern int ipe_enforce;
>  extern int ipe_success_audit;
> +extern int ipe_strict_parse;
>  
>  #endif /* IPE_H */
> -- 
> 2.26.0