Re: [PATCH] ceph: copy_file_range needs to strip setuid bits and update timestamps

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2019-06-14 at 07:43 -0400, Jeff Layton wrote:
> On Fri, 2019-06-14 at 09:52 +0100, Luis Henriques wrote:
> > 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,
> > --
> > Luis
> > 
> > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > index c5517ffeb11c..f6b0683dd8dc 100644
> > --- a/fs/ceph/file.c
> > +++ b/fs/ceph/file.c
> > @@ -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.
> 

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.


> > +       /* 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