[PATCH] vfs: allow copy_file_range from a swapfile

[PATCH] vfs: allow copy_file_range from a swapfile

[Amir Goldstein]

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(-)

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

Re: [PATCH] vfs: allow copy_file_range from a swapfile

[Darrick J. Wong]

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
> 

Re: [PATCH] vfs: allow copy_file_range from a swapfile

[Theodore Ts'o]

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

Re: [PATCH] vfs: allow copy_file_range from a swapfile

[Darrick J. Wong]

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

Re: [PATCH] vfs: allow copy_file_range from a swapfile

[Christoph Hellwig]

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.

Re: [PATCH] vfs: allow copy_file_range from a swapfile

[Amir Goldstein]

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.