Stable Archive on lore.kernel.org
 help / Atom feed
* [PATCH][V3] overlayfs: During copy up, first copy up data and then xattrs
@ 2019-01-11 18:37 Vivek Goyal
  2019-01-15  9:26 ` Miklos Szeredi
  0 siblings, 1 reply; 2+ messages in thread
From: Vivek Goyal @ 2019-01-11 18:37 UTC (permalink / raw)
  To: linux-unionfs, miklos, amir73il; +Cc: stable, dwalsh, gscrivan

If a file with capability set (and hence security.capability xattr) is
written kernel clears security.capability xattr. For overlay, during file
copy up if xattrs are copied up first and then data is, copied up. This
means data copy up will result in clearing of security.capability xattr
file on lower has. And this can result into surprises. If
a lower file has CAP_SETUID, then it should not be cleared over
copy up (if nothing was actually written to file).

This also creates problems with chown logic where it first copies up file
and then tries to clear setuid bit. But by that time security.capability
xattr is already gone (due to data copy up), and caller gets -ENODATA.
This has been reported by Giuseppe here.

https://github.com/containers/libpod/issues/2015#issuecomment-447824842

Fix this by copying up data first and then metadta. This is a regression
which has been introduced by my commit as part of metadata only copy up
patches.

TODO: There will be some corner cases where a file is copied up metadata
      only and later data copy up happens and that will clear
      security.capability xattr. Something needs to be done about that too.

Fixes: bd64e57586d3 ("ovl: During copy up, first copy up metadata and then data")
Cc: <stable@vger.kernel.org> # v4.19+
Reported-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/copy_up.c |   31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

Index: rhvgoyal-linux/fs/overlayfs/copy_up.c
===================================================================
--- rhvgoyal-linux.orig/fs/overlayfs/copy_up.c	2019-01-10 17:25:25.594461223 -0500
+++ rhvgoyal-linux/fs/overlayfs/copy_up.c	2019-01-11 10:47:33.657461223 -0500
@@ -443,6 +443,24 @@ static int ovl_copy_up_inode(struct ovl_
 {
 	int err;
 
+	/*
+	 * Copy up data first and then xattrs. Writing data after
+	 * xattrs will remove security.capability xattr automatically.
+	 */
+	if (S_ISREG(c->stat.mode) && !c->metacopy) {
+		struct path upperpath, datapath;
+
+		ovl_path_upper(c->dentry, &upperpath);
+		if(WARN_ON(upperpath.dentry != NULL))
+			return -EIO;
+		upperpath.dentry = temp;
+
+		ovl_path_lowerdata(c->dentry, &datapath);
+		err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
+		if (err)
+			return err;
+	}
+
 	err = ovl_copy_xattr(c->lowerpath.dentry, temp);
 	if (err)
 		return err;
@@ -459,19 +477,6 @@ static int ovl_copy_up_inode(struct ovl_
 		if (err)
 			return err;
 	}
-
-	if (S_ISREG(c->stat.mode) && !c->metacopy) {
-		struct path upperpath, datapath;
-
-		ovl_path_upper(c->dentry, &upperpath);
-		BUG_ON(upperpath.dentry != NULL);
-		upperpath.dentry = temp;
-
-		ovl_path_lowerdata(c->dentry, &datapath);
-		err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
-		if (err)
-			return err;
-	}
 
 	if (c->metacopy) {
 		err = ovl_check_setxattr(c->dentry, temp, OVL_XATTR_METACOPY,

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH][V3] overlayfs: During copy up, first copy up data and then xattrs
  2019-01-11 18:37 [PATCH][V3] overlayfs: During copy up, first copy up data and then xattrs Vivek Goyal
@ 2019-01-15  9:26 ` Miklos Szeredi
  0 siblings, 0 replies; 2+ messages in thread
From: Miklos Szeredi @ 2019-01-15  9:26 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: overlayfs, Amir Goldstein, stable, Daniel J Walsh, gscrivan

On Fri, Jan 11, 2019 at 7:37 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> If a file with capability set (and hence security.capability xattr) is
> written kernel clears security.capability xattr. For overlay, during file
> copy up if xattrs are copied up first and then data is, copied up. This
> means data copy up will result in clearing of security.capability xattr
> file on lower has. And this can result into surprises. If
> a lower file has CAP_SETUID, then it should not be cleared over
> copy up (if nothing was actually written to file).
>
> This also creates problems with chown logic where it first copies up file
> and then tries to clear setuid bit. But by that time security.capability
> xattr is already gone (due to data copy up), and caller gets -ENODATA.
> This has been reported by Giuseppe here.
>
> https://github.com/containers/libpod/issues/2015#issuecomment-447824842
>
> Fix this by copying up data first and then metadta. This is a regression
> which has been introduced by my commit as part of metadata only copy up
> patches.
>
> TODO: There will be some corner cases where a file is copied up metadata
>       only and later data copy up happens and that will clear
>       security.capability xattr. Something needs to be done about that too.
>
> Fixes: bd64e57586d3 ("ovl: During copy up, first copy up metadata and then data")
> Cc: <stable@vger.kernel.org> # v4.19+
> Reported-by: Giuseppe Scrivano <gscrivan@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Thanks, applied.

Miklos

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11 18:37 [PATCH][V3] overlayfs: During copy up, first copy up data and then xattrs Vivek Goyal
2019-01-15  9:26 ` Miklos Szeredi

Stable Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://lore.kernel.org/stable \
		stable@vger.kernel.org stable@archiver.kernel.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox