Re: [PATCH 1/2] Smack: fix seq operations in smackfs

[Casey Schaufler <casey@schaufler-ca.com>]

On 5/21/2015 9:24 AM, Rafal Krypa wrote:
> Use proper RCU functions and read locking in smackfs seq_operations.
>
> Smack gets away with not using proper RCU functions in smackfs, because
> it never removes entries from these lists. But now one list will be
> needed (with interface in smackfs) that will have both elements added and
> removed to it.
> This change will also help any future changes implementing removal of
> unneeded entries from other Smack lists.
>
> The patch also fixes handling of pos argument in smk_seq_start and
> smk_seq_next. This fixes a bug in case when smackfs is read with a small
> buffer:
>
> Kernel panic - not syncing: Kernel mode fault at addr 0xfa0000011b
> CPU: 0 PID: 1292 Comm: dd Not tainted 4.1.0-rc1-00012-g98179b8 #13
> Stack:
>  00000003 0000000d 7ff39e48 7f69fd00
>  7ff39ce0 601ae4b0 7ff39d50 600e587b
>  00000010 6039f690 7f69fd40 00612003
> Call Trace:
>  [<601ae4b0>] load2_seq_show+0x19/0x1d
>  [<600e587b>] seq_read+0x168/0x331
>  [<600c5943>] __vfs_read+0x21/0x101
>  [<601a595e>] ? security_file_permission+0xf8/0x105
>  [<600c5ec6>] ? rw_verify_area+0x86/0xe2
>  [<600c5fc3>] vfs_read+0xa1/0x14c
>  [<600c68e2>] SyS_read+0x57/0xa0
>  [<6001da60>] handle_syscall+0x60/0x80
>  [<6003087d>] userspace+0x442/0x548
>  [<6001aa77>] ? interrupt_end+0x0/0x80
>  [<6001daae>] ? copy_chunk_to_user+0x0/0x2b
>  [<6002cb6b>] ? save_registers+0x1f/0x39
>  [<60032ef7>] ? arch_prctl+0xf5/0x170
>  [<6001a92d>] fork_handler+0x85/0x87
>
> Signed-off-by: Rafal Krypa <r.krypa@samsung.com>

Applied to https://github.com/cschaufler/smack-next.git#smack-for-4.2-stacked

> ---
>  security/smack/smackfs.c | 52 ++++++++++++++++++++----------------------------
>  1 file changed, 22 insertions(+), 30 deletions(-)
>
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 3e42426..e40dc48 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -566,23 +566,17 @@ static void *smk_seq_start(struct seq_file *s, loff_t *pos,
>  				struct list_head *head)
>  {
>  	struct list_head *list;
> +	int i = *pos;
> +
> +	rcu_read_lock();
> +	for (list = rcu_dereference(list_next_rcu(head));
> +		list != head;
> +		list = rcu_dereference(list_next_rcu(list))) {
> +		if (i-- == 0)
> +			return list;
> +	}
>  
> -	/*
> -	 * This is 0 the first time through.
> -	 */
> -	if (s->index == 0)
> -		s->private = head;
> -
> -	if (s->private == NULL)
> -		return NULL;
> -
> -	list = s->private;
> -	if (list_empty(list))
> -		return NULL;
> -
> -	if (s->index == 0)
> -		return list->next;
> -	return list;
> +	return NULL;
>  }
>  
>  static void *smk_seq_next(struct seq_file *s, void *v, loff_t *pos,
> @@ -590,17 +584,15 @@ static void *smk_seq_next(struct seq_file *s, void *v, loff_t *pos,
>  {
>  	struct list_head *list = v;
>  
> -	if (list_is_last(list, head)) {
> -		s->private = NULL;
> -		return NULL;
> -	}
> -	s->private = list->next;
> -	return list->next;
> +	++*pos;
> +	list = rcu_dereference(list_next_rcu(list));
> +
> +	return (list == head) ? NULL : list;
>  }
>  
>  static void smk_seq_stop(struct seq_file *s, void *v)
>  {
> -	/* No-op */
> +	rcu_read_unlock();
>  }
>  
>  static void smk_rule_show(struct seq_file *s, struct smack_rule *srp, int max)
> @@ -660,7 +652,7 @@ static int load_seq_show(struct seq_file *s, void *v)
>  {
>  	struct list_head *list = v;
>  	struct smack_master_list *smlp =
> -		 list_entry(list, struct smack_master_list, list);
> +		list_entry_rcu(list, struct smack_master_list, list);
>  
>  	smk_rule_show(s, smlp->smk_rule, SMK_LABELLEN);
>  
> @@ -808,7 +800,7 @@ static int cipso_seq_show(struct seq_file *s, void *v)
>  {
>  	struct list_head  *list = v;
>  	struct smack_known *skp =
> -		 list_entry(list, struct smack_known, list);
> +		list_entry_rcu(list, struct smack_known, list);
>  	struct netlbl_lsm_catmap *cmp = skp->smk_netlabel.attr.mls.cat;
>  	char sep = '/';
>  	int i;
> @@ -999,7 +991,7 @@ static int cipso2_seq_show(struct seq_file *s, void *v)
>  {
>  	struct list_head  *list = v;
>  	struct smack_known *skp =
> -		 list_entry(list, struct smack_known, list);
> +		list_entry_rcu(list, struct smack_known, list);
>  	struct netlbl_lsm_catmap *cmp = skp->smk_netlabel.attr.mls.cat;
>  	char sep = '/';
>  	int i;
> @@ -1083,7 +1075,7 @@ static int netlbladdr_seq_show(struct seq_file *s, void *v)
>  {
>  	struct list_head *list = v;
>  	struct smk_netlbladdr *skp =
> -			 list_entry(list, struct smk_netlbladdr, list);
> +			list_entry_rcu(list, struct smk_netlbladdr, list);
>  	unsigned char *hp = (char *) &skp->smk_host.sin_addr.s_addr;
>  	int maskn;
>  	u32 temp_mask = be32_to_cpu(skp->smk_mask.s_addr);
> @@ -1917,7 +1909,7 @@ static int load_self_seq_show(struct seq_file *s, void *v)
>  {
>  	struct list_head *list = v;
>  	struct smack_rule *srp =
> -		 list_entry(list, struct smack_rule, list);
> +		list_entry_rcu(list, struct smack_rule, list);
>  
>  	smk_rule_show(s, srp, SMK_LABELLEN);
>  
> @@ -2046,7 +2038,7 @@ static int load2_seq_show(struct seq_file *s, void *v)
>  {
>  	struct list_head *list = v;
>  	struct smack_master_list *smlp =
> -		 list_entry(list, struct smack_master_list, list);
> +		list_entry_rcu(list, struct smack_master_list, list);
>  
>  	smk_rule_show(s, smlp->smk_rule, SMK_LONGLABEL);
>  
> @@ -2123,7 +2115,7 @@ static int load_self2_seq_show(struct seq_file *s, void *v)
>  {
>  	struct list_head *list = v;
>  	struct smack_rule *srp =
> -		 list_entry(list, struct smack_rule, list);
> +		list_entry_rcu(list, struct smack_rule, list);
>  
>  	smk_rule_show(s, srp, SMK_LONGLABEL);
>