[PATCH 0/2 RESEND] vfs: Fix crashes when reading /proc/mounts

[PATCH 0/2 RESEND] vfs: Fix crashes when reading /proc/mounts

[Jan Kara]

Hello,

resending this patch series since I got no reply for almost two months.  This
patch series aims at fixing possible races between functions formatting output
for /proc/mounts and friends and remount. Especially ->show_options functions
of filesystems are not counting with the fact that options can change under
them and thus races can result in bogus output or outright crashes (like was
the case with ext4 handling of quota mount option, or udf could crash when
printing charset name, or xfs when printing log device name etc.).

Admittedly the patch 2/2 is somewhat ugly because namespace_sem ranks below
sb->s_umount semaphore but I think it is bearable. Al, what do you think?

								Honza

[PATCH 1/2] vfs: Provide function to grab superblock reference

[Jan Kara]

Provide function to hold superblock reference when we are sure the
superblock is active. This function will get used by code reading
/proc/mounts and friends.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/internal.h |  1 +
 fs/super.c    | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/fs/internal.h b/fs/internal.h
index d410186bc369..608f4f276ba5 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -104,6 +104,7 @@ extern bool trylock_super(struct super_block *sb);
 extern struct dentry *mount_fs(struct file_system_type *,
 			       int, const char *, void *);
 extern struct super_block *user_get_super(dev_t);
+extern void hold_sb(struct super_block *sb);
 
 /*
  * open.c
diff --git a/fs/super.c b/fs/super.c
index ca53a08497ed..d34b8c49af9d 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -775,6 +775,22 @@ struct super_block *get_super_exclusive_thawed(struct block_device *bdev)
 }
 EXPORT_SYMBOL(get_super_exclusive_thawed);
 
+/**
+ *	hold_sb - get superblock reference for active superblock
+ *	@sb: superblock to get reference for
+ *
+ *	Get reference to superblock. The caller must make sure the superblock
+ *	is currently active by other means (e.g. holding an active reference
+ * 	itself or holding namespace_sem preventing unmount).
+ */
+void hold_sb(struct super_block *sb)
+{
+	spin_lock(&sb_lock);
+	WARN_ON_ONCE(!atomic_read(&sb->s_active));
+	sb->s_count++;
+	spin_unlock(&sb_lock);
+}
+
 /**
  * get_active_super - get an active reference to the superblock of a device
  * @bdev: device to get the superblock for
-- 
2.16.4

[PATCH 2/2] proc: Protect readers of /proc/mounts from remount

[Jan Kara]

Readers of /proc/mounts (and similar files) are currently unprotected
from concurrently running remount on the filesystem they are reporting.
This can not only result in bogus reported information but also in
confusion in filesystems ->show_options callbacks resulting in
use-after-free issues or similar (for example ext4 handling of quota
file names is prone to such races).

Fix the problem by protecting showing of mount information with
sb->s_umount semaphore.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/mount.h     |  1 +
 fs/namespace.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/fs/mount.h b/fs/mount.h
index f39bc9da4d73..5ca0a9e714b3 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -134,6 +134,7 @@ struct proc_mounts {
 	void *cached_mount;
 	u64 cached_event;
 	loff_t cached_index;
+	struct super_block *locked_sb;
 };
 
 extern const struct seq_operations mounts_op;
diff --git a/fs/namespace.c b/fs/namespace.c
index a7f91265ea67..706c802cb75b 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1237,26 +1237,68 @@ struct vfsmount *mnt_clone_internal(const struct path *path)
 }
 
 #ifdef CONFIG_PROC_FS
+
+static bool mounts_trylock_super(struct proc_mounts *p, struct super_block *sb)
+{
+	if (p->locked_sb == sb)
+		return true;
+	if (p->locked_sb) {
+		drop_super(p->locked_sb);
+		p->locked_sb = NULL;
+	}
+	if (down_read_trylock(&sb->s_umount)) {
+		hold_sb(sb);
+		p->locked_sb = sb;
+		return true;
+	}
+	return false;
+}
+
 /* iterator; we want it to have access to namespace_sem, thus here... */
 static void *m_start(struct seq_file *m, loff_t *pos)
 {
 	struct proc_mounts *p = m->private;
+	struct mount *mount;
+	struct super_block *sb;
 
+restart:
 	down_read(&namespace_sem);
 	if (p->cached_event == p->ns->event) {
 		void *v = p->cached_mount;
 		if (*pos == p->cached_index)
-			return v;
+			goto lock_sb;
 		if (*pos == p->cached_index + 1) {
 			v = seq_list_next(v, &p->ns->list, &p->cached_index);
-			return p->cached_mount = v;
+			p->cached_mount = v;
+			goto lock_sb;
 		}
 	}
 
 	p->cached_event = p->ns->event;
 	p->cached_mount = seq_list_start(&p->ns->list, *pos);
 	p->cached_index = *pos;
-	return p->cached_mount;
+lock_sb:
+	if (!p->cached_mount)
+		return NULL;
+	mount = list_entry(p->cached_mount, struct mount, mnt_list);
+	sb = mount->mnt.mnt_sb;
+	if (mounts_trylock_super(p, sb))
+		return p->cached_mount;
+	/*
+	 * Trylock failed. Since namepace_sem ranks below s_umount (through
+	 * sb->s_umount > dir->i_rwsem > namespace_sem in the mount path), we
+	 * have to drop it, wait for s_umount and then try again to guarantee
+	 * forward progress.
+	 */
+	hold_sb(sb);
+	up_read(&namespace_sem);
+	down_read(&sb->s_umount);
+	/*
+	 * Sb may be dead by now but that just means we won't find it in any
+	 * mount and drop lock & reference anyway.
+	 */
+	p->locked_sb = sb;
+	goto restart;
 }
 
 static void *m_next(struct seq_file *m, void *v, loff_t *pos)
