| Message ID | 20190415132155.11260-1-christian.brauner@ubuntu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Disco] UBUNTU: SAUCE: shiftfs: prevent use-after-free when verifying mount options | expand |
On 15.04.19 15:21, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1824735 > > Copy up the passthrough mount settings of the mark mount point to the > shiftfs overlay. > > Before this commit we used to keep a reference to the shiftfs mark > mount's shiftfs_super_info which was stashed in the superblock of the > mark mount. The problem is that we only take a reference to the mount of > the underlay, i.e. the filesystem that is *under* the shiftfs mark > mount. This means when someone performs a shiftfs mark mount, then a > shiftfs overlay mount and then immediately unmounts the shiftfs mark > mount we muck with invalid memory since shiftfs_put_super might have > already been called freeing that memory. > > Another solution would be to start reference counting. But this would be > overkill. We only care about the passthrough mount option of the mark > mount. And we only need it to verify that on remount the new passthrough > options of the shiftfs overlay are a subset of the mark mount's > passthrough options. In other scenarios we don't care. So copying up is > good enough and also only needs to happen once on mount, i.e. when a new > superblock is created and the .fill_super method is called. > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > fs/shiftfs.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index 8e064756ea0c..4c8a6ec2a617 100644 > --- a/fs/shiftfs.c > +++ b/fs/shiftfs.c > @@ -28,7 +28,7 @@ struct shiftfs_super_info { > const struct cred *creator_cred; > bool mark; > unsigned int passthrough; > - struct shiftfs_super_info *info_mark; > + unsigned int passthrough_mark; > }; > > struct shiftfs_file_info { > @@ -52,10 +52,6 @@ static inline bool shiftfs_passthrough_ioctls(struct shiftfs_super_info *info) > if (!(info->passthrough & SHIFTFS_PASSTHROUGH_IOCTL)) > return false; > > - if (info->info_mark && > - !(info->info_mark->passthrough & SHIFTFS_PASSTHROUGH_IOCTL)) > - return false; > - > return true; > } > > @@ -64,10 +60,6 @@ static inline bool shiftfs_passthrough_statfs(struct shiftfs_super_info *info) > if (!(info->passthrough & SHIFTFS_PASSTHROUGH_STAT)) > return false; > > - if (info->info_mark && > - !(info->info_mark->passthrough & SHIFTFS_PASSTHROUGH_STAT)) > - return false; > - > return true; > } > > @@ -1824,7 +1816,7 @@ static int shiftfs_remount(struct super_block *sb, int *flags, char *data) > > if (info->passthrough != new.passthrough) { > /* Don't allow exceeding passthrough options of mark mount. */ > - if (!passthrough_is_subset(info->info_mark->passthrough, > + if (!passthrough_is_subset(info->passthrough_mark, > info->passthrough)) > return -EPERM; > > @@ -1926,9 +1918,19 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > > sbinfo->mnt = mntget(sbinfo_mp->mnt); > dentry = dget(path.dentry->d_fsdata); > + /* > + * Copy up the passthrough mount options from the > + * parent mark mountpoint. > + */ > + sbinfo->passthrough_mark = sbinfo_mp->passthrough_mark; > } else { > sbinfo->mnt = mntget(path.mnt); > dentry = dget(path.dentry); > + /* > + * For a new mark passthrough_mark and passthrough > + * are identical. > + */ > + sbinfo->passthrough_mark = sbinfo->passthrough; > } > > sbinfo->creator_cred = prepare_creds(); > @@ -1956,7 +1958,12 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, > sbinfo->mnt = mntget(sbinfo_mp->mnt); > sbinfo->creator_cred = get_cred(sbinfo_mp->creator_cred); > dentry = dget(path.dentry->d_fsdata); > - sbinfo->info_mark = sbinfo_mp; > + /* > + * Copy up passthrough settings from mark mountpoint so we can > + * verify when the overlay wants to remount with different > + * passthrough settings. > + */ > + sbinfo->passthrough_mark = sbinfo_mp->passthrough; > } > > sb->s_stack_depth = dentry->d_sb->s_stack_depth + 1; >
On Mon, Apr 15, 2019 at 03:21:55PM +0200, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1824735 > > Copy up the passthrough mount settings of the mark mount point to the > shiftfs overlay. > > Before this commit we used to keep a reference to the shiftfs mark > mount's shiftfs_super_info which was stashed in the superblock of the > mark mount. The problem is that we only take a reference to the mount of > the underlay, i.e. the filesystem that is *under* the shiftfs mark > mount. This means when someone performs a shiftfs mark mount, then a > shiftfs overlay mount and then immediately unmounts the shiftfs mark > mount we muck with invalid memory since shiftfs_put_super might have > already been called freeing that memory. > > Another solution would be to start reference counting. But this would be > overkill. We only care about the passthrough mount option of the mark > mount. And we only need it to verify that on remount the new passthrough > options of the shiftfs overlay are a subset of the mark mount's > passthrough options. In other scenarios we don't care. So copying up is > good enough and also only needs to happen once on mount, i.e. when a new > superblock is created and the .fill_super method is called. > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Applied to disco/master-next, thanks!
diff --git a/fs/shiftfs.c b/fs/shiftfs.c index 8e064756ea0c..4c8a6ec2a617 100644 --- a/fs/shiftfs.c +++ b/fs/shiftfs.c @@ -28,7 +28,7 @@ struct shiftfs_super_info { const struct cred *creator_cred; bool mark; unsigned int passthrough; - struct shiftfs_super_info *info_mark; + unsigned int passthrough_mark; }; struct shiftfs_file_info { @@ -52,10 +52,6 @@ static inline bool shiftfs_passthrough_ioctls(struct shiftfs_super_info *info) if (!(info->passthrough & SHIFTFS_PASSTHROUGH_IOCTL)) return false; - if (info->info_mark && - !(info->info_mark->passthrough & SHIFTFS_PASSTHROUGH_IOCTL)) - return false; - return true; } @@ -64,10 +60,6 @@ static inline bool shiftfs_passthrough_statfs(struct shiftfs_super_info *info) if (!(info->passthrough & SHIFTFS_PASSTHROUGH_STAT)) return false; - if (info->info_mark && - !(info->info_mark->passthrough & SHIFTFS_PASSTHROUGH_STAT)) - return false; - return true; } @@ -1824,7 +1816,7 @@ static int shiftfs_remount(struct super_block *sb, int *flags, char *data) if (info->passthrough != new.passthrough) { /* Don't allow exceeding passthrough options of mark mount. */ - if (!passthrough_is_subset(info->info_mark->passthrough, + if (!passthrough_is_subset(info->passthrough_mark, info->passthrough)) return -EPERM; @@ -1926,9 +1918,19 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, sbinfo->mnt = mntget(sbinfo_mp->mnt); dentry = dget(path.dentry->d_fsdata); + /* + * Copy up the passthrough mount options from the + * parent mark mountpoint. + */ + sbinfo->passthrough_mark = sbinfo_mp->passthrough_mark; } else { sbinfo->mnt = mntget(path.mnt); dentry = dget(path.dentry); + /* + * For a new mark passthrough_mark and passthrough + * are identical. + */ + sbinfo->passthrough_mark = sbinfo->passthrough; } sbinfo->creator_cred = prepare_creds(); @@ -1956,7 +1958,12 @@ static int shiftfs_fill_super(struct super_block *sb, void *raw_data, sbinfo->mnt = mntget(sbinfo_mp->mnt); sbinfo->creator_cred = get_cred(sbinfo_mp->creator_cred); dentry = dget(path.dentry->d_fsdata); - sbinfo->info_mark = sbinfo_mp; + /* + * Copy up passthrough settings from mark mountpoint so we can + * verify when the overlay wants to remount with different + * passthrough settings. + */ + sbinfo->passthrough_mark = sbinfo_mp->passthrough; } sb->s_stack_depth = dentry->d_sb->s_stack_depth + 1;
BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1824735 Copy up the passthrough mount settings of the mark mount point to the shiftfs overlay. Before this commit we used to keep a reference to the shiftfs mark mount's shiftfs_super_info which was stashed in the superblock of the mark mount. The problem is that we only take a reference to the mount of the underlay, i.e. the filesystem that is *under* the shiftfs mark mount. This means when someone performs a shiftfs mark mount, then a shiftfs overlay mount and then immediately unmounts the shiftfs mark mount we muck with invalid memory since shiftfs_put_super might have already been called freeing that memory. Another solution would be to start reference counting. But this would be overkill. We only care about the passthrough mount option of the mark mount. And we only need it to verify that on remount the new passthrough options of the shiftfs overlay are a subset of the mark mount's passthrough options. In other scenarios we don't care. So copying up is good enough and also only needs to happen once on mount, i.e. when a new superblock is created and the .fill_super method is called. Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> --- fs/shiftfs.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)