From e67f0216939c048f02fe58dc1113738380480061 Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Wed, 22 Apr 2020 09:08:49 -0400 Subject: ovl: clear ATTR_FILE from attr->ia_valid ovl_setattr() can be passed an attr which has ATTR_FILE set and attr->ia_file is a file pointer to overlay file. This is done in open(O_TRUNC) path. We should either replace with attr->ia_file with underlying file object or clear ATTR_FILE so that underlying filesystem does not end up using overlayfs file object pointer. There are no good use cases yet so for now clear ATTR_FILE. fuse seems to be one user which can use this. But it can work even without this. So it is not mandatory to pass ATTR_FILE to fuse. Signed-off-by: Vivek Goyal Fixes: bccece1ead36 ("ovl: allow remote upper") Signed-off-by: Miklos Szeredi --- fs/overlayfs/inode.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c index b0d42ece4d7c..34bfe0f912e1 100644 --- a/fs/overlayfs/inode.c +++ b/fs/overlayfs/inode.c @@ -58,6 +58,13 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr) if (attr->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID)) attr->ia_valid &= ~ATTR_MODE; + /* + * We might have to translate ovl file into underlying file + * object once some use cases are there. For now, simply don't + * let underlying filesystem rely on attr->ia_file + */ + attr->ia_valid &= ~ATTR_FILE; + inode_lock(upperdentry->d_inode); old_cred = ovl_override_creds(dentry->d_sb); err = notify_change(upperdentry, attr, NULL); -- cgit From 15fd2ea9f4f3d85fef787ba7db1b87939d0a2754 Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Wed, 22 Apr 2020 09:08:50 -0400 Subject: ovl: clear ATTR_OPEN from attr->ia_valid As of now during open(), we don't pass bunch of flags to underlying filesystem. O_TRUNC is one of these. Normally this is not a problem as VFS calls ->setattr() with zero size and underlying filesystem sets file size to 0. But when overlayfs is running on top of virtiofs, it has an optimization where it does not send setattr request to server if dectects that truncation is part of open(O_TRUNC). It assumes that server already zeroed file size as part of open(O_TRUNC). fuse_do_setattr() { if (attr->ia_valid & ATTR_OPEN) { /* * No need to send request to userspace, since actual * truncation has already been done by OPEN. But still * need to truncate page cache. */ } } IOW, fuse expects O_TRUNC to be passed to it as part of open flags. But currently overlayfs does not pass O_TRUNC to underlying filesystem hence fuse/virtiofs breaks. Setup overlayfs on top of virtiofs and following does not zero the file size of a file is either upper only or has already been copied up. fd = open(foo.txt, O_TRUNC | O_WRONLY); There are two ways to fix this. Either pass O_TRUNC to underlying filesystem or clear ATTR_OPEN from attr->ia_valid so that fuse ends up sending a SETATTR request to server. Miklos is concerned that O_TRUNC might have side affects so it is better to clear ATTR_OPEN for now. Hence this patch clears ATTR_OPEN from attr->ia_valid. I found this problem while running unionmount-testsuite. With this patch, unionmount-testsuite passes with overlayfs on top of virtiofs. Signed-off-by: Vivek Goyal Fixes: bccece1ead36 ("ovl: allow remote upper") Signed-off-by: Miklos Szeredi --- fs/overlayfs/inode.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c index 34bfe0f912e1..981f11ec51bc 100644 --- a/fs/overlayfs/inode.c +++ b/fs/overlayfs/inode.c @@ -59,12 +59,23 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr) attr->ia_valid &= ~ATTR_MODE; /* - * We might have to translate ovl file into underlying file - * object once some use cases are there. For now, simply don't - * let underlying filesystem rely on attr->ia_file + * We might have to translate ovl file into real file object + * once use cases emerge. For now, simply don't let underlying + * filesystem rely on attr->ia_file */ attr->ia_valid &= ~ATTR_FILE; + /* + * If open(O_TRUNC) is done, VFS calls ->setattr with ATTR_OPEN + * set. Overlayfs does not pass O_TRUNC flag to underlying + * filesystem during open -> do not pass ATTR_OPEN. This + * disables optimization in fuse which assumes open(O_TRUNC) + * already set file size to 0. But we never passed O_TRUNC to + * fuse. So by clearing ATTR_OPEN, fuse will be forced to send + * setattr request to server. + */ + attr->ia_valid &= ~ATTR_OPEN; + inode_lock(upperdentry->d_inode); old_cred = ovl_override_creds(dentry->d_sb); err = notify_change(upperdentry, attr, NULL); -- cgit From 9aafc1b0187322fa4fd4eb905d0903172237206c Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Tue, 5 May 2020 21:33:31 +0300 Subject: ovl: potential crash in ovl_fid_to_fh() The "buflen" value comes from the user and there is a potential that it could be zero. In do_handle_to_path() we know that "handle->handle_bytes" is non-zero and we do: handle_dwords = handle->handle_bytes >> 2; So values 1-3 become zero. Then in ovl_fh_to_dentry() we do: int len = fh_len << 2; So now len is in the "0,4-128" range and a multiple of 4. But if "buflen" is zero it will try to copy negative bytes when we do the memcpy in ovl_fid_to_fh(). memcpy(&fh->fb, fid, buflen - OVL_FH_WIRE_OFFSET); And that will lead to a crash. Thanks to Amir Goldstein for his help with this patch. Fixes: cbe7fba8edfc ("ovl: make sure that real fid is 32bit aligned in memory") Signed-off-by: Dan Carpenter Reviewed-by: Amir Goldstein Cc: # v5.5 Signed-off-by: Miklos Szeredi --- fs/overlayfs/export.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c index 475c61f53f0f..ed5c1078919c 100644 --- a/fs/overlayfs/export.c +++ b/fs/overlayfs/export.c @@ -783,6 +783,9 @@ static struct ovl_fh *ovl_fid_to_fh(struct fid *fid, int buflen, int fh_type) if (fh_type != OVL_FILEID_V0) return ERR_PTR(-EINVAL); + if (buflen <= OVL_FH_WIRE_OFFSET) + return ERR_PTR(-EINVAL); + fh = kzalloc(buflen, GFP_KERNEL); if (!fh) return ERR_PTR(-ENOMEM); -- cgit