| Message ID | 20190502214702.20535-1-christian.brauner@ubuntu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Disco] UBUNTU: SAUCE: shiftfs: lock down certain superblock flags | expand |
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
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
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;
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 <christian.brauner@ubuntu.com> --- fs/shiftfs.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)