[PATCH][SRU][Disco] UBUNTU: SAUCE: shiftfs: lock down certain superblock flags

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH][SRU][Disco] UBUNTU: SAUCE: shiftfs: lock down certain superblock flags

Christian Brauner
BugLink: https://bugs.launchpad.net/bugs/1827122

This locks down various superblock flags to prevent userns-root from
remounting a superblock with less restrictive options than the original
mark or underlay mount.

Signed-off-by: Christian Brauner <[hidden email]>
---
 fs/shiftfs.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index 9771165d1ce0..ee2e770810b9 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -1808,6 +1808,33 @@ static inline bool passthrough_is_subset(int old_flags, int new_flags)
  return true;
 }
 
+static int shiftfs_super_check_flags(unsigned long old_flags,
+     unsigned long new_flags)
+{
+ if ((old_flags & SB_RDONLY) && !(new_flags & SB_RDONLY))
+ return -EPERM;
+
+ if ((old_flags & SB_NOSUID) && !(new_flags & SB_NOSUID))
+ return -EPERM;
+
+ if ((old_flags & SB_NODEV) && !(new_flags & SB_NODEV))
+ return -EPERM;
+
+ if ((old_flags & SB_NOEXEC) && !(new_flags & SB_NOEXEC))
+ return -EPERM;
+
+ if ((old_flags & SB_NOATIME) && !(new_flags & SB_NOATIME))
+ return -EPERM;
+
+ if ((old_flags & SB_NODIRATIME) && !(new_flags & SB_NODIRATIME))
+ return -EPERM;
+
+ if (!(old_flags & SB_POSIXACL) && (new_flags & SB_POSIXACL))
+ return -EPERM;
+
+ return 0;
+}
+
 static int shiftfs_remount(struct super_block *sb, int *flags, char *data)
 {
  int err;
@@ -1818,6 +1845,10 @@ static int shiftfs_remount(struct super_block *sb, int *flags, char *data)
  if (err)
  return err;
 
+ err = shiftfs_super_check_flags(sb->s_flags, *flags);
+ if (err)
+ return err;
+
  /* Mark mount option cannot be changed. */
  if (info->mark || (info->mark != new.mark))
  return -EPERM;
@@ -1847,6 +1878,31 @@ struct shiftfs_data {
  const char *path;
 };
 
+static void shiftfs_super_force_flags(struct super_block *sb,
+      unsigned long lower_flags)
+{
+ if (lower_flags & SB_RDONLY)
+ sb->s_flags |= SB_RDONLY;
+
+ if (lower_flags & SB_NOSUID)
+ sb->s_flags |= SB_NOSUID;
+
+ if (lower_flags & SB_NODEV)
+ sb->s_flags |= SB_NODEV;
+
+ if (lower_flags & SB_NOEXEC)
+ sb->s_flags |= SB_NOEXEC;
+
+ if (lower_flags & SB_NOATIME)
+ sb->s_flags |= SB_NOATIME;
+
+ if (lower_flags & SB_NODIRATIME)
+ sb->s_flags |= SB_NODIRATIME;
+
+ if (!(lower_flags & SB_POSIXACL))
+ sb->s_flags &= ~SB_POSIXACL;
+}
+
 static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
       int silent)
 {
@@ -1904,6 +1960,8 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
  */
  sb->s_iflags = SB_I_NOEXEC;
 
+ shiftfs_super_force_flags(sb, lower_sb->s_flags);
+
  /*
  * Handle nesting of shiftfs mounts by referring this mark
  * mount back to the original mark mount. This is more
@@ -1972,6 +2030,7 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
  * passthrough settings.
  */
  sbinfo->passthrough_mark = sbinfo_mp->passthrough;
+ shiftfs_super_force_flags(sb, path.mnt->mnt_sb->s_flags);
  }
 
  sb->s_stack_depth = dentry->d_sb->s_stack_depth + 1;
--
2.21.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NAK: [PATCH][SRU][Disco] UBUNTU: SAUCE: shiftfs: lock down certain superblock flags

Seth Forshee
On Thu, May 02, 2019 at 11:47:02PM +0200, Christian Brauner wrote:

