Date:	Mon, 25 May 2015 18:04:13 -0700
From:	Casey Schaufler <casey@...aufler-ca.com>
To:	Rafal Krypa <r.krypa@...sung.com>
CC:	James Morris <james.l.morris@...cle.com>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] Smack: allow multiple labels in onlycap

On 5/21/2015 9:24 AM, Rafal Krypa wrote:
> Smack onlycap allows limiting of CAP_MAC_ADMIN and CAP_MAC_OVERRIDE to
> processes running with the configured label. But having single privileged
> label is not enough in some real use cases. On a complex system like Tizen,
> there maybe few programs that need to configure Smack policy in run-time
> and running them all with a single label is not always practical.
> This patch extends onlycap feature for multiple labels. They are configured
> in the same smackfs "onlycap" interface, separated by spaces.
>
> Signed-off-by: Rafal Krypa <r.krypa@...sung.com>

I have a comment below. I will test this out while you work
on the (simple) revision.

> ---
>  Documentation/security/Smack.txt |   6 +-
>  security/smack/smack.h           |  25 +++---
>  security/smack/smack_access.c    |  41 ++++++++++
>  security/smack/smackfs.c         | 162 ++++++++++++++++++++++++++-------------
>  4 files changed, 162 insertions(+), 72 deletions(-)
>
> diff --git a/Documentation/security/Smack.txt b/Documentation/security/Smack.txt
> index abc82f8..de5e1ae 100644
> --- a/Documentation/security/Smack.txt
> +++ b/Documentation/security/Smack.txt
> @@ -206,11 +206,11 @@ netlabel
>  	label. The format accepted on write is:
>  		"%d.%d.%d.%d label" or "%d.%d.%d.%d/%d label".
>  onlycap
> -	This contains the label processes must have for CAP_MAC_ADMIN
> +	This contains labels processes must have for CAP_MAC_ADMIN
>  	and CAP_MAC_OVERRIDE to be effective. If this file is empty
>  	these capabilities are effective at for processes with any
> -	label. The value is set by writing the desired label to the
> -	file or cleared by writing "-" to the file.
> +	label. The values are set by writing the desired labels, separated
> +	by spaces, to the file or cleared by writing "-" to the file.
>  ptrace
>  	This is used to define the current ptrace policy
>  	0 - default: this is the policy that relies on Smack access rules.
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index b8c1a86..244e035 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -138,6 +138,11 @@ struct smk_port_label {
>  	struct smack_known	*smk_out;	/* outgoing label */
>  };
>  
> +struct smack_onlycap {
> +	struct list_head	list;
> +	struct smack_known	*smk_label;
> +};
> +
>  /*
>   * Mount options
>   */
> @@ -249,6 +254,7 @@ int smk_netlbl_mls(int, char *, struct netlbl_lsm_secattr *, int);
>  struct smack_known *smk_import_entry(const char *, int);
>  void smk_insert_entry(struct smack_known *skp);
>  struct smack_known *smk_find_entry(const char *);
> +int smack_privileged(int cap);
>  
>  /*
>   * Shared data.
> @@ -257,7 +263,6 @@ extern int smack_enabled;
>  extern int smack_cipso_direct;
>  extern int smack_cipso_mapped;
>  extern struct smack_known *smack_net_ambient;
> -extern struct smack_known *smack_onlycap;
>  extern struct smack_known *smack_syslog_label;
>  #ifdef CONFIG_SECURITY_SMACK_BRINGUP
>  extern struct smack_known *smack_unconfined;
> @@ -276,6 +281,9 @@ extern struct mutex	smack_known_lock;
>  extern struct list_head smack_known_list;
>  extern struct list_head smk_netlbladdr_list;
>  
> +extern struct mutex     smack_onlycap_lock;
> +extern struct list_head smack_onlycap_list;
> +
>  #define SMACK_HASH_SLOTS 16
>  extern struct hlist_head smack_known_hash[SMACK_HASH_SLOTS];
>  
> @@ -332,21 +340,6 @@ static inline struct smack_known *smk_of_current(void)
>  }
>  
>  /*
> - * Is the task privileged and allowed to be privileged
> - * by the onlycap rule.
> - */
> -static inline int smack_privileged(int cap)
> -{
> -	struct smack_known *skp = smk_of_current();
> -
> -	if (!capable(cap))
> -		return 0;
> -	if (smack_onlycap == NULL || smack_onlycap == skp)
> -		return 1;
> -	return 0;
> -}
> -
> -/*
>   * logging functions
>   */
>  #define SMACK_AUDIT_DENIED 0x1
> diff --git a/security/smack/smack_access.c b/security/smack/smack_access.c
> index 408e20b..00f6b38 100644
> --- a/security/smack/smack_access.c
> +++ b/security/smack/smack_access.c
> @@ -617,3 +617,44 @@ struct smack_known *smack_from_secid(const u32 secid)
>  	rcu_read_unlock();
>  	return &smack_known_invalid;
>  }
> +
> +/*
> + * Unless a process is running with one of these labels
> + * even having CAP_MAC_OVERRIDE isn't enough to grant
> + * privilege to violate MAC policy. If no labels are
> + * designated (the empty list case) capabilities apply to
> + * everyone.
> + */
> +LIST_HEAD(smack_onlycap_list);
> +DEFINE_MUTEX(smack_onlycap_lock);
> +
> +/*
> + * Is the task privileged and allowed to be privileged
> + * by the onlycap rule.
> + *
> + * Returns 1 if the task is allowed to be privileged, 0 if it's not.
> + */
> +int smack_privileged(int cap)
> +{
> +	struct smack_known *skp = smk_of_current();
> +	struct smack_onlycap *sop;
> +
> +	if (!capable(cap))
> +		return 0;
> +
> +	rcu_read_lock();
> +	if (list_empty(&smack_onlycap_list)) {
> +		rcu_read_unlock();
> +		return 1;
> +	}
> +
> +	list_for_each_entry_rcu(sop, &smack_onlycap_list, list) {
> +		if (sop->smk_label == skp) {
> +			rcu_read_unlock();
> +			return 1;
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	return 0;
> +}
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index e40dc48..2aabcb2 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -87,16 +87,6 @@ int smack_cipso_direct = SMACK_CIPSO_DIRECT_DEFAULT;
>   */
>  int smack_cipso_mapped = SMACK_CIPSO_MAPPED_DEFAULT;
>  
> -/*
> - * Unless a process is running with this label even
> - * having CAP_MAC_OVERRIDE isn't enough to grant
> - * privilege to violate MAC policy. If no label is
> - * designated (the NULL case) capabilities apply to
> - * everyone. It is expected that the hat (^) label
> - * will be used if any label is used.
> - */
> -struct smack_known *smack_onlycap;
> -
>  #ifdef CONFIG_SECURITY_SMACK_BRINGUP
>  /*
>   * Allow one label to be unconfined. This is for
> @@ -1636,34 +1626,78 @@ static const struct file_operations smk_ambient_ops = {
>  	.llseek		= default_llseek,
>  };
>  
> -/**
> - * smk_read_onlycap - read() for smackfs/onlycap
> - * @filp: file pointer, not actually used
> - * @buf: where to put the result
> - * @cn: maximum to send along
> - * @ppos: where to start
> - *
> - * Returns number of bytes read or error code, as appropriate
> +/*
> + * Seq_file operations for /smack/onlycap
>   */
> -static ssize_t smk_read_onlycap(struct file *filp, char __user *buf,
> -				size_t cn, loff_t *ppos)
> +static void *onlycap_seq_start(struct seq_file *s, loff_t *pos)
>  {
> -	char *smack = "";
> -	ssize_t rc = -EINVAL;
> -	int asize;
> +	return smk_seq_start(s, pos, &smack_onlycap_list);
> +}
>  
> -	if (*ppos != 0)
> -		return 0;
> +static void *onlycap_seq_next(struct seq_file *s, void *v, loff_t *pos)
> +{
> +	return smk_seq_next(s, v, pos, &smack_onlycap_list);
> +}
>  
> -	if (smack_onlycap != NULL)
> -		smack = smack_onlycap->smk_known;
> +static int onlycap_seq_show(struct seq_file *s, void *v)
> +{
> +	struct list_head *list = v;
> +	struct smack_onlycap *sop =
> +		list_entry_rcu(list, struct smack_onlycap, list);
>  
> -	asize = strlen(smack) + 1;
> +	seq_puts(s, sop->smk_label->smk_known);
> +	seq_putc(s, ' ');
>  
> -	if (cn >= asize)
> -		rc = simple_read_from_buffer(buf, cn, ppos, smack, asize);
> +	return 0;
> +}
>  
> -	return rc;
> +static const struct seq_operations onlycap_seq_ops = {
> +	.start = onlycap_seq_start,
> +	.next  = onlycap_seq_next,
> +	.show  = onlycap_seq_show,
> +	.stop  = smk_seq_stop,
> +};
> +
> +static int smk_open_onlycap(struct inode *inode, struct file *file)
> +{
> +	return seq_open(file, &onlycap_seq_ops);
> +}
> +
> +/**
> + * list_swap_rcu - swap public list with a private one in RCU-safe way
> + * The caller must hold appropriate mutex to prevent concurrent modifications
> + * to the public list.
> + * Private list is assumed to be not accessible to other threads yet.
> + *
> + * @public: public list
> + * @private: private list
> + */
> +static void list_swap_rcu(struct list_head *public, struct list_head *private)

Let's not get in the habit of using names that reasonably belong to someone else.
Change this to smk_list_swap_rcu() or some such name. Starting with "list_" is
asking from confusion.

> +{
> +	struct list_head *first, *last;
> +
> +	if (list_empty(public)) {
> +		list_splice_init_rcu(private, public, synchronize_rcu);
> +	} else {
> +		/* Remember public list before replacing it */
> +		first = public->next;
> +		last = public->prev;
> +
> +		/* Publish private list in place of public in RCU-safe way */
> +		private->prev->next = public;
> +		private->next->prev = public;
> +		rcu_assign_pointer(public->next, private->next);
> +		public->prev = private->prev;
> +
> +		synchronize_rcu();
> +
> +		/* When all readers are done with the old public list,
> +		 * attach it in place of private */
> +		private->next = first;
> +		private->prev = last;
> +		first->prev = private;
> +		last->next = private;
> +	}
>  }
>  
>  /**
> @@ -1679,28 +1713,47 @@ static ssize_t smk_write_onlycap(struct file *file, const char __user *buf,
>  				 size_t count, loff_t *ppos)
>  {
>  	char *data;
> -	struct smack_known *skp = smk_of_task(current->cred->security);
> +	char *data_parse
> +	char *tok;
> +	struct smack_known *skp;
> +	struct smack_onlycap *sop;
> +	struct smack_onlycap *sop2;
> +	LIST_HEAD(list_tmp);
>  	int rc = count;
>  
>  	if (!smack_privileged(CAP_MAC_ADMIN))
>  		return -EPERM;
>  
> -	/*
> -	 * This can be done using smk_access() but is done
> -	 * explicitly for clarity. The smk_access() implementation
> -	 * would use smk_access(smack_onlycap, MAY_WRITE)
> -	 */
> -	if (smack_onlycap != NULL && smack_onlycap != skp)
> -		return -EPERM;
> -
>  	data = kzalloc(count + 1, GFP_KERNEL);
>  	if (data == NULL)
>  		return -ENOMEM;
>  
>  	if (copy_from_user(data, buf, count) != 0) {
> -		rc = -EFAULT;
> -		goto freeout;
> +		kfree(data);
> +		return -EFAULT;
> +	}
> +
> +	data_parse = data;
> +	while ((tok = strsep(&data_parse, " ")) != NULL) {
> +		if (!*tok)
> +			continue;
> +
> +		skp = smk_import_entry(tok, 0);
> +		if (IS_ERR(skp)) {
> +			rc = PTR_ERR(skp);
> +			break;
> +		}
> +
> +		sop = kzalloc(sizeof(*sop), GFP_KERNEL);
> +		if (sop == NULL) {
> +			rc = -ENOMEM;
> +			break;
> +		}
> +
> +		sop->smk_label = skp;
> +		list_add_rcu(&sop->list, &list_tmp);
>  	}
> +	kfree(data);
>  
>  	/*
>  	 * Clear the smack_onlycap on invalid label errors. This means
> @@ -1710,26 +1763,29 @@ static ssize_t smk_write_onlycap(struct file *file, const char __user *buf,
>  	 * so "-usecapabilities" will also work.
>  	 *
>  	 * But do so only on invalid label, not on system errors.
> +	 * The invalid label must be first to count as clearing attempt.
>  	 */
> -	skp = smk_import_entry(data, count);
> -	if (PTR_ERR(skp) == -EINVAL)
> -		skp = NULL;
> -	else if (IS_ERR(skp)) {
> -		rc = PTR_ERR(skp);
> -		goto freeout;
> +	if (rc == -EINVAL && list_empty(&list_tmp))
> +		rc = count;
> +
> +	if (rc >= 0) {
> +		mutex_lock(&smack_onlycap_lock);
> +		list_swap_rcu(&smack_onlycap_list, &list_tmp);
> +		mutex_unlock(&smack_onlycap_lock);
>  	}
>  
> -	smack_onlycap = skp;
> +	list_for_each_entry_safe(sop, sop2, &list_tmp, list)
> +		kfree(sop);
>  
> -freeout:
> -	kfree(data);
>  	return rc;
>  }
>  
>  static const struct file_operations smk_onlycap_ops = {
> -	.read		= smk_read_onlycap,
> +	.open		= smk_open_onlycap,
> +	.read		= seq_read,
>  	.write		= smk_write_onlycap,
> -	.llseek		= default_llseek,
> +	.llseek		= seq_lseek,
> +	.release	= seq_release,
>  };
>  
>  #ifdef CONFIG_SECURITY_SMACK_BRINGUP

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists