On Fri, Jan 11, 2019 at 12:29 AM Vivek Goyal <vgoyal@xxxxxxxxxx> 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") > Reported-by: Giuseppe Scrivano <gscrivan@xxxxxxxxxx> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx> > --- > fs/overlayfs/copy_up.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > Index: rhvgoyal-linux/fs/overlayfs/copy_up.c > =================================================================== > --- rhvgoyal-linux.orig/fs/overlayfs/copy_up.c 2018-11-28 08:45:58.122478207 -0500 > +++ rhvgoyal-linux/fs/overlayfs/copy_up.c 2019-01-10 13:29:05.997079686 -0500 > @@ -443,6 +443,23 @@ 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); > + 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; > + } > + I know you just moved this code around, but please change it to if (WARN_ON(...)) return -EIO; If it was up to me, I would change the call arguments to: err = ovl_copy_up_data(c, temp); and compose the paths inside ovl_copy_up_data(). There are more similarities to both call sites than there are differences. Thanks, Amir.
