Re: [PATCH v4 09/11] smack: namespace groundwork

["Hillf Danton" <hillf.zj@alibaba-inc.com>]

> On czw, 2015-10-15 at 14:41 +0200, Lukasz Pawelczyk wrote:
> 
> > No, not a typo. A regular bug. Thanks for spotting it. Also sync
> > mechanism before freeing was missing:
> 
> 
> Hitfix, will be integrated with the next respin:
> 
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 3d432f4..3a795bf 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -97,6 +97,7 @@ struct smack_ns {
>  struct smack_known_ns {
>         struct list_head        smk_list_known;
>         struct list_head        smk_list_ns;
> +       struct rcu_head         smk_rcu;
>         struct user_namespace   *smk_ns;
>         char                    *smk_mapped;
>         struct smack_known      *smk_unmapped;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8e0da67..234da71 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4669,6 +4669,15 @@ static inline int smack_userns_create(struct user_namespace *ns)
>         return 0;
>  }
> 
> +static void smk_free_known_ns(struct rcu_head *head)
> +{
> +       struct smack_known_ns *sknp = container_of(head, struct smack_known_ns, smk_rcu);
> +
> +       if (sknp->smk_allocated)
> +               kfree(sknp->smk_mapped);
> +       kfree(sknp);
> +}
> +
>  static inline void smack_userns_free(struct user_namespace *ns)
>  {
>         struct smack_ns *snsp = ns->security;
> @@ -4680,12 +4689,11 @@ static inline void smack_userns_free(struct user_namespace *ns)
> 
>                 mutex_lock(&skp->smk_mapped_lock);
>                 list_del_rcu(&sknp->smk_list_known);
> -               if (sknp->smk_allocated)
> -                       kfree(sknp->smk_mapped);
> -               kfree(sknp);
>                 mutex_unlock(&skp->smk_mapped_lock);
> 
>                 list_del(&sknp->smk_list_ns);

Is list_del safe, given the operation

+	mutex_lock(&snsp->smk_mapped_lock);
+	list_add_rcu(&sknp->smk_list_ns, &snsp->smk_mapped);
+	mutex_unlock(&snsp->smk_mapped_lock);

in smk_import_mapped() function(copied below)?

> +
> +               call_rcu(&sknp->smk_rcu, smk_free_known_ns);
>         }
> 
>         kfree(snsp);
> --
>

+static struct smack_known_ns *smk_import_mapped(struct smack_known *skp,
+						struct user_namespace *ns,
+						const char *string, int len)
+{
+	struct smack_ns *snsp = ns->security;
+	struct smack_known_ns *sknp;
+	char *mapped;
+	bool allocated;
+
+	/* Mapping init_user_ns is against the design and pointless */
+	if (ns == &init_user_ns)
+		return ERR_PTR(-EBADR);
+
+	mapped = smk_parse_smack(string, len, &allocated);
+	if (IS_ERR(mapped))
+		return ERR_CAST(mapped);
+
+	mutex_lock(&skp->smk_mapped_lock);
+
+	/*
+	 * Don't allow one<->many mappings in namespace, rename.
+	 * This code won't get triggered for now as trying to assign
+	 * a duplicate is forbidden in proc_label_map_write().
+	 * Leaving this as this function might be also used elsewhere.
+	 */
+	sknp = smk_find_mapped(skp, ns);
+	if (sknp != NULL) {
+		if (sknp->smk_allocated)
+			kfree(sknp->smk_mapped);
+		sknp->smk_mapped = mapped;
+		sknp->smk_allocated = allocated;
+		goto unlockout;
+	}
+
+	sknp = kzalloc(sizeof(*sknp), GFP_KERNEL);
+	if (sknp == NULL) {
+		sknp = ERR_PTR(-ENOMEM);
+		if (allocated)
+			kfree(mapped);
+		goto unlockout;
+	}
+
+	sknp->smk_ns = ns;
+	sknp->smk_mapped = mapped;
+	sknp->smk_allocated = allocated;
+	sknp->smk_unmapped = skp;
+	list_add_rcu(&sknp->smk_list_known, &skp->smk_mapped);
+
+	mutex_lock(&snsp->smk_mapped_lock);
+	list_add_rcu(&sknp->smk_list_ns, &snsp->smk_mapped);
+	mutex_unlock(&snsp->smk_mapped_lock);
+
+unlockout:
+	mutex_unlock(&skp->smk_mapped_lock);
+
+	return sknp;
+}
 
> 
> 
> --
> Lukasz Pawelczyk
> Samsung R&D Institute Poland
> Samsung Electronics
> 
>