[RFC PATCH] ovl: keep some file attrubutions after copy-up

[RFC PATCH] ovl: keep some file attrubutions after copy-up

[Chengguang Xu]

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

Re: [RFC PATCH] ovl: keep some file attrubutions after copy-up

[Amir Goldstein]

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.

Re: [RFC PATCH] ovl: keep some file attrubutions after copy-up

[Chengguang Xu]

 ---- 在 星期一, 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

Re: [RFC PATCH] ovl: keep some file attrubutions after copy-up

[Amir Goldstein]

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.