@@ -1277,7 +1319,16 @@ static int m_show(struct seq_file *m, void *v)
 {
 	struct proc_mounts *p = m->private;
 	struct mount *r = list_entry(v, struct mount, mnt_list);
-	return p->show(m, &r->mnt);
+	struct super_block *sb = r->mnt.mnt_sb;
+	int ret;
+
+	/* Protect show function against racing remount */
+	if (!mounts_trylock_super(p, sb))
+		return -EAGAIN;
+	ret = p->show(m, &r->mnt);
+	drop_super(sb);
+	p->locked_sb = NULL;
+	return ret;
 }
 
 const struct seq_operations mounts_op = {
-- 
2.16.4

Re: [PATCH 2/2] proc: Protect readers of /proc/mounts from remount

[Al Viro]

On Tue, Dec 11, 2018 at 06:24:23PM +0100, Jan Kara wrote:
> Readers of /proc/mounts (and similar files) are currently unprotected
> from concurrently running remount on the filesystem they are reporting.
> This can not only result in bogus reported information but also in
> confusion in filesystems ->show_options callbacks resulting in
> use-after-free issues or similar (for example ext4 handling of quota
> file names is prone to such races).
> 
> Fix the problem by protecting showing of mount information with
> sb->s_umount semaphore.

Umm...  Which tree is it against and what exactly does your hold_sb() do?

Re: [PATCH 2/2] proc: Protect readers of /proc/mounts from remount

[Al Viro]

On Tue, Dec 11, 2018 at 06:36:19PM +0000, Al Viro wrote:
> On Tue, Dec 11, 2018 at 06:24:23PM +0100, Jan Kara wrote:
> > Readers of /proc/mounts (and similar files) are currently unprotected
> > from concurrently running remount on the filesystem they are reporting.
> > This can not only result in bogus reported information but also in
> > confusion in filesystems ->show_options callbacks resulting in
> > use-after-free issues or similar (for example ext4 handling of quota
> > file names is prone to such races).
> > 
> > Fix the problem by protecting showing of mount information with
> > sb->s_umount semaphore.
> 
> Umm...  Which tree is it against and what exactly does your hold_sb() do?

D'oh... I need more coffee.  Nevermind.

Re: [PATCH 2/2] proc: Protect readers of /proc/mounts from remount

[Al Viro]

On Tue, Dec 11, 2018 at 06:24:23PM +0100, Jan Kara wrote:
> Readers of /proc/mounts (and similar files) are currently unprotected
> from concurrently running remount on the filesystem they are reporting.
> This can not only result in bogus reported information but also in
> confusion in filesystems ->show_options callbacks resulting in
> use-after-free issues or similar (for example ext4 handling of quota
> file names is prone to such races).
> 
> Fix the problem by protecting showing of mount information with
> sb->s_umount semaphore.

> +static bool mounts_trylock_super(struct proc_mounts *p, struct super_block *sb)
> +{
> +	if (p->locked_sb == sb)
> +		return true;
> +	if (p->locked_sb) {
> +		drop_super(p->locked_sb);
> +		p->locked_sb = NULL;
> +	}
> +	if (down_read_trylock(&sb->s_umount)) {
> +		hold_sb(sb);
> +		p->locked_sb = sb;
> +		return true;
> +	}
> +	return false;
> +}

Bad calling conventions, and you are paying for those with making
hold_sb() non-static (and having it, in the first place).

> +	if (mounts_trylock_super(p, sb))
> +		return p->cached_mount;
> +	/*
> +	 * Trylock failed. Since namepace_sem ranks below s_umount (through
> +	 * sb->s_umount > dir->i_rwsem > namespace_sem in the mount path), we
> +	 * have to drop it, wait for s_umount and then try again to guarantee
> +	 * forward progress.
> +	 */
> +	hold_sb(sb);

That.  Just hoist that hold_sb() into your trylock (and put it before the
down_read_trylock() there, while we are at it).  And turn the other caller
into
	if (unlikely(!.....))
		ret = -EAGAIN;
	else
		ret = p->show(m, &r->mnt);
followed by unconditional drop_super().  And I would probably go for
	mount_trylock_super(&p->locked_super, sb)
while we are at it, so that it's isolated from proc_mounts and can
be moved to fs/super.c


> +	up_read(&namespace_sem);
> +	down_read(&sb->s_umount);
> +	/*
> +	 * Sb may be dead by now but that just means we won't find it in any
> +	 * mount and drop lock & reference anyway.
> +	 */
> +	p->locked_sb = sb;
> +	goto restart;

No.
	if (likely(sb->s_root))
		p->locked_sb = sb;
	else
		drop_super(sb);
	goto restart;

Re: [PATCH 2/2] proc: Protect readers of /proc/mounts from remount

[Al Viro]

On Tue, Dec 11, 2018 at 06:58:31PM +0000, Al Viro wrote:

> > +static bool mounts_trylock_super(struct proc_mounts *p, struct super_block *sb)
> > +{
> > +	if (p->locked_sb == sb)
> > +		return true;
> > +	if (p->locked_sb) {
> > +		drop_super(p->locked_sb);
> > +		p->locked_sb = NULL;
> > +	}
> > +	if (down_read_trylock(&sb->s_umount)) {
> > +		hold_sb(sb);
> > +		p->locked_sb = sb;
> > +		return true;
> > +	}
> > +	return false;
> > +}
> 
> Bad calling conventions, and you are paying for those with making
> hold_sb() non-static (and having it, in the first place).
> 
> > +	if (mounts_trylock_super(p, sb))
> > +		return p->cached_mount;
> > +	/*
> > +	 * Trylock failed. Since namepace_sem ranks below s_umount (through
> > +	 * sb->s_umount > dir->i_rwsem > namespace_sem in the mount path), we
> > +	 * have to drop it, wait for s_umount and then try again to guarantee
> > +	 * forward progress.
> > +	 */
> > +	hold_sb(sb);
> 
> That.  Just hoist that hold_sb() into your trylock (and put it before the
> down_read_trylock() there, while we are at it).  And turn the other caller
> into
> 	if (unlikely(!.....))
> 		ret = -EAGAIN;
> 	else
> 		ret = p->show(m, &r->mnt);
> followed by unconditional drop_super().  And I would probably go for
> 	mount_trylock_super(&p->locked_super, sb)
> while we are at it, so that it's isolated from proc_mounts and can
> be moved to fs/super.c

Looking at it some more...  I still hate it ;-/  Take a look at traverse()
in fs/seq_file.c and think what kind of clusterfuck will it cause...

Re: [PATCH 2/2] proc: Protect readers of /proc/mounts from remount

[Jan Kara]

On Tue 11-12-18 19:14:52, Al Viro wrote:
> On Tue, Dec 11, 2018 at 06:58:31PM +0000, Al Viro wrote:
> 
> > > +static bool mounts_trylock_super(struct proc_mounts *p, struct super_block *sb)
> > > +{
> > > +	if (p->locked_sb == sb)
> > > +		return true;
> > > +	if (p->locked_sb) {
> > > +		drop_super(p->locked_sb);
> > > +		p->locked_sb = NULL;
> > > +	}
> > > +	if (down_read_trylock(&sb->s_umount)) {
> > > +		hold_sb(sb);
> > > +		p->locked_sb = sb;
> > > +		return true;
> > > +	}
> > > +	return false;
> > > +}
> > 
> > Bad calling conventions, and you are paying for those with making
> > hold_sb() non-static (and having it, in the first place).
> > 
> > > +	if (mounts_trylock_super(p, sb))
> > > +		return p->cached_mount;
> > > +	/*
> > > +	 * Trylock failed. Since namepace_sem ranks below s_umount (through
> > > +	 * sb->s_umount > dir->i_rwsem > namespace_sem in the mount path), we
> > > +	 * have to drop it, wait for s_umount and then try again to guarantee
> > > +	 * forward progress.
> > > +	 */
> > > +	hold_sb(sb);
> > 
> > That.  Just hoist that hold_sb() into your trylock (and put it before the
> > down_read_trylock() there, while we are at it).  And turn the other caller
> > into
> > 	if (unlikely(!.....))
> > 		ret = -EAGAIN;
> > 	else
> > 		ret = p->show(m, &r->mnt);
> > followed by unconditional drop_super().  And I would probably go for
> > 	mount_trylock_super(&p->locked_super, sb)
> > while we are at it, so that it's isolated from proc_mounts and can
> > be moved to fs/super.c
> 
> Looking at it some more...  I still hate it ;-/  Take a look at traverse()
> in fs/seq_file.c and think what kind of clusterfuck will it cause...

I guess you mean that in case we fail to lock s_umount semaphore, we'll
return -EAGAIN and traverse() will abort? That is true but since we return
-EAGAIN, callers will call into traverse() again - both do:

while ((err = traverse(m, *ppos)) == -EAGAIN) ;

and then in m_start() we will do the blocking lock on s_umount. I agree it
is ugly and twisted but it should be rare...

Now looking at the code, maybe we could avoid this weird retry dance with
traverse(). Something like following in m_show():

        sb = mnt->mnt_sb;
	if (mount_trylock_super())
		show and done
	get passive sb reference
	namespace_unlock();
	down_read(&sb->s_umount);
	namespace_lock();
	new_mnt = seq_list_start();
	if (new_mnt != mnt)
		retry
	show and done

This could be handled completely inside m_show() so no strange retry dance
with traverse(). Do you find this better?

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 0/2 v2] vfs: Fix crashes when reading /proc/mounts