> +static void shiftfs_super_force_flags(struct super_block *sb,
> +      unsigned long lower_flags)
> +{
> + if (lower_flags & SB_RDONLY)
> + sb->s_flags |= SB_RDONLY;
> +
> + if (lower_flags & SB_NOSUID)
> + sb->s_flags |= SB_NOSUID;
> +
> + if (lower_flags & SB_NODEV)
> + sb->s_flags |= SB_NODEV;
> +
> + if (lower_flags & SB_NOEXEC)
> + sb->s_flags |= SB_NOEXEC;
> +
> + if (lower_flags & SB_NOATIME)
> + sb->s_flags |= SB_NOATIME;
> +
> + if (lower_flags & SB_NODIRATIME)
> + sb->s_flags |= SB_NODIRATIME;

It seems kind of odd to silently set rather important flags that weren't
requested rather than returning an error, though I do find precedent for
it. Seems like unexpectedly ending up with a read-only or noexec
filesystem isn't great, but I guess it's a thing the kernel does.

But what about making this more succinct, something like:

        sb->s_flags |= lower_flags & (SB_RDONLY | SB_NOSUID | SB_NODEV |
                                      SB_NOEXEC | SB_NOATIME | SB_NODIRATIME);

> +
> + if (!(lower_flags & SB_POSIXACL))
> + sb->s_flags &= ~SB_POSIXACL;
> +}
> +
>  static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
>        int silent)
>  {
> @@ -1904,6 +1960,8 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
>   */
>   sb->s_iflags = SB_I_NOEXEC;
>  
> + shiftfs_super_force_flags(sb, lower_sb->s_flags);
> +
>   /*
>   * Handle nesting of shiftfs mounts by referring this mark
>   * mount back to the original mark mount. This is more
> @@ -1972,6 +2030,7 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
>   * passthrough settings.
>   */
>   sbinfo->passthrough_mark = sbinfo_mp->passthrough;
> + shiftfs_super_force_flags(sb, path.mnt->mnt_sb->s_flags);
>   }

You're calling this before shiftfs_fill_super() unconditionally sets
SB_POSIXACL, which renders forcing that flag ineffective. This needs to
be fixed.

Thanks,
Seth

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: NAK: [PATCH][SRU][Disco] UBUNTU: SAUCE: shiftfs: lock down certain superblock flags

Christian Brauner-2
On Tue, May 07, 2019 at 02:50:27PM -0500, Seth Forshee wrote:

> On Thu, May 02, 2019 at 11:47:02PM +0200, Christian Brauner wrote:
> > +static void shiftfs_super_force_flags(struct super_block *sb,
> > +      unsigned long lower_flags)
> > +{
> > + if (lower_flags & SB_RDONLY)
> > + sb->s_flags |= SB_RDONLY;
> > +
> > + if (lower_flags & SB_NOSUID)
> > + sb->s_flags |= SB_NOSUID;
> > +
> > + if (lower_flags & SB_NODEV)
> > + sb->s_flags |= SB_NODEV;
> > +
> > + if (lower_flags & SB_NOEXEC)
> > + sb->s_flags |= SB_NOEXEC;
> > +
> > + if (lower_flags & SB_NOATIME)
> > + sb->s_flags |= SB_NOATIME;
> > +
> > + if (lower_flags & SB_NODIRATIME)
> > + sb->s_flags |= SB_NODIRATIME;
>
> It seems kind of odd to silently set rather important flags that weren't
> requested rather than returning an error, though I do find precedent for
> it. Seems like unexpectedly ending up with a read-only or noexec
> filesystem isn't great, but I guess it's a thing the kernel does.

Yep, it's the standard right now. Not sure, what the new mount api will
end up doing though...

>
> But what about making this more succinct, something like:
>
>         sb->s_flags |= lower_flags & (SB_RDONLY | SB_NOSUID | SB_NODEV |
>                                       SB_NOEXEC | SB_NOATIME | SB_NODIRATIME);

Yep, sounds good, will do.

>
> > +
> > + if (!(lower_flags & SB_POSIXACL))
> > + sb->s_flags &= ~SB_POSIXACL;
> > +}
> > +
> >  static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
> >        int silent)
> >  {
> > @@ -1904,6 +1960,8 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
> >   */
> >   sb->s_iflags = SB_I_NOEXEC;
> >  
> > + shiftfs_super_force_flags(sb, lower_sb->s_flags);
> > +
> >   /*
> >   * Handle nesting of shiftfs mounts by referring this mark
> >   * mount back to the original mark mount. This is more
> > @@ -1972,6 +2030,7 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data,
> >   * passthrough settings.
> >   */
> >   sbinfo->passthrough_mark = sbinfo_mp->passthrough;
> > + shiftfs_super_force_flags(sb, path.mnt->mnt_sb->s_flags);
> >   }
>
> You're calling this before shiftfs_fill_super() unconditionally sets
> SB_POSIXACL, which renders forcing that flag ineffective. This needs to
> be fixed.

Oh good catch!
I think I'll just move setting that flag further up.

Thanks!
Christian

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team