| Message ID | 20181203083416.28978-6-david@fromorbit.com |
|---|---|
| State | New |
| Headers | show |
| Series | fs: fixes for major copy_file_range() issues | expand |
On Mon, Dec 3, 2018 at 10:34 AM Dave Chinner <david@fromorbit.com> wrote: > > From: Dave Chinner <dchinner@redhat.com> > > Similar to FI_DEDUPERANGE, make copy_file_range() check that we have > write permissions to the destination inode. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- Looks good. Reviewed-by: Amir Goldstein <amir73il@gmail.com> Thanks, Amir.
On Mon, Dec 03, 2018 at 07:34:10PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Similar to FI_DEDUPERANGE, make copy_file_range() check that we have TLDR: No, it's not similar to FIDEDUPERANGE -- the use of inode_permission() in allow_file_dedupe() is to enable callers to dedupe into a file for which the caller has write permissions but opened the file O_RDONLY. [Please keep reading...] > write permissions to the destination inode. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > mm/filemap.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/mm/filemap.c b/mm/filemap.c > index 0a170425935b..876df5275514 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -3013,6 +3013,11 @@ int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > (file_out->f_flags & O_APPEND)) > return -EBADF; > > + /* may sure we really are allowed to write to the destination inode */ > + ret = inode_permission(inode_out, MAY_WRITE); What's the difference between security_file_permission and inode_permission, and when do we call them for a regular open-write-close sequence? Hmmm, let me take a look: It looks like we call inode_permission at open() time to make sure that the file permissions permit writes and the file isn't immutable. security_file_permission gets called at write() time to recheck with the security policy, but once a process has been granted a writable file descriptor, it retains that privilege until it closes the fd. In other words, we check at open time, not at operation time. I think. Nothing is ever that simple, so let's check behavior: So let's try opening a file for write, removing write permissions, then writing to the file: $ rm -rf xyz $ touch xyz $ ls -lad xyz -rw-rw-r-- 1 djwong djwong 0 Dec 3 09:28 xyz $ xfs_io xyz xfs_io> pwrite -S 0x58 0 4k wrote 4096/4096 bytes at offset 0 4 KiB, 1 ops; 0.0000 sec (130.208 MiB/sec and 33333.3333 ops/sec) xfs_io> [1]+ Stopped xfs_io xyz $ chmod a-w xyz $ sudo chown root:root xyz $ ls -lad xyz -r--r--r-- 1 root root 4096 Dec 3 09:28 xyz $ fg xfs_io xyz xfs_io> pwrite -S 0x58 0 8k wrote 8192/8192 bytes at offset 0 8 KiB, 2 ops; 0.0000 sec (558.036 MiB/sec and 142857.1429 ops/sec) xfs_io> Yep, we can write to an already-open file even if we change its ownership and take away write permission. How about the immutable flag? $ touch b $ xfs_io b xfs_io> pwrite -S 0x58 0 4k wrote 4096/4096 bytes at offset 0 4 KiB, 1 ops; 0.0000 sec (75.120 MiB/sec and 19230.7692 ops/sec) xfs_io> sync xfs_io> [1]+ Stopped xfs_io b $ sudo chown root:root b $ sudo chattr +i b $ ls -lad b ; lsattr b -rw-rw-r-- 1 root root 4096 Dec 3 09:51 b ----i-------------- b $ fg xfs_io b xfs_io> pwrite -S 0x58 0 6k wrote 6144/6144 bytes at offset 0 6 KiB, 2 ops; 0.0000 sec (102.796 MiB/sec and 35087.7193 ops/sec) xfs_io> sync xfs_io> Similarly, it looks like we can write to an already-open file even if we change its ownership and mark the file immutable. Both of these are a little unexpected; I would have thought at least that +i would have prevented the write. How about reflink? $ rm -rf a b $ xfs_io -f -c 'pwrite -S 0x58 0 64k' a wrote 65536/65536 bytes at offset 0 64 KiB, 16 ops; 0.0000 sec (1.744 GiB/sec and 457142.8571 ops/sec) $ xfs_io -f -c 'pwrite -S 0x58 0 64k' b wrote 65536/65536 bytes at offset 0 64 KiB, 16 ops; 0.0000 sec (1.969 GiB/sec and 516129.0323 ops/sec) $ xfs_io b xfs_io> [1]+ Stopped xfs_io b $ chmod a-w b $ sudo chown root:root b $ sudo chattr +i b $ fg xfs_io b xfs_io> reflink a 0 0 4k XFS_IOC_CLONE_RANGE: Operation not permitted xfs_io> [1]+ Stopped xfs_io b $ sudo chattr -i b $ fg xfs_io b xfs_io> reflink a 0 0 4k linked 4096/4096 bytes at offset 0 4 KiB, 1 ops; 0.0004 sec (7.988 MiB/sec and 2044.9898 ops/sec) xfs_io> We cannot reflink into a file that becomes immutable after we open it for write, but we can reflink into a file that loses its write permissions after we open it. What about dedupe? $ rm -rf a b $ xfs_io -f -c 'pwrite -S 0x58 0 64k' a wrote 65536/65536 bytes at offset 0 64 KiB, 16 ops; 0.0000 sec (1.795 GiB/sec and 470588.2353 ops/sec) $ xfs_io -f -c 'pwrite -S 0x58 0 64k' b wrote 65536/65536 bytes at offset 0 64 KiB, 16 ops; 0.0001 sec (512.295 MiB/sec and 131147.5410 ops/sec) $ chmod a-w b $ sudo chown root:root b $ sudo chattr +i b $ fg xfs_io b xfs_io> dedupe a 0 0 4k XFS_IOC_FILE_EXTENT_SAME: Operation not permitted xfs_io> [1]+ Stopped xfs_io b $ sudo chattr -i b $ fg xfs_io b xfs_io> dedupe a 0 0 4k deduped 4096/4096 bytes at offset 0 4 KiB, 1 ops; 0.0160 sec (249.688 KiB/sec and 62.4220 ops/sec) xfs_io> We also cannot dedupe into a file that becomes immutable after we open it for write, but we can dedupe into a file that loses its write permissions after we open it. Summarized: op: after +immutable? after chmod a-w? write yes yes clonerange no yes dedupe no yes newcopyrange no no My reaction: I don't think that writes should be allowed after an administrator marks a file immutable (but that's a separate issue) but I do think we should be consistent in allowing copying into a file that has lost its write permissions after we opened the file for write, like we do for write() and the remap ioct.... *OH* Now I remember what the FI_DEDUPERANGE inode_permission call is for! It's because dedupe tools want to be able to open a file readonly and have dedupe remap another file's identical blocks into the readonly file, provided that the process would have been able to open the file for writing had it asked. [Hugging hug hug huggy hugging hug of hug interface!!! :P] --D > + if (ret < 0) > + return ret; > + > /* Ensure offsets don't wrap. */ > if (pos_in + count < pos_in || pos_out + count < pos_out) > return -EOVERFLOW; > -- > 2.19.1 >
On Mon, Dec 03, 2018 at 07:34:10PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Similar to FI_DEDUPERANGE, make copy_file_range() check that we have > write permissions to the destination inode. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > mm/filemap.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/mm/filemap.c b/mm/filemap.c > index 0a170425935b..876df5275514 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -3013,6 +3013,11 @@ int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > (file_out->f_flags & O_APPEND)) > return -EBADF; > > + /* may sure we really are allowed to write to the destination inode */ > + ret = inode_permission(inode_out, MAY_WRITE); > + if (ret < 0) > + return ret; > + > /* Ensure offsets don't wrap. */ > if (pos_in + count < pos_in || pos_out + count < pos_out) > return -EOVERFLOW; > -- > 2.19.1 > Why? The file descriptor was already checked for write permission above: if (!(file_in->f_mode & FMODE_READ) || !(file_out->f_mode & FMODE_WRITE) || (file_out->f_flags & O_APPEND)) return -EBADF; Yes, that doesn't detect removing write permission from the *inode*, but write() doesn't either. - Eric
On Mon, Dec 03, 2018 at 10:18:03AM -0800, Darrick J. Wong wrote: > On Mon, Dec 03, 2018 at 07:34:10PM +1100, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > Similar to FI_DEDUPERANGE, make copy_file_range() check that we have > > TLDR: No, it's not similar to FIDEDUPERANGE -- the use of > inode_permission() in allow_file_dedupe() is to enable callers to dedupe > into a file for which the caller has write permissions but opened the > file O_RDONLY. What a grotty, nasty hack. > [Please keep reading...] > > > write permissions to the destination inode. > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > --- > > mm/filemap.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > index 0a170425935b..876df5275514 100644 > > --- a/mm/filemap.c > > +++ b/mm/filemap.c > > @@ -3013,6 +3013,11 @@ int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > > (file_out->f_flags & O_APPEND)) > > return -EBADF; > > > > + /* may sure we really are allowed to write to the destination inode */ > > + ret = inode_permission(inode_out, MAY_WRITE); > > What's the difference between security_file_permission and > inode_permission, and when do we call them for a regular > open-write-close sequence? Hmmm, let me take a look: ..... > We also cannot dedupe into a file that becomes immutable after we open > it for write, but we can dedupe into a file that loses its write > permissions after we open it. It's more nuanced than that - dedupe will proceed after write permissions have been removed only if you are root or own the file, otherwise it will fail. Updated summary: > op: after +immutable? after chmod a-w? > write yes yes > clonerange no yes > dedupe no maybe > newcopyrange no no > > My reaction: I don't think that writes should be allowed after an > administrator marks a file immutable (but that's a separate issue) but I > do think we should be consistent in allowing copying into a file that > has lost its write permissions after we opened the file for write, like > we do for write() and the remap ioct.... If we want to allow copying to files we don't actually have permission to write to anymore, then I'll remove this from the test, the man page and the code. But, quite frankly, I don't trust remote server side copies to follow the same permission models as the client side OS, so I think we have to treat copy_file_range differently to a normal write syscall.... Cheers, Dave.
As Darrick already pointed our this looks wrong - for "normal" file operations, which copy defintively should be we should only allow write access on a writable fd.
On Tue, Dec 04, 2018 at 10:55:17AM +1100, Dave Chinner wrote: > On Mon, Dec 03, 2018 at 10:18:03AM -0800, Darrick J. Wong wrote: > > On Mon, Dec 03, 2018 at 07:34:10PM +1100, Dave Chinner wrote: > > > From: Dave Chinner <dchinner@redhat.com> > > > > > > Similar to FI_DEDUPERANGE, make copy_file_range() check that we have > > > > TLDR: No, it's not similar to FIDEDUPERANGE -- the use of > > inode_permission() in allow_file_dedupe() is to enable callers to dedupe > > into a file for which the caller has write permissions but opened the > > file O_RDONLY. > > What a grotty, nasty hack. > > > [Please keep reading...] > > > > > write permissions to the destination inode. > > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > > --- > > > mm/filemap.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > > index 0a170425935b..876df5275514 100644 > > > --- a/mm/filemap.c > > > +++ b/mm/filemap.c > > > @@ -3013,6 +3013,11 @@ int generic_copy_file_checks(struct file *file_in, loff_t pos_in, > > > (file_out->f_flags & O_APPEND)) > > > return -EBADF; > > > > > > + /* may sure we really are allowed to write to the destination inode */ > > > + ret = inode_permission(inode_out, MAY_WRITE); > > > > What's the difference between security_file_permission and > > inode_permission, and when do we call them for a regular > > open-write-close sequence? Hmmm, let me take a look: > ..... > > We also cannot dedupe into a file that becomes immutable after we open > > it for write, but we can dedupe into a file that loses its write > > permissions after we open it. > > It's more nuanced than that - dedupe will proceed after write > permissions have been removed only if you are root or own the file, > otherwise it will fail. > > Updated summary: > > > op: after +immutable? after chmod a-w? > > write yes yes > > clonerange no yes > > dedupe no maybe > > newcopyrange no no > > > > My reaction: I don't think that writes should be allowed after an > > administrator marks a file immutable (but that's a separate issue) but I > > do think we should be consistent in allowing copying into a file that > > has lost its write permissions after we opened the file for write, like > > we do for write() and the remap ioct.... > > If we want to allow copying to files we don't actually have > permission to write to anymore, then I'll remove this from the test, > the man page and the code. But, quite frankly, I don't trust remote > server side copies to follow the same permission models as the > client side OS, so I think we have to treat copy_file_range > differently to a normal write syscall.... The NFS COPY command takes references to the protocol's equivalent to open files, and I'd expect permission checks should depend on the open mode, not the current file permissions. But server behavior may vary. I'm not sure that's a good guide for what to do locally. In general I'm more comfortable the closer copy is to read & write. --b.
diff --git a/mm/filemap.c b/mm/filemap.c index 0a170425935b..876df5275514 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -3013,6 +3013,11 @@ int generic_copy_file_checks(struct file *file_in, loff_t pos_in, (file_out->f_flags & O_APPEND)) return -EBADF; + /* may sure we really are allowed to write to the destination inode */ + ret = inode_permission(inode_out, MAY_WRITE); + if (ret < 0) + return ret; + /* Ensure offsets don't wrap. */ if (pos_in + count < pos_in || pos_out + count < pos_out) return -EOVERFLOW;