nfs_xdev_get_sb appears to sometimes reuse a sb and makes SELinux angry

When I wrote the new NFS/SELinux mount options code I was working under the assumption that nfs_xdev_get_sb() would always give a new superblock without the security struct initialized. I now have a report of a user in which we hit BUG_ON(newsbsec->initialized) indicating to me that NFS is reusing a superblock. The user says that he only has one mount to the server in fstab, but doesn't know much about the server setup. Is it expected that nfs_xdev_get_sb might reuse a superblock? If so maybe we want this patch below? Instead of me BUGing every time selinux sees a reused superblock we but only if the reused superblock has different security options than the old one. I can't reproduce the issue so I can't really test it....

comments? should NFS be reusing a superblock here? Can the NFS people let me know how I can trigger it to make sure my patch fixes it?

(quoted diff) +0 -2

-Eric
 
---
 
 security/selinux/hooks.c |   38 ++++++++++++++++++++++++++++++++++----
 1 files changed, 34 insertions(+), 4 deletions(-)

security/selinux/hooks.c +34 -4

index 89bb6d3..71b01a4 100644
@@ -760,13 +760,43 @@ static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
 	 * this early in the boot process. */
 	BUG_ON(!ss_initialized);
 
-	/* this might go away sometime down the line if there is a new user
-	 * of clone, but for now, nfs better not get here... */
-	BUG_ON(newsbsec->initialized);
-
 	/* how can we clone if the old one wasn't set up?? */
 	BUG_ON(!oldsbsec->initialized);
 
+	/*
+	 * check to make sure something isn't trying to reuse a superblock that
+	 * isn't actually the same.  if it is, that's the FS fault, not ours
+	 */
+	if (unlikely(newsbsec->initialized)) {
+		struct security_mnt_opts new_opts, old_opts;
+		int i;
+
+		if (newsb == oldsb)
+			return;
+
+		if (selinux_get_mnt_opts(newsb, &new_opts))
+			return;
+		if (selinux_get_mnt_opts(oldsb, &old_opts)) {
+			security_free_mnt_opts(&new_opts);
+			return;
+		}
+
+		BUG_ON(new_opts.num_mnt_opts != old_opts.num_mnt_opts);
+
+		/*
+		 * this is safe since _get_mnt_opts puts things in the same
+		 * order every time.
+		 */
+		for (i = 0; i < new_opts.num_mnt_opts; i++) {
+			BUG_ON(new_opts.mnt_opts_flags[i] != old_opts.mnt_opts_flags[i]);
+			BUG_ON(strcmp(new_opts.mnt_opts[i], old_opts.mnt_opts[i]));
+		}
+
+		security_free_mnt_opts(&new_opts);
+		security_free_mnt_opts(&old_opts);
+		return;
+	}
+
 	mutex_lock(&newsbsec->lock);
 
 	newsbsec->flags = oldsbsec->flags;

From nfs_xdev_get_sb:

When I wrote the new NFS/SELinux mount options code I was working under the assumption that nfs_xdev_get_sb() would always give a new superblock without the security struct initialized. I now have a report of a user in which we hit BUG_ON(newsbsec->initialized) indicating to me that NFS is reusing a superblock. The user says that he only has one mount to the server in fstab, but doesn't know much about the server setup. Is it expected that nfs_xdev_get_sb might reuse a superblock? If so maybe we want this patch below? Instead of me BUGing every time selinux sees a reused superblock we but only if the reused superblock has different security options than the old one. I can't reproduce the issue so I can't really test it....

comments? should NFS be reusing a superblock here? Can the NFS people let me know how I can trigger it to make sure my patch fixes it?

(quoted diff) +0 -2

-Eric
 
---
 
 security/selinux/hooks.c |   38 ++++++++++++++++++++++++++++++++++----
 1 files changed, 34 insertions(+), 4 deletions(-)

security/selinux/hooks.c +34 -4

index 89bb6d3..71b01a4 100644
@@ -760,13 +760,43 @@ static void selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
 	 * this early in the boot process. */
 	BUG_ON(!ss_initialized);
 
