Currently after copy-up, upper file will lose most of file attributions except copy-up triggered by setting fsflags. Because ioctl operation of underlying file systems does not expect calling from kernel component, it seems hard to copy fsflags during copy-up. Overlayfs keeps limited attributions(append-only, etc) in it's inode flags after successfully updating attributions. so ater copy-up, lsattr(1) does not show correct result but overlayfs can still prohibit ramdom write for those files which originally have append-only attribution. However, recently I found this protection can be easily broken in below operations. 1, Set append attribution to lower file. 2, Mount overlayfs. 3, Trigger copy-up by data append. 4, Set noatime attributtion to the file. 5, The file is random writable. This patch tries to keep some file attributions after copy-up so that overlayfs keeps compatible behavior with local filesystem as much as possible. Signed-off-by: Chengguang Xu <cgxu519@mykernel.net> --- fs/overlayfs/file.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index efccb7c1f9bc..e0eb055d00a6 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -655,12 +655,24 @@ static long ovl_ioctl_set_fsxflags(struct file *file, unsigned int cmd, long ovl_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { + unsigned int imask = S_SYNC | S_APPEND | S_NOATIME; + unsigned int fsmask = FS_SYNC_FL | FS_APPEND_FL | FS_NOATIME_FL; + unsigned int flags, ovl_fsflags; long ret; switch (cmd) { case FS_IOC_GETFLAGS: case FS_IOC_FSGETXATTR: ret = ovl_real_ioctl(file, cmd, arg); + if (!ret) { + if (get_user(flags, (int __user *) arg)) + return -EFAULT; + + ovl_fsflags = ovl_iflags_to_fsflags(file_inode(file)->i_flags & imask); + if ((flags & fsmask) != ovl_fsflags) + flags |= ovl_fsflags; + ret = put_user(flags, (int __user *)arg); + } break; case FS_IOC_SETFLAGS: -- 2.18.4
On Sat, Dec 26, 2020 at 12:48 PM Chengguang Xu <cgxu519@mykernel.net> wrote: > > Currently after copy-up, upper file will lose most of file > attributions except copy-up triggered by setting fsflags. > Because ioctl operation of underlying file systems does not > expect calling from kernel component, it seems hard to > copy fsflags during copy-up. > > Overlayfs keeps limited attributions(append-only, etc) in it's > inode flags after successfully updating attributions. so ater > copy-up, lsattr(1) does not show correct result but overlayfs > can still prohibit ramdom write for those files which originally > have append-only attribution. However, recently I found this > protection can be easily broken in below operations. > > 1, Set append attribution to lower file. > 2, Mount overlayfs. > 3, Trigger copy-up by data append. > 4, Set noatime attributtion to the file. > 5, The file is random writable. > > This patch tries to keep some file attributions after copy-up > so that overlayfs keeps compatible behavior with local filesystem > as much as possible. > This approach seems quite wrong. For one thing, mount cycle overlay or drop caches will result in loss of append only flag after copy-up, so this is not a security fix. Second, Miklos has already proposed a much more profound change to address this and similar issues [1] and he has already made some changes to ioctl handler to master doesn't have ovl_iflags_to_fsflags(). [1] https://lore.kernel.org/linux-fsdevel/20201123141207.GC327006@miu.piliscsaba.redhat.com/ One more thing. It seems like ovl_copyflags() in ovl_inode_init() would have been better to copy from ovl_inode_realdata() inode instead of ovl_inode_real(). This way, copy up still loses the append-only flag, but metacopy up does not. So at least for the common use case of containers that chown -R won't cause losing all the file flags. ovl_ioctl_set_flags() triggers data copy up, so that will break the link to lower flags anyway. I also noticed that we can lose the immutable flag on lower directory when copying up parents and it's probably not the only way to lose an immutable flag on a lower file/dir. Thanks, Amir.
---- 在 星期一, 2021-01-04 13:04:56 Amir Goldstein <amir73il@gmail.com> 撰写 ---- > On Sat, Dec 26, 2020 at 12:48 PM Chengguang Xu <cgxu519@mykernel.net> wrote: > > > > Currently after copy-up, upper file will lose most of file > > attributions except copy-up triggered by setting fsflags. > > Because ioctl operation of underlying file systems does not > > expect calling from kernel component, it seems hard to > > copy fsflags during copy-up. > > > > Overlayfs keeps limited attributions(append-only, etc) in it's > > inode flags after successfully updating attributions. so ater > > copy-up, lsattr(1) does not show correct result but overlayfs > > can still prohibit ramdom write for those files which originally > > have append-only attribution. However, recently I found this > > protection can be easily broken in below operations. > > > > 1, Set append attribution to lower file. > > 2, Mount overlayfs. > > 3, Trigger copy-up by data append. > > 4, Set noatime attributtion to the file. > > 5, The file is random writable. > > > > This patch tries to keep some file attributions after copy-up > > so that overlayfs keeps compatible behavior with local filesystem > > as much as possible. > > > > This approach seems quite wrong. > For one thing, mount cycle overlay or drop caches will result in loss > of append only flag after copy-up, so this is not a security fix. > You are right, I overlooked the case of dropping cache. > Second, Miklos has already proposed a much more profound change > to address this and similar issues [1] and he has already made some > changes to ioctl handler to master doesn't have ovl_iflags_to_fsflags(). > > [1] https://lore.kernel.org/linux-fsdevel/20201123141207.GC327006@miu.piliscsaba.redhat.com/ > > One more thing. > It seems like ovl_copyflags() in ovl_inode_init() would have been better > to copy from ovl_inode_realdata() inode instead of ovl_inode_real(). > This way, copy up still loses the append-only flag, but metacopy up > does not. So at least for the common use case of containers that > chown -R won't cause losing all the file flags. IIUC, the flags will still keep in overlayfs' inode after copy up until the inode cleaned by dropping cache. So I think your suggestion will be helpful for the case of meta-copyup & dropping cache. Hi Miklos Is it worth to change like above? > > ovl_ioctl_set_flags() triggers data copy up, so that will break the link > to lower flags anyway. I think though ovl_ioctl_set_flags() triggers data copy up but the flags will be set correctly to upper file, because chattr(1) will get the flags first and set the whole flags(include original flags) to upper file. Thanks, Chengguang
On Mon, Jan 4, 2021 at 8:55 AM Chengguang Xu <cgxu519@mykernel.net> wrote: > > ---- 在 星期一, 2021-01-04 13:04:56 Amir Goldstein <amir73il@gmail.com> 撰写 ---- > > On Sat, Dec 26, 2020 at 12:48 PM Chengguang Xu <cgxu519@mykernel.net> wrote: > > > > > > Currently after copy-up, upper file will lose most of file > > > attributions except copy-up triggered by setting fsflags. > > > Because ioctl operation of underlying file systems does not > > > expect calling from kernel component, it seems hard to > > > copy fsflags during copy-up. > > > > > > Overlayfs keeps limited attributions(append-only, etc) in it's > > > inode flags after successfully updating attributions. so ater > > > copy-up, lsattr(1) does not show correct result but overlayfs > > > can still prohibit ramdom write for those files which originally > > > have append-only attribution. However, recently I found this > > > protection can be easily broken in below operations. > > > > > > 1, Set append attribution to lower file. > > > 2, Mount overlayfs. > > > 3, Trigger copy-up by data append. > > > 4, Set noatime attributtion to the file. > > > 5, The file is random writable. > > > > > > This patch tries to keep some file attributions after copy-up > > > so that overlayfs keeps compatible behavior with local filesystem > > > as much as possible. > > > > > > > This approach seems quite wrong. > > For one thing, mount cycle overlay or drop caches will result in loss > > of append only flag after copy-up, so this is not a security fix. > > > > You are right, I overlooked the case of dropping cache. > > > Second, Miklos has already proposed a much more profound change > > to address this and similar issues [1] and he has already made some > > changes to ioctl handler to master doesn't have ovl_iflags_to_fsflags(). > > > > [1] https://lore.kernel.org/linux-fsdevel/20201123141207.GC327006@miu.piliscsaba.redhat.com/ > > > > One more thing. > > It seems like ovl_copyflags() in ovl_inode_init() would have been better > > to copy from ovl_inode_realdata() inode instead of ovl_inode_real(). > > This way, copy up still loses the append-only flag, but metacopy up > > does not. So at least for the common use case of containers that > > chown -R won't cause losing all the file flags. > > IIUC, the flags will still keep in overlayfs' inode after copy up until > the inode cleaned by dropping cache. So I think your suggestion will be > helpful for the case of meta-copyup & dropping cache. Yes, for the use case of chowning all files sure cannot rely on caches and I believe those containers are also used as persistent containers that can be mounted again later after initial ownership fix. > > Hi Miklos > > Is it worth to change like above? > I guess that depends what are the use cases that benefit. After all it is not a security fix it just increases the amount of use cases that preserve the append-only flag. I *think* it could fix a lot of cases like: chmod foo; drop_caches; touch foo # should fail mv foo bar; drop_caches; touch bar # should fail and in order to lose the append-only flag, users will need to first open with O_APPEND, set noatime flag or some unusual operations that do not happen by mistake as often as chmod,chown,rename. > > > > > ovl_ioctl_set_flags() triggers data copy up, so that will break the link > > to lower flags anyway. > > I think though ovl_ioctl_set_flags() triggers data copy up but the flags > will be set correctly to upper file, because chattr(1) will get the flags > first and set the whole flags(include original flags) to upper file. > Sure, unless the user is not privileged to set flags, but copy up will still happen. But what I meant is if user changes the flags, data copy up happens and ovl_copyflags() after drop caches will no longer copy the lower flags. Thanks, Amir.