Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: "Mickaël Salaün" <mic@digikod.net>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: "Mickaël Salaün" <mickael.salaun@ssi.gouv.fr>,
	LKML <linux-kernel@vger.kernel.org>,
	"Alexander Viro" <viro@zeniv.linux.org.uk>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Andy Lutomirski" <luto@amacapital.net>,
	"Arnaldo Carvalho de Melo" <acme@kernel.org>,
	"Casey Schaufler" <casey@schaufler-ca.com>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"David Drysdale" <drysdale@google.com>,
	"David S . Miller" <davem@davemloft.net>,
	"Eric W . Biederman" <ebiederm@xmission.com>,
	"James Morris" <jmorris@namei.org>, "Jann Horn" <jann@thejh.net>,
	"John Johansen" <john.johansen@canonical.com>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Kees Cook" <keescook@chromium.org>,
	"Michael Kerrisk" <mtk.manpages@gmail.com>,
	"Paul Moore" <paul@paul-moore.com>,
	"Sargun Dhillon" <sargun@sargun.me>,
	"Serge E . Hallyn" <serge@hallyn.com>,
	"Shuah Khan" <shuah@kernel.org>,
	"Stephen Smalley" <sds@tycho.nsa.gov>,
	"Tejun Heo" <tj@kernel.org>,
	"Tetsuo Handa" <penguin-kernel@i-love.sakura.ne.jp>,
	"Thomas Graf" <tgraf@suug.ch>, "Tycho Andersen" <tycho@tycho.ws>,
	"Will Drewry" <wad@chromium.org>,
	"Kernel Hardening" <kernel-hardening@lists.openwall.com>,
	"Linux API" <linux-api@vger.kernel.org>,
	Linux-Fsdevel <linux-fsdevel@vger.kernel.org>,
	"LSM List" <linux-security-module@vger.kernel.org>,
	"Network Development" <netdev@vger.kernel.org>
Subject: Re: [PATCH bpf-next v10 06/10] bpf,landlock: Add a new map type: inode
Date: Tue, 6 Aug 2019 18:24:12 +0200
Message-ID: <9c64ff70-b3bb-d4ad-0d57-e4c941c61503@digikod.net> (raw)
In-Reply-To: <20190801173534.etfls5ltixp5hfrh@ast-mbp.dhcp.thefacebook.com>


On 01/08/2019 19:35, Alexei Starovoitov wrote:
> On Wed, Jul 31, 2019 at 09:11:10PM +0200, Mickaël Salaün wrote:
>>
>>
>> On 31/07/2019 20:58, Alexei Starovoitov wrote:
>>> On Wed, Jul 31, 2019 at 11:46 AM Mickaël Salaün
>>> <mickael.salaun@ssi.gouv.fr> wrote:
>>>>>> +    for (i = 0; i < htab->n_buckets; i++) {
>>>>>> +            head = select_bucket(htab, i);
>>>>>> +            hlist_nulls_for_each_entry_safe(l, n, head, hash_node) {
>>>>>> +                    landlock_inode_remove_map(*((struct inode **)l->key), map);
>>>>>> +            }
>>>>>> +    }
>>>>>> +    htab_map_free(map);
>>>>>> +}
>>>>>
>>>>> user space can delete the map.
>>>>> that will trigger inode_htab_map_free() which will call
>>>>> landlock_inode_remove_map().
>>>>> which will simply itereate the list and delete from the list.
>>>>
>>>> landlock_inode_remove_map() removes the reference to the map (being
>>>> freed) from the inode (with an RCU lock).
>>>
>>> I'm going to ignore everything else for now and focus only on this bit,
>>> since it's fundamental issue to address before this discussion can
>>> go any further.
>>> rcu_lock is not a spin_lock. I'm pretty sure you know this.
>>> But you're arguing that it's somehow protecting from the race
>>> I mentioned above?
>>>
>>
>> I was just clarifying your comment to avoid misunderstanding about what
>> is being removed.
>>
>> As said in the full response, there is currently a race but, if I add a
>> bpf_map_inc() call when the map is referenced by inode->security, then I
>> don't see how a race could occur because such added map could only be
>> freed in a security_inode_free() (as long as it retains a reference to
>> this inode).
> 
> then it will be a cycle and a map will never be deleted?
> closing map_fd should delete a map. It cannot be alive if it's not
> pinned in bpffs, there are no FDs that are holding it, and no progs using it.
> So the map deletion will iterate over inodes that belong to this map.
> In parallel security_inode_free() will be called that will iterate
> over its link list that contains elements from different maps.
> So the same link list is modified by two cpus.
> Where is a lock that protects from concurrent links list manipulations?

Ok, I think I got it. What about this fix?

diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 4fc7755042f0..3226e50b6211 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -1708,10 +1708,16 @@ static void inode_htab_map_free(struct bpf_map *map)

 	for (i = 0; i < htab->n_buckets; i++) {
 		head = select_bucket(htab, i);
-		hlist_nulls_for_each_entry_safe(l, n, head, hash_node) {
+		rcu_read_lock();
+		hlist_nulls_for_each_entry_rcu(l, n, head, hash_node) {
 			landlock_inode_remove_map(*((struct inode **)l->key), map);
 		}
+		rcu_read_unlock();
 	}
+	/*
+	 * The last pending put_landlock_inode_map() may be called here, before
+	 * the rcu_barrier() from htab_map_free().
+	 */
 	htab_map_free(map);
 }