[Jan Kara]

Hello,

this is a second revision of the patch series which aims at fixing possible
races between functions formatting output for /proc/mounts and friends and
remount. Especially ->show_options functions of filesystems are not counting
with the fact that options can change under them and thus races can result in
bogus output or outright crashes (like was the case with ext4 handling of quota
mount option, or udf could crash when printing charset name, or xfs when
printing log device name etc.).

Since v1, I have changed the logic so that all the locking & restart magic
happens in m_show() so that we don't bail back to traverse(), then m_start(),
and then back to m_show(). Al, is it better this way?

I was not able to get rid of the hold_sb() helper which you didn't like much as
namespace_sem is private to fs/namespace.c and we need to drop it after
acquiring sb reference and before blocking on sb->s_umount semaphore. So
hold_sb() still looked like the least painful solution.

								Honza

[PATCH 2/2] proc: Protect readers of /proc/mounts from remount

[Jan Kara]

Readers of /proc/mounts (and similar files) are currently unprotected
from concurrently running remount on the filesystem they are reporting.
This can not only result in bogus reported information but also in
confusion in filesystems ->show_options callbacks resulting in
use-after-free issues or similar (for example ext4 handling of quota
file names is prone to such races).

Fix the problem by protecting showing of mount information with
sb->s_umount semaphore.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/namespace.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index a7f91265ea67..3b7cd4465658 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1263,7 +1263,7 @@ static void *m_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	struct proc_mounts *p = m->private;
 
