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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <026701d107bf$6ef3e280$4cdba780$@alibaba-inc.com>
Date:	Fri, 16 Oct 2015 11:04:54 +0800
From:	"Hillf Danton" <hillf.zj@...baba-inc.com>
To:	"'Lukasz Pawelczyk'" <l.pawelczyk@...sung.com>
Cc:	"'Andy Lutomirski'" <luto@...capital.net>,
	"'Kees Cook'" <keescook@...omium.org>,
	"'linux-kernel'" <linux-kernel@...r.kernel.org>,
	<linux-security-module@...r.kernel.org>
Subject: Re: [PATCH v4 09/11] smack: namespace groundwork

> 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
> 
> 


--
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/