diff --git a/security/landlock/common.h b/security/landlock/common.h
index b0ba3f31ac7d..535c6a4292b9 100644
--- a/security/landlock/common.h
+++ b/security/landlock/common.h
@@ -58,6 +58,11 @@ struct landlock_prog_set {
 	refcount_t usage;
 };

+struct landlock_inode_security {
+	struct list_head list;
+	spinlock_t lock;
+};
+
 struct landlock_inode_map {
 	struct list_head list;
 	struct rcu_head rcu_put;
diff --git a/security/landlock/hooks_fs.c b/security/landlock/hooks_fs.c
index 8c9d6a333111..b9bfd558f8b8 100644
--- a/security/landlock/hooks_fs.c
+++ b/security/landlock/hooks_fs.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h> /* ARRAY_SIZE */
 #include <linux/lsm_hooks.h>
 #include <linux/rcupdate.h> /* synchronize_rcu() */
+#include <linux/spinlock.h>
 #include <linux/stat.h> /* S_ISDIR */
 #include <linux/stddef.h> /* offsetof */
 #include <linux/types.h> /* uintptr_t */
@@ -251,13 +252,16 @@ static int hook_sb_pivotroot(const struct path *old_path,

 /* inode helpers */

-static inline struct list_head *inode_landlock(const struct inode *inode)
+static inline struct landlock_inode_security *inode_landlock(
+		const struct inode *inode)
 {
 	return inode->i_security + landlock_blob_sizes.lbs_inode;
 }

 int landlock_inode_add_map(struct inode *inode, struct bpf_map *map)
 {
+	unsigned long flags;
+	struct landlock_inode_security *inode_sec = inode_landlock(inode);
 	struct landlock_inode_map *inode_map;

 	inode_map = kzalloc(sizeof(*inode_map), GFP_ATOMIC);
@@ -266,60 +270,66 @@ int landlock_inode_add_map(struct inode *inode, struct bpf_map *map)
 	INIT_LIST_HEAD(&inode_map->list);
 	inode_map->map = map;
 	inode_map->inode = inode;
-	list_add_tail(&inode_map->list, inode_landlock(inode));
+	spin_lock_irqsave(&inode_sec->lock, flags);
+	list_add_tail_rcu(&inode_map->list, &inode_sec->list);
+	spin_unlock_irqrestore(&inode_sec->lock, flags);
 	return 0;
 }

 static void put_landlock_inode_map(struct rcu_head *head)
 {
 	struct landlock_inode_map *inode_map;
-	int err;

 	inode_map = container_of(head, struct landlock_inode_map, rcu_put);
-	err = bpf_inode_ptr_unlocked_htab_map_delete_elem(inode_map->map,
+	bpf_inode_ptr_unlocked_htab_map_delete_elem(inode_map->map,
 			&inode_map->inode, false);
-	bpf_map_put(inode_map->map);
 	kfree(inode_map);
 }

 void landlock_inode_remove_map(struct inode *inode, const struct bpf_map *map)
 {
+	unsigned long flags;
+	struct landlock_inode_security *inode_sec = inode_landlock(inode);
 	struct landlock_inode_map *inode_map;
-	bool found = false;

+	spin_lock_irqsave(&inode_sec->lock, flags);
 	rcu_read_lock();
-	list_for_each_entry_rcu(inode_map, inode_landlock(inode), list) {
+	list_for_each_entry_rcu(inode_map, &inode_sec->list, list) {
 		if (inode_map->map == map) {
-			found = true;
 			list_del_rcu(&inode_map->list);
 			kfree_rcu(inode_map, rcu_put);
 			break;
 		}
 	}
 	rcu_read_unlock();
-	WARN_ON(!found);
+	spin_unlock_irqrestore(&inode_sec->lock, flags);
 }

 /* inode hooks */

 static int hook_inode_alloc_security(struct inode *inode)
 {
-	struct list_head *ll_inode = inode_landlock(inode);
+	struct landlock_inode_security *inode_sec = inode_landlock(inode);

-	INIT_LIST_HEAD(ll_inode);
+	INIT_LIST_HEAD(&inode_sec->list);
+	spin_lock_init(&inode_sec->lock);
 	return 0;
 }

 static void hook_inode_free_security(struct inode *inode)
 {
+	unsigned long flags;
+	struct landlock_inode_security *inode_sec = inode_landlock(inode);
 	struct landlock_inode_map *inode_map;

+	spin_lock_irqsave(&inode_sec->lock, flags);
 	rcu_read_lock();
-	list_for_each_entry_rcu(inode_map, inode_landlock(inode), list) {
+	list_for_each_entry_rcu(inode_map, &inode_sec->list, list) {
 		list_del_rcu(&inode_map->list);
 		call_rcu(&inode_map->rcu_put, put_landlock_inode_map);
 	}
 	rcu_read_unlock();
+	spin_unlock_irqrestore(&inode_sec->lock, flags);
 }

 /* a directory inode contains only one dentry */
diff --git a/security/landlock/init.c b/security/landlock/init.c
index 35165fc8a595..1305255f5d2e 100644
--- a/security/landlock/init.c
+++ b/security/landlock/init.c
@@ -137,7 +137,7 @@ static int __init landlock_init(void)
 }

 struct lsm_blob_sizes landlock_blob_sizes __lsm_ro_after_init = {
-	.lbs_inode = sizeof(struct list_head),
+	.lbs_inode = sizeof(struct landlock_inode_security),
 };

 DEFINE_LSM(LANDLOCK_NAME) = {


> 
>> Les données à caractère personnel recueillies et traitées dans le cadre de cet échange, le sont à seule fin d’exécution d’une relation professionnelle et s’opèrent dans cette seule finalité et pour la durée nécessaire à cette relation. Si vous souhaitez faire usage de vos droits de consultation, de rectification et de suppression de vos données, veuillez contacter contact.rgpd@sgdsn.gouv.fr. Si vous avez reçu ce message par erreur, nous vous remercions d’en informer l’expéditeur et de détruire le message. The personal data collected and processed during this exchange aims solely at completing a business relationship and is limited to the necessary duration of that relationship. If you wish to use your rights of consultation, rectification and deletion of your data, please contact: contact.rgpd@sgdsn.gouv.fr. If you have received this message in error, we thank you for informing the sender and destroying the message.
> 
> Please get rid of this. It's absolutely not appropriate on public mailing list.
> Next time I'd have to ignore emails that contain such disclaimers.

Unfortunately this message is automatically appended (server-side) to all my
professional emails...

  reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-21 21:31 [PATCH bpf-next v10 00/10] Landlock LSM: Toward unprivileged sandboxing Mickaël Salaün
2019-07-21 21:31 ` [PATCH bpf-next v10 01/10] fs,security: Add a new file access type: MAY_CHROOT Mickaël Salaün
2019-07-21 21:31 ` [PATCH bpf-next v10 02/10] bpf: Add expected_attach_triggers and a is_valid_triggers() verifier Mickaël Salaün
2019-07-21 21:31 ` [PATCH bpf-next v10 03/10] bpf,landlock: Define an eBPF program type for Landlock hooks Mickaël Salaün
2019-07-21 21:31 ` [PATCH bpf-next v10 04/10] seccomp,landlock: Enforce Landlock programs per process hierarchy Mickaël Salaün
2019-07-21 21:31 ` [PATCH bpf-next v10 05/10] landlock: Handle filesystem access control Mickaël Salaün
2019-07-21 21:31 ` [PATCH bpf-next v10 06/10] bpf,landlock: Add a new map type: inode Mickaël Salaün
2019-07-27  1:40   ` Alexei Starovoitov
2019-07-31 18:46     ` Mickaël Salaün
2019-07-31 18:58       ` Alexei Starovoitov
2019-07-31 19:11         ` Mickaël Salaün
2019-08-01 17:35           ` Alexei Starovoitov
2019-08-06 16:24             ` Mickaël Salaün [this message]
2019-09-08 22:09       ` Mickaël Salaün
2019-09-08 22:19         ` Al Viro
2019-07-21 21:31 ` [PATCH bpf-next v10 07/10] landlock: Add ptrace restrictions Mickaël Salaün
2019-07-21 21:31 ` [PATCH bpf-next v10 08/10] bpf: Add a Landlock sandbox example Mickaël Salaün
2019-07-21 21:31 ` [PATCH bpf-next v10 09/10] bpf,landlock: Add tests for Landlock Mickaël Salaün
2019-07-21 21:31 ` [PATCH bpf-next v10 10/10] landlock: Add user and kernel documentation " Mickaël Salaün
2019-07-31  1:53   ` Randy Dunlap
2019-08-01 17:03     ` Mickaël Salaün
2019-08-01 17:49       ` Randy Dunlap

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9c64ff70-b3bb-d4ad-0d57-e4c941c61503@digikod.net \
    --to=mic@digikod.net \
    --cc=acme@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=alexei.starovoitov@gmail.com \
    --cc=ast@kernel.org \
    --cc=casey@schaufler-ca.com \
    --cc=corbet@lwn.net \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=drysdale@google.com \
    --cc=ebiederm@xmission.com \
    --cc=jann@thejh.net \
    --cc=jmorris@namei.org \
    --cc=john.johansen@canonical.com \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=mickael.salaun@ssi.gouv.fr \
    --cc=mtk.manpages@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --cc=sargun@sargun.me \
    --cc=sds@tycho.nsa.gov \
    --cc=serge@hallyn.com \
    --cc=shuah@kernel.org \
    --cc=tgraf@suug.ch \
    --cc=tj@kernel.org \
    --cc=tycho@tycho.ws \
    --cc=viro@zeniv.linux.org.uk \
    --cc=wad@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org linux-security-module@archiver.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/ public-inbox