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?
-Eric
---
security/selinux/hooks.c | 38 ++++++++++++++++++++++++++++++++++----
1 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 89bb6d3..71b01a4 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -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;
On Wed, 09 Apr 2008 11:05:02 -0400
Eric Paris <[email protected]> wrote:
> 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?
>
> -Eric
>
> ---
>
> security/selinux/hooks.c | 38 ++++++++++++++++++++++++++++++++++----
> 1 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 89bb6d3..71b01a4 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -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:
int (*compare_super)(struct super_block *, void *) = nfs_compare_super;
...
s = sget(&nfs_fs_type, compare_super, nfs_set_super, &sb_mntdata);
...if "compare_super" succeeds then an existing superblock will be
returned here.
To reproduce this, you'll probably need to have a server that
exports 2 filesystems, one mounted on the other. I've not tested this
but I suspect something like this would work:
Suppose you have 2 filesystems on the server /a and /a/b. Export them
both, and make sure to export /a/b with "nohide".
Now, on the client, mount up /a/b on a directory.
Then mount up /a in a different spot and "cd" into /a/b through that
mountpoint. When you walk into /a/b then it should search and find the
existing superblock from the first mount and use that.
If that's not clear, ping me and I'll see if I can help...
Cheers,
--
Jeff Layton <[email protected]>
On Wed, 2008-04-09 at 11:05 -0400, Eric Paris wrote:
> 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?
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.
> -Eric
>
> ---
>
> security/selinux/hooks.c | 38 ++++++++++++++++++++++++++++++++++----
> 1 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 89bb6d3..71b01a4 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -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;
>
--
Stephen Smalley
National Security Agency
On Wed, 2008-04-09 at 11:25 -0400, Stephen Smalley wrote:
> On Wed, 2008-04-09 at 11:05 -0400, Eric Paris wrote:
> > 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?
>
> 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.....
-Eric
On Wed, 2008-04-09 at 12:08 -0400, Eric Paris wrote:
> On Wed, 2008-04-09 at 11:25 -0400, Stephen Smalley wrote:
> > On Wed, 2008-04-09 at 11:05 -0400, Eric Paris wrote:
> > > 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?
> >
> > 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.....
Yes, and that is consistent with how user-initiated mounts used to work,
IIRC - first mount would determine the security options and all
subsequent ones would just re-use them as the superblock security
structure was already set up.
--
Stephen Smalley
National Security Agency