ceph: copy_file_range needs to strip setuid bits and update timestamps
Subsystems (2)
PATCH 1/1 ceph: copy_file_range needs to strip setuid bits and update timestamps
Because ceph doesn't hold destination inode lock throughout the copy, strip setuid bits before and after copy.
The destination inode mtime is updated before and after the copy and the source inode atime is updated after the copy, similar to the filesystem ->read_iter() implementation.
---
Hi Ilya,
Please consider applying this patch to ceph branch after merging Darrick's copy-file-range-fixes branch from:
git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git
The series (including this patch) was tested on ceph by Luis Henriques using new copy_range xfstests.
AFAIK, only fallback from ceph to generic_copy_file_range() implementation was tested and not the actual ceph clustered copy_file_range.
Thanks,
Amir.
fs/ceph/file.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
fs/ceph/file.c +17 -0
@@ -1949,6 +1949,15 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, goto out; } +/* Should dst_inode lock be held throughout the copy operation? */ +inode_lock (dst_inode); + ret =file_modified (dst_file); +inode_unlock (dst_inode); +if (ret <0 ) { +dout ("failed to modify dst file before copy (%zd)\n" , ret); + goto out; + } +/* * We need FILE_WR caps for dst_ci and FILE_RD for src_ci as other * clients may have dirty data in their caches. And OSDs know nothing
@@ -2099,6 +2108,14 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, out:ceph_free_cap_flush (prealloc_cf); +file_accessed (src_file); +/* To be on the safe side, try to remove privs also after copy */ +inode_lock (dst_inode); + err =file_modified (dst_file); +inode_unlock (dst_inode); +if (err <0 ) +dout ("failed to modify dst file after copy (%d)\n" , err); +return ret; }
Signature · 1 line
2.17.1
Ilya Dryomov
maintainerQuoted 17 lines · Amir Goldstein
Because ceph doesn't hold destination inode lock throughout the copy, strip setuid bits before and after copy.
The destination inode mtime is updated before and after the copy and the source inode atime is updated after the copy, similar to the filesystem ->read_iter() implementation.
Signed-off-by Amir Goldstein <[email protected]>---
Hi Ilya,
Please consider applying this patch to ceph branch after merging Darrick's copy-file-range-fixes branch from:
git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git
Quoted 6 lines
The series (including this patch) was tested on ceph by Luis Henriques using new
copy_rangexfstests.AFAIK, only fallback from ceph to
generic_copy_file_range()implementation was tested and not the actual ceph clusteredcopy_file_range.
Zheng, Jeff, please take a look.
Thanks,
Ilya
Quoted 16 lines · Amir Goldstein
Because ceph doesn't hold destination inode lock throughout the copy, strip setuid bits before and after copy.
The destination inode mtime is updated before and after the copy and the source inode atime is updated after the copy, similar to the filesystem ->read_iter() implementation.
Signed-off-by Amir Goldstein <[email protected]>---
Hi Ilya,
Please consider applying this patch to ceph branch after merging Darrick's copy-file-range-fixes branch from:
git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git
Quoted 6 lines
The series (including this patch) was tested on ceph by Luis Henriques using new
copy_rangexfstests.AFAIK, only fallback from ceph to
generic_copy_file_range()implementation was tested and not the actual ceph clusteredcopy_file_range.
JFYI I've also run tests that exercise the ceph implementation, i.e. that actually do the copy offload. It's the xfstests that (AFAIR) only exercise the generic VFS copy_file_range as they never meet the requirements for this copy offload to happen (for example, the copy must be at least the same length as the files object size which is 4M by default).
Cheers,
Signature · 1 line
Luis
Quoted 41 lines
Thanks,
Amir.fs/ceph/file.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)fs/ceph/file.c +17
-0@@ -1949,6 +1949,15 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, goto out; } +/* Should dst_inode lock be held throughout the copy operation? */ +inode_lock (dst_inode); + ret =file_modified (dst_file); +inode_unlock (dst_inode); +if (ret <0 ) { +dout ("failed to modify dst file before copy (%zd)\n" , ret); + goto out; + } +/* * We need FILE_WR caps for dst_ci and FILE_RD for src_ci as other * clients may have dirty data in their caches. And OSDs know nothing @@ -2099,6 +2108,14 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, out:ceph_free_cap_flush (prealloc_cf); +file_accessed (src_file); +/* To be on the safe side, try to remove privs also after copy */ +inode_lock (dst_inode); + err =file_modified (dst_file); +inode_unlock (dst_inode); +if (err <0 ) +dout ("failed to modify dst file after copy (%d)\n" , err); +return ret; }
Quoted 36 lines · Amir Goldstein
Because ceph doesn't hold destination inode lock throughout the copy, strip setuid bits before and after copy.
The destination inode mtime is updated before and after the copy and the source inode atime is updated after the copy, similar to the filesystem ->read_iter() implementation.
Signed-off-by Amir Goldstein <[email protected]>---
Hi Ilya,
Please consider applying this patch to ceph branch after merging Darrick's copy-file-range-fixes branch from:
git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.gitThe series (including this patch) was tested on ceph by Luis Henriques using new
copy_rangexfstests.AFAIK, only fallback from ceph to
generic_copy_file_range()implementation was tested and not the actual ceph clusteredcopy_file_range.Thanks,
Amir.fs/ceph/file.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)fs/ceph/file.c +0
-0@@ -1949,6 +1949,15 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, goto out; }
fs/ceph/file.c static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, · line 1960
Quoted 10 lines
fs/ceph/file.c +9
-0+/* Should dst_inode lock be held throughout the copy operation? */ +inode_lock (dst_inode); + ret =file_modified (dst_file); +inode_unlock (dst_inode); +if (ret <0 ) { +dout ("failed to modify dst file before copy (%zd)\n" , ret); + goto out; + } +
I don't see anything that guarantees that the mode of the destination file is up to date at this point. file_modified() just ends up checking the mode cached in the inode.
I wonder if we ought to fix get_rd_wr_caps() to also acquire a reference to AUTH_SHARED caps on the destination inode, and then call file_modified() after we get those caps. That would also mean that we wouldn't need to do this a second time after the copy.
The catch is that if we did need to issue a setattr, I'm not sure if we'd need to release those caps first.
Luis, Zheng, thoughts?
Quoted 18 lines
fs/ceph/file.c +8
-0/* * We need FILE_WR caps for dst_ci and FILE_RD for src_ci as other * clients may have dirty data in their caches. And OSDs know nothing @@ -2099,6 +2108,14 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, out:ceph_free_cap_flush (prealloc_cf); +file_accessed (src_file); +/* To be on the safe side, try to remove privs also after copy */ +inode_lock (dst_inode); + err =file_modified (dst_file); +inode_unlock (dst_inode); +if (err <0 ) +dout ("failed to modify dst file after copy (%d)\n" , err); +return ret; }
Quoted 57 lines · Jeff Layton
Quoted 36 lines · Amir Goldstein
Because ceph doesn't hold destination inode lock throughout the copy, strip setuid bits before and after copy.
The destination inode mtime is updated before and after the copy and the source inode atime is updated after the copy, similar to the filesystem ->read_iter() implementation.
Signed-off-by Amir Goldstein <[email protected]>---
Hi Ilya,
Please consider applying this patch to ceph branch after merging Darrick's copy-file-range-fixes branch from:
git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.gitThe series (including this patch) was tested on ceph by Luis Henriques using new
copy_rangexfstests.AFAIK, only fallback from ceph to
generic_copy_file_range()implementation was tested and not the actual ceph clusteredcopy_file_range.Thanks,
Amir.fs/ceph/file.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)fs/ceph/file.c +0
-0@@ -1949,6 +1949,15 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, goto out; }fs/ceph/file.c static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, · line 1960
Quoted 10 lines
fs/ceph/file.c +9
-0+/* Should dst_inode lock be held throughout the copy operation? */ +inode_lock (dst_inode); + ret =file_modified (dst_file); +inode_unlock (dst_inode); +if (ret <0 ) { +dout ("failed to modify dst file before copy (%zd)\n" , ret); + goto out; + } +I don't see anything that guarantees that the mode of the destination file is up to date at this point.
file_modified()just ends up checking the mode cached in the inode.I wonder if we ought to fix
get_rd_wr_caps()to also acquire a reference toAUTH_SHAREDcaps on the destination inode, and then callfile_modified()after we get those caps. That would also mean that we wouldn't need to do this a second time after the copy.
Quoted 4 lines
The catch is that if we did need to issue a setattr, I'm not sure if we'd need to release those caps first.
Luis, Zheng, thoughts?
Hmm... I missed that. IIRC the FILE_WR caps allow to modify some metadata (such as timestamps, and file size). I suppose it doesn't allow to cache the mode, does it? If it does, fixing it would be a matter of moving the code a bit further down. If it doesn't the ceph_copy_file_range function already has this problem, as it calls file_update_time. And I wonder if other code paths have this problem too.
Obviously, the chunk below will have the same problem.
Cheers,
Signature · 1 line
Luis
Quoted 22 lines
Quoted 19 lines
fs/ceph/file.c +8
-0/* * We need FILE_WR caps for dst_ci and FILE_RD for src_ci as other * clients may have dirty data in their caches. And OSDs know nothing @@ -2099,6 +2108,14 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, out:ceph_free_cap_flush (prealloc_cf); +file_accessed (src_file); +/* To be on the safe side, try to remove privs also after copy */ +inode_lock (dst_inode); + err =file_modified (dst_file); +inode_unlock (dst_inode); +if (err <0 ) +dout ("failed to modify dst file after copy (%d)\n" , err); +return ret; }
Quoted 59 lines · Luis Henriques
Quoted 57 lines · Jeff Layton
Quoted 36 lines · Amir Goldstein
Because ceph doesn't hold destination inode lock throughout the copy, strip setuid bits before and after copy.
The destination inode mtime is updated before and after the copy and the source inode atime is updated after the copy, similar to the filesystem ->read_iter() implementation.
Signed-off-by Amir Goldstein <[email protected]>---
Hi Ilya,
Please consider applying this patch to ceph branch after merging Darrick's copy-file-range-fixes branch from:
git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.gitThe series (including this patch) was tested on ceph by Luis Henriques using new
copy_rangexfstests.AFAIK, only fallback from ceph to
generic_copy_file_range()implementation was tested and not the actual ceph clusteredcopy_file_range.Thanks,
Amir.fs/ceph/file.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)fs/ceph/file.c +0
-0@@ -1949,6 +1949,15 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, goto out; }fs/ceph/file.c static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, · line 1960
Quoted 10 lines
fs/ceph/file.c +9
-0+/* Should dst_inode lock be held throughout the copy operation? */ +inode_lock (dst_inode); + ret =file_modified (dst_file); +inode_unlock (dst_inode); +if (ret <0 ) { +dout ("failed to modify dst file before copy (%zd)\n" , ret); + goto out; + } +I don't see anything that guarantees that the mode of the destination file is up to date at this point.
file_modified()just ends up checking the mode cached in the inode.I wonder if we ought to fix
get_rd_wr_caps()to also acquire a reference toAUTH_SHAREDcaps on the destination inode, and then callfile_modified()after we get those caps. That would also mean that we wouldn't need to do this a second time after the copy.
Quoted 8 lines
Quoted 4 lines
The catch is that if we did need to issue a setattr, I'm not sure if we'd need to release those caps first.
Luis, Zheng, thoughts?
Hmm... I missed that. IIRC the
FILE_WRcaps allow to modify some metadata (such as timestamps, and file size). I suppose it doesn't allow to cache the mode, does it?
No, W caps don't guarantee that the mode won't change. You need As or Ax caps for that.
Quoted 6 lines
If it does, fixing it would be a
matter of moving the code a bit further down. If it doesn't theceph_copy_file_rangefunction already has this problem, as it callsfile_update_time. And I wonder if other code paths have this problem too.
I think you mean file_remove_privs, but yes...the write codepath has a similar problem. file_remove_privs is called before acquiring any caps, so the same thing could happen there too.
It'd be good to fix both places, but taking As cap references in the write codepath could have performance impact in some cases. OTOH, they don't change that much, so maybe that's OK.
Quoted 2 lines
Obviously, the chunk below will have the same problem.
Right. If however, we have this code take an As cap reference before doing the copy, then we can be sure that the mode can't change until we drop them. That way we wouldn't need the second call.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 87 lines · Jeff Layton
Quoted 59 lines · Luis Henriques
Quoted 57 lines · Jeff Layton
Quoted 36 lines · Amir Goldstein
Because ceph doesn't hold destination inode lock throughout the copy, strip setuid bits before and after copy.
The destination inode mtime is updated before and after the copy and the source inode atime is updated after the copy, similar to the filesystem ->read_iter() implementation.
Signed-off-by Amir Goldstein <[email protected]>---
Hi Ilya,
Please consider applying this patch to ceph branch after merging Darrick's copy-file-range-fixes branch from:
git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.gitThe series (including this patch) was tested on ceph by Luis Henriques using new
copy_rangexfstests.AFAIK, only fallback from ceph to
generic_copy_file_range()implementation was tested and not the actual ceph clusteredcopy_file_range.Thanks,
Amir.fs/ceph/file.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)fs/ceph/file.c +0
-0@@ -1949,6 +1949,15 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, goto out; }fs/ceph/file.c static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, · line 1960
Quoted 10 lines
fs/ceph/file.c +9
-0+/* Should dst_inode lock be held throughout the copy operation? */ +inode_lock (dst_inode); + ret =file_modified (dst_file); +inode_unlock (dst_inode); +if (ret <0 ) { +dout ("failed to modify dst file before copy (%zd)\n" , ret); + goto out; + } +I don't see anything that guarantees that the mode of the destination file is up to date at this point.
file_modified()just ends up checking the mode cached in the inode.I wonder if we ought to fix
get_rd_wr_caps()to also acquire a reference toAUTH_SHAREDcaps on the destination inode, and then callfile_modified()after we get those caps. That would also mean that we wouldn't need to do this a second time after the copy.Quoted 8 lines
Quoted 4 lines
The catch is that if we did need to issue a setattr, I'm not sure if we'd need to release those caps first.
Luis, Zheng, thoughts?
Hmm... I missed that. IIRC the
FILE_WRcaps allow to modify some metadata (such as timestamps, and file size). I suppose it doesn't allow to cache the mode, does it?No, W caps don't guarantee that the mode won't change. You need As or Ax caps for that.
Quoted 6 lines
If it does, fixing it would be a
matter of moving the code a bit further down. If it doesn't theceph_copy_file_rangefunction already has this problem, as it callsfile_update_time. And I wonder if other code paths have this problem too.I think you mean
file_remove_privs, but yes...the write codepath has a similar problem.file_remove_privsis called before acquiring any caps, so the same thing could happen there too.It'd be good to fix both places, but taking As cap references in the write codepath could have performance impact in some cases. OTOH, they don't change that much, so maybe that's OK.
Quoted 6 lines
Quoted 2 lines
Obviously, the chunk below will have the same problem.
Right. If however, we have this code take an As cap reference before doing the copy, then we can be sure that the mode can't change until we drop them. That way we wouldn't need the second call.
So, do you think the patch below would be enough? It's totally untested, but I wanted to know if that would be acceptable before running some tests on it.
Cheers,
Signature · 1 line
Luis
fs/ceph/file.c +15 -0
@@ -1949,6 +1949,21 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, goto out; } + ret =ceph_do_getattr (dst_inode,CEPH_CAP_AUTH_SHARED , false); +if (ret <0 ) { +dout ("failed to get auth caps on dst file (%zd)\n" , ret); + goto out; + } + +/* Should dst_inode lock be held throughout the copy operation? */ +inode_lock (dst_inode); + ret =file_modified (dst_file); +inode_unlock (dst_inode); +if (ret <0 ) { +dout ("failed to modify dst file before copy (%zd)\n" , ret); + goto out; + } +/* * We need FILE_WR caps for dst_ci and FILE_RD for src_ci as other * clients may have dirty data in their caches. And OSDs know nothing
Quoted 13 lines · Luis Henriques
So, do you think the patch below would be enough? It's totally untested, but I wanted to know if that would be acceptable before running some tests on it.
Cheers,
--
Luisfs/ceph/file.c +0
-0
fs/ceph/file.c static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, · line 1957
Quoted 9 lines
fs/ceph/file.c +6
-0@@ -1949,6 +1949,21 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, goto out; } + ret =ceph_do_getattr (dst_inode,CEPH_CAP_AUTH_SHARED , false); +if (ret <0 ) { +dout ("failed to get auth caps on dst file (%zd)\n" , ret); + goto out; + } +
I think this is still racy. You could lose As caps before file_modified is called. IMO, this code should hold a reference to As caps until the c_f_r operation is complete.
That may get tricky however if you do need to issue a setattr to change the mode, as the MDS may try to recall As caps at that point. You won't be able to release them until you drop the reference, so will that deadlock? I'm not sure.
Quoted 12 lines
fs/ceph/file.c +9
-0+/* Should dst_inode lock be held throughout the copy operation? */ +inode_lock (dst_inode); + ret =file_modified (dst_file); +inode_unlock (dst_inode); +if (ret <0 ) { +dout ("failed to modify dst file before copy (%zd)\n" , ret); + goto out; + } +/* * We need FILE_WR caps for dst_ci and FILE_RD for src_ci as other * clients may have dirty data in their caches. And OSDs know nothing
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 28 lines · Jeff Layton
Quoted 13 lines · Luis Henriques
So, do you think the patch below would be enough? It's totally untested, but I wanted to know if that would be acceptable before running some tests on it.
Cheers,
--
Luisfs/ceph/file.c +0
-0fs/ceph/file.c static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, · line 1957
Quoted 9 lines
fs/ceph/file.c +6
-0@@ -1949,6 +1949,21 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, goto out; } + ret =ceph_do_getattr (dst_inode,CEPH_CAP_AUTH_SHARED , false); +if (ret <0 ) { +dout ("failed to get auth caps on dst file (%zd)\n" , ret); + goto out; + } +I think this is still racy. You could lose As caps before
file_modifiedis called. IMO, this code should hold a reference to As caps until thec_f_roperation is complete.
Quoted 5 lines
That may get tricky however if you do need to issue a setattr to change the mode, as the MDS may try to recall As caps at that point. You won't be able to release them until you drop the reference, so will that deadlock? I'm not sure.
That said...in many (most?) cases the client will already have As caps on the file from the permission check during open, and mode changes (and AUTH cap revokes) are relatively rare. So, the race I'm talking about probably almost never happens in practice.
But...privilege escalation attacks often involve setuid changes, so I think we really ought to be careful here.
In any case, if holding a reference to As is not feasible, then I we could take the original version of the patch, and maybe pair it with the getattr above. It's not ideal, but it's better than nothing.
Quoted 12 lines
Quoted 12 lines
fs/ceph/file.c +9
-0+/* Should dst_inode lock be held throughout the copy operation? */ +inode_lock (dst_inode); + ret =file_modified (dst_file); +inode_unlock (dst_inode); +if (ret <0 ) { +dout ("failed to modify dst file before copy (%zd)\n" , ret); + goto out; + } +/* * We need FILE_WR caps for dst_ci and FILE_RD for src_ci as other * clients may have dirty data in their caches. And OSDs know nothing