-	/* this might go away sometime down the line if there is a new user
-	 * of clone, but for now, nfs better not get here... */
-	BUG_ON(newsbsec->initialized);
-
 	/* how can we clone if the old one wasn't set up?? */
 	BUG_ON(!oldsbsec->initialized);
 
+	/*
+	 * check to make sure something isn't trying to reuse a superblock that
+	 * isn't actually the same.  if it is, that's the FS fault, not ours
+	 */
+	if (unlikely(newsbsec->initialized)) {
+		struct security_mnt_opts new_opts, old_opts;
+		int i;
+
+		if (newsb == oldsb)
+			return;
+
+		if (selinux_get_mnt_opts(newsb, &new_opts))
+			return;
+		if (selinux_get_mnt_opts(oldsb, &old_opts)) {
+			security_free_mnt_opts(&new_opts);
+			return;
+		}
+
+		BUG_ON(new_opts.num_mnt_opts != old_opts.num_mnt_opts);
+
+		/*
+		 * this is safe since _get_mnt_opts puts things in the same
+		 * order every time.
+		 */
+		for (i = 0; i < new_opts.num_mnt_opts; i++) {
+			BUG_ON(new_opts.mnt_opts_flags[i] != old_opts.mnt_opts_flags[i]);
+			BUG_ON(strcmp(new_opts.mnt_opts[i], old_opts.mnt_opts[i]));
+		}
+
+		security_free_mnt_opts(&new_opts);
+		security_free_mnt_opts(&old_opts);
+		return;
+	}
+
 	mutex_lock(&newsbsec->lock);
 
 	newsbsec->flags = oldsbsec->flags;

Quoted 12 lines · Eric Paris

When I wrote the new NFS/SELinux mount options code I was working under the assumption that nfs_xdev_get_sb() would always give a new superblock without the security struct initialized. I now have a report of a user in which we hit BUG_ON(newsbsec->initialized) indicating to me that NFS is reusing a superblock. The user says that he only has one mount to the server in fstab, but doesn't know much about the server setup. Is it expected that nfs_xdev_get_sb might reuse a superblock? If so maybe we want this patch below? Instead of me BUGing every time selinux sees a reused superblock we but only if the reused superblock has different security options than the old one. I can't reproduce the issue so I can't really test it....

Quoted 2 lines

comments? should NFS be reusing a superblock here? Can the NFS people let me know how I can trigger it to make sure my patch fixes it?

Looks to me like nfs_compare_mount_options() needs to also compare security options as part of the criteria for deciding when sharing is permissible. Otherwise, it seems quite possible that you'll still hit the new BUG.

Quoted 13 lines · Stephen Smalley

Quoted 12 lines · Eric Paris

When I wrote the new NFS/SELinux mount options code I was working under the assumption that nfs_xdev_get_sb() would always give a new superblock without the security struct initialized. I now have a report of a user in which we hit BUG_ON(newsbsec->initialized) indicating to me that NFS is reusing a superblock. The user says that he only has one mount to the server in fstab, but doesn't know much about the server setup. Is it expected that nfs_xdev_get_sb might reuse a superblock? If so maybe we want this patch below? Instead of me BUGing every time selinux sees a reused superblock we but only if the reused superblock has different security options than the old one. I can't reproduce the issue so I can't really test it....

Quoted 7 lines

Quoted 2 lines

comments? should NFS be reusing a superblock here? Can the NFS people let me know how I can trigger it to make sure my patch fixes it?

Looks to me like nfs_compare_mount_options() needs to also compare security options as part of the criteria for deciding when sharing is permissible. Otherwise, it seems quite possible that you'll still hit the new BUG.

Making nfs_compare_mount_options() security aware is not the right answer. Doing so means that we could end up with the same data 2 places with different security options. This is just not what we want...

I could make the _clone_ functions return -EINVAL instead of BUG but since this is inside nfs_xdev_get_sb the user has no way to 'fix' it. So that's not the right fix. (this is what I do with user controlled mounts already which go through nfs_get_sb)

I'm currently leaning towards changing all of this to

if (newsbsec->initialized)

	return;

so the first sb wins and its mount options stick forever. At least we know we did our best to maintain labeling of the data and nothing is going to explode.....