-	p->cached_mount = seq_list_next(v, &p->ns->list, pos);
+	p->cached_mount = seq_list_next(p->cached_mount, &p->ns->list, pos);
 	p->cached_index = *pos;
 	return p->cached_mount;
 }
@@ -1276,8 +1276,38 @@ static void m_stop(struct seq_file *m, void *v)
 static int m_show(struct seq_file *m, void *v)
 {
 	struct proc_mounts *p = m->private;
-	struct mount *r = list_entry(v, struct mount, mnt_list);
-	return p->show(m, &r->mnt);
+	struct mount *r;
+	struct super_block *sb;
+	int ret;
+
+restart:
+	r = list_entry(v, struct mount, mnt_list);
+	sb = r->mnt.mnt_sb;
+
+	/* Protect show function against racing remount */
+	if (mount_trylock_super(sb))
+		goto show;
+	/*
+	 * Trylock failed. Since namepace_sem ranks below s_umount (through
+	 * sb->s_umount > dir->i_rwsem > namespace_sem in the mount path), we
+	 * have to drop it, wait for s_umount and then try again to guarantee
+	 * forward progress.
+	 */
+	hold_sb(sb);
+	up_read(&namespace_sem);
+	down_read(&sb->s_umount);
+	down_read(&namespace_sem);
+	v = seq_list_start(&p->ns->list, p->cached_index);
+	if (!sb->s_root || v != p->cached_mount) {
+		drop_super(sb);
+		p->cached_event = p->ns->event;
+		p->cached_mount = v;
+		goto restart;
+	}
+show:
+	ret = p->show(m, &r->mnt);
+	drop_super(sb);
+	return ret;
 }
 
 const struct seq_operations mounts_op = {
-- 
2.16.4

[PATCH 0/2] vfs: Fix crashes when reading /proc/mounts

[Jan Kara]

Hello,

this patch series aims at fixing possible races between functions formatting
output for /proc/mounts and friends and remount. Especially ->show_options
functions of filesystems are not counting with the fact that options can
change under them and thus races can result in bogus output or outright
crashes (like was the case with ext4 handling of quota mount option, or udf
could crash when printing charset name, or xfs when printing log device name
etc.).

Admittedly the patch 2/2 is somewhat ugly because namespace_sem ranks below
sb->s_umount semaphore but I think it is bearable. Al, what do you think?

								Honza

[PATCH 2/2] proc: Protect readers of /proc/mounts from remount

[Jan Kara]

Readers of /proc/mounts (and similar files) are currently unprotected
from concurrently running remount on the filesystem they are reporting.
This can not only result in bogus reported information but also in
confusion in filesystems ->show_options callbacks resulting in
use-after-free issues or similar (for example ext4 handling of quota
file names is prone to such races).

Fix the problem by protecting showing of mount information with
sb->s_umount semaphore.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/mount.h     |  1 +
 fs/namespace.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/fs/mount.h b/fs/mount.h
index f39bc9da4d73..5ca0a9e714b3 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -134,6 +134,7 @@ struct proc_mounts {
 	void *cached_mount;
 	u64 cached_event;
 	loff_t cached_index;
+	struct super_block *locked_sb;
 };
 
 extern const struct seq_operations mounts_op;
diff --git a/fs/namespace.c b/fs/namespace.c
index 99186556f8d3..31270baa4c0b 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1237,26 +1237,68 @@ struct vfsmount *mnt_clone_internal(const struct path *path)
 }
 
 #ifdef CONFIG_PROC_FS
