| Message ID | 20190610172606.4119-1-amir73il@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfs: allow copy_file_range from a swapfile | expand |
On Mon, Jun 10, 2019 at 08:26:06PM +0300, Amir Goldstein wrote: > read(2) is allowed from a swapfile, so copy_file_range(2) should > be allowed as well. > > Reported-by: Theodore Ts'o <tytso@mit.edu> > Fixes: 96e6e8f4a68d ("vfs: add missing checks to copy_file_range") > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > Darrick, > > This fixes the generic/554 issue reported by Ted. Frankly I think we should go the other way -- non-root doesn't get to copy from or read from swap files. --D > I intend to remove the test case of copy from swap file from > generic/554, so test is expected to pass with or without this fix. > But if you wait for next week's xfstests update before applying > this fix, then at lease maintainer that run current xfstests master > could use current copy-file-range-fixes branch to pass the tests. > > Thanks, > Amir. > > mm/filemap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/filemap.c b/mm/filemap.c > index aac71aef4c61..f74e5ce7ca50 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -3081,7 +3081,7 @@ int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > if (IS_IMMUTABLE(inode_out)) > return -EPERM; > > - if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out)) > + if (IS_SWAPFILE(inode_out)) > return -ETXTBSY; > > /* Ensure offsets don't wrap. */ > -- > 2.17.1 >
On Mon, Jun 10, 2019 at 06:16:12PM -0700, Darrick J. Wong wrote: > On Mon, Jun 10, 2019 at 08:26:06PM +0300, Amir Goldstein wrote: > > read(2) is allowed from a swapfile, so copy_file_range(2) should > > be allowed as well. > > > > Reported-by: Theodore Ts'o <tytso@mit.edu> > > Fixes: 96e6e8f4a68d ("vfs: add missing checks to copy_file_range") > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > > > Darrick, > > > > This fixes the generic/554 issue reported by Ted. > > Frankly I think we should go the other way -- non-root doesn't get to > copy from or read from swap files. The issue is that without this patch, *root* doesn't get to copy from swap files. Non-root shouldn't have access via Unix permissions. We could add a special case if we don't trust system administrators to be able to set the Unix permissions correctly, I suppose, but we don't do that for block devices when they are mounted.... - Ted
On Mon, Jun 10, 2019 at 10:51:08PM -0400, Theodore Ts'o wrote: > On Mon, Jun 10, 2019 at 06:16:12PM -0700, Darrick J. Wong wrote: > > On Mon, Jun 10, 2019 at 08:26:06PM +0300, Amir Goldstein wrote: > > > read(2) is allowed from a swapfile, so copy_file_range(2) should > > > be allowed as well. > > > > > > Reported-by: Theodore Ts'o <tytso@mit.edu> > > > Fixes: 96e6e8f4a68d ("vfs: add missing checks to copy_file_range") > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > > > > Darrick, > > > > > > This fixes the generic/554 issue reported by Ted. > > > > Frankly I think we should go the other way -- non-root doesn't get to > > copy from or read from swap files. > > The issue is that without this patch, *root* doesn't get to copy from > swap files. Non-root shouldn't have access via Unix permissions. We I'm not sure even root should have that privilege - it's a swap file, and until you swapoff, it's owned by the kernel and we shouldn't let backup programs copy your swapped out credit card numbers onto tape. > could add a special case if we don't trust system administrators to be > able to set the Unix permissions correctly, I suppose, but we don't do > that for block devices when they are mounted.... ...and administrators often mkfs over mounted filesystems because we let them read and write block devices. Granted I tried to fix that once and LVM totally stopped working... --D > > - Ted
On Mon, Jun 10, 2019 at 08:29:26PM -0700, Darrick J. Wong wrote: > ...and administrators often mkfs over mounted filesystems because we let > them read and write block devices. Granted I tried to fix that once and > LVM totally stopped working... That must have been a long time ago. We now overload O_EXCL on blockdevice to only allow a single exclusive openers, and mounted filesystems as well as raid drivers have been setting set for at leat 10 years.
On Tue, Jun 11, 2019 at 6:29 AM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Mon, Jun 10, 2019 at 10:51:08PM -0400, Theodore Ts'o wrote: > > On Mon, Jun 10, 2019 at 06:16:12PM -0700, Darrick J. Wong wrote: > > > On Mon, Jun 10, 2019 at 08:26:06PM +0300, Amir Goldstein wrote: > > > > read(2) is allowed from a swapfile, so copy_file_range(2) should > > > > be allowed as well. > > > > > > > > Reported-by: Theodore Ts'o <tytso@mit.edu> > > > > Fixes: 96e6e8f4a68d ("vfs: add missing checks to copy_file_range") > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > --- > > > > > > > > Darrick, > > > > > > > > This fixes the generic/554 issue reported by Ted. > > > > > > Frankly I think we should go the other way -- non-root doesn't get to > > > copy from or read from swap files. > > > > The issue is that without this patch, *root* doesn't get to copy from > > swap files. Non-root shouldn't have access via Unix permissions. We > > I'm not sure even root should have that privilege - it's a swap file, > and until you swapoff, it's owned by the kernel and we shouldn't let > backup programs copy your swapped out credit card numbers onto tape. > I am not a security expert and I do not want to be, but I believe it's better to have a complete security model before plugging random "security holes". That said. I don't have a strong feeling about allowing copy_file_range from swap file. If someone complains and they have a valid use case, we can always relax it then. Anyway, as you saw, I removed the test case from xfstest, leaving the behavior (as far as the testsuite cares) undefined. Thanks, Amir.
diff --git a/mm/filemap.c b/mm/filemap.c index aac71aef4c61..f74e5ce7ca50 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -3081,7 +3081,7 @@ int generic_copy_file_checks(struct file *file_in, loff_t pos_in, if (IS_IMMUTABLE(inode_out)) return -EPERM; - if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out)) + if (IS_SWAPFILE(inode_out)) return -ETXTBSY; /* Ensure offsets don't wrap. */
read(2) is allowed from a swapfile, so copy_file_range(2) should be allowed as well. Reported-by: Theodore Ts'o <tytso@mit.edu> Fixes: 96e6e8f4a68d ("vfs: add missing checks to copy_file_range") Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Darrick, This fixes the generic/554 issue reported by Ted. I intend to remove the test case of copy from swap file from generic/554, so test is expected to pass with or without this fix. But if you wait for next week's xfstests update before applying this fix, then at lease maintainer that run current xfstests master could use current copy-file-range-fixes branch to pass the tests. Thanks, Amir. mm/filemap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)