netdev - Re: [RFC][PATCH net] bpf: Use mount_nodev not mount

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: <573CD3D8.3030308@iogearbox.net>
Date:	Wed, 18 May 2016 22:43:04 +0200
From:	Daniel Borkmann <daniel@...earbox.net>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>
CC:	Alexei Starovoitov <ast@...nel.org>,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [RFC][PATCH net] bpf: Use mount_nodev not mount_ns to mount the
 bpf filesystem

On 05/18/2016 04:56 PM, Eric W. Biederman wrote:
> Hannes Frederic Sowa <hannes@...essinduktion.org> writes:
>> On 18.05.2016 01:12, Eric W. Biederman wrote:
>>>
>>> While reviewing the filesystems that set FS_USERNS_MOUNT I spotted the
>>> bpf filesystem.  Looking at the code I saw a broken usage of mount_ns
>>> with current->nsproxy->mnt_ns. As the code does not acquire a reference
>>> to the mount namespace it can not possibly be correct to store the mount
>>> namespace on the superblock as it does.
>>>
>>> Replace mount_ns with mount_nodev so that each mount of the bpf
>>> filesystem returns a distinct instance, and the code is not utterly
>>> broken.
>>>
>>> Fixes: b2197755b263 ("bpf: add support for persistent maps/progs")
>>> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
>>> ---
>>>
>>> No one should care about this change, as userspace typically only mounts
>>> things once and does not depend on things in one mount do not showing up
>>> in another.  Can someone who actually uses the bpf filesystem please
>>> verify this.
[...]

LGTM.

Acked-by: Daniel Borkmann <daniel@...earbox.net>

>> The idea is to have the bpf filesystem as a singeleton per mnt-namespace
>> to prevent endless instances being created and kernel resources being
>> hogged by pinning them to hard to discover bpf mounts.

Eric, please send the patch officially and feel free to add my Ack. Given
the circumstances, moving to mount_nodev() seems the best way forward. To
also address above mentioned concern from Hannes, we need to remove the
FS_USERNS_MOUNT flag along with the change. It looks like the fix is best
addressed in a single patch if you want to include it. If not, we can
otherwise send it separately as well, I don't mind.

Thanks for your feedback!

diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index 8f94ca1..b2aefa2 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -378,7 +378,7 @@ static int bpf_fill_super(struct super_block *sb, void *data, int silent)
  static struct dentry *bpf_mount(struct file_system_type *type, int flags,
  				const char *dev_name, void *data)
  {
-	return mount_ns(type, flags, current->nsproxy->mnt_ns, bpf_fill_super);
+	return mount_nodev(type, flags, data, bpf_fill_super);
  }

  static struct file_system_type bpf_fs_type = {
@@ -386,7 +386,6 @@ static struct file_system_type bpf_fs_type = {
  	.name		= "bpf",
  	.mount		= bpf_mount,
  	.kill_sb	= kill_litter_super,
-	.fs_flags	= FS_USERNS_MOUNT,
  };

  MODULE_ALIAS_FS("bpf");
-- 
1.9.3