+
+static bool mounts_trylock_super(struct proc_mounts *p, struct super_block *sb)
+{
+	if (p->locked_sb == sb)
+		return true;
+	if (p->locked_sb) {
+		drop_super(p->locked_sb);
+		p->locked_sb = NULL;
+	}
+	if (down_read_trylock(&sb->s_umount)) {
+		hold_sb(sb);
+		p->locked_sb = sb;
+		return true;
+	}
+	return false;
+}
+
 /* iterator; we want it to have access to namespace_sem, thus here... */
 static void *m_start(struct seq_file *m, loff_t *pos)
 {
 	struct proc_mounts *p = m->private;
+	struct mount *mount;
+	struct super_block *sb;
 
+restart:
 	down_read(&namespace_sem);
 	if (p->cached_event == p->ns->event) {
 		void *v = p->cached_mount;
 		if (*pos == p->cached_index)
-			return v;
+			goto lock_sb;
 		if (*pos == p->cached_index + 1) {
 			v = seq_list_next(v, &p->ns->list, &p->cached_index);
-			return p->cached_mount = v;
+			p->cached_mount = v;
+			goto lock_sb;
 		}
 	}
 
 	p->cached_event = p->ns->event;
 	p->cached_mount = seq_list_start(&p->ns->list, *pos);
 	p->cached_index = *pos;
-	return p->cached_mount;
+lock_sb:
+	if (!p->cached_mount)
+		return NULL;
+	mount = list_entry(p->cached_mount, struct mount, mnt_list);
+	sb = mount->mnt.mnt_sb;
+	if (mounts_trylock_super(p, sb))
+		return p->cached_mount;
+	/*
+	 * Trylock failed. Since namepace_sem ranks below s_umount (through
+	 * sb->s_umount > dir->i_rwsem > namespace_sem in the mount path), we
+	 * have to drop it, wait for s_umount and then try again to guarantee
+	 * forward progress.
+	 */
+	hold_sb(sb);
+	up_read(&namespace_sem);
+	down_read(&sb->s_umount);
+	/*
+	 * Sb may be dead by now but that just means we won't find it in any
+	 * mount and drop lock & reference anyway.
+	 */
+	p->locked_sb = sb;
+	goto restart;
 }
 
 static void *m_next(struct seq_file *m, void *v, loff_t *pos)
@@ -1277,7 +1319,16 @@ static int m_show(struct seq_file *m, void *v)
 {
 	struct proc_mounts *p = m->private;
 	struct mount *r = list_entry(v, struct mount, mnt_list);
-	return p->show(m, &r->mnt);
+	struct super_block *sb = r->mnt.mnt_sb;
+	int ret;
+
+	/* Protect show function against racing remount */
+	if (!mounts_trylock_super(p, sb))
+		return -EAGAIN;
+	ret = p->show(m, &r->mnt);
+	drop_super(sb);
+	p->locked_sb = NULL;
+	return ret;
 }
 
 const struct seq_operations mounts_op = {
-- 
2.16.4