diff mbox series

[v2,4/6] fuse: Kill suid/sgid using ATTR_MODE if it is not truncate

Message ID 20200916161737.38028-5-vgoyal@redhat.com
State New
Headers show
Series fuse: Implement FUSE_HANDLE_KILLPRIV_V2 and enable SB_NOSEC | expand

Commit Message

Vivek Goyal Sept. 16, 2020, 4:17 p.m. UTC
If a truncate is happening with ->handle_killpriv_v2 is enabled, then
we don't have to send ATTR_MODE to kill suid/sgid as server will
kill it as part of the protocol.

But if this is non-truncate setattr then server will not kill suid/sgid.
So continue to send ATTR_MODE to kill suid/sgid for non-truncate setattr,
even if ->handle_killpriv_v2 is enabled.

This path is taken when client does a write on a file which has suid/
sgid is set. VFS will first kill suid/sgid and then proceed with WRITE.

One can argue that why not simply ignore ATTR_MODE because a WRITE
will follow and ->handle_killpriv_v2 will kill suid/sgid that time.
I feel this is a safer approach for following reasons.

- With ->writeback_cache enabled, WRITE will not go to server. I feel
  that for this reason ->writeback_cache mode is not fully compatible
  with ->handle_killpriv_v2. But if we kill suid/sgid now, this will
  solve this particular issue for ->writeback_cache mode too.

  Again, I will not solve all the issues around ->writeback_cache but
  makes things better.

- If we rely on WRITE killing suid/sgid, then after cache becomes
  out of sync w.r.t host. Client will still have suid/sgid set but
  subsequent WRITE will clear suid/sgid. Well WRITE will also invalidate
  client cache so further access to inode->i_mode should result in
  a ->getattr. Hmm..., for the case of ->writeback_cache, I am
  kind of inclined to send ATTR_MODE.

- We are sending setattr(ATTR_FORCE) anyway (even if we clear ATTR_MODE).
  So if we are not saving on setattr(), why not kill suid/sgid now.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/dir.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Miklos Szeredi Sept. 22, 2020, 1:56 p.m. UTC | #1
On Wed, Sep 16, 2020 at 6:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:

> But if this is non-truncate setattr then server will not kill suid/sgid.
> So continue to send ATTR_MODE to kill suid/sgid for non-truncate setattr,
> even if ->handle_killpriv_v2 is enabled.

Sending ATTR_MODE doesn't make sense, since that is racy.   The
refresh-recalculate makes the race window narrower, but it doesn't
eliminate it.

I think I suggested sending write synchronously if suid/sgid/caps are
set.  Do you see a problem with this?

Does this affect anything other than cached writes?

Thanks,
Miklos
Vivek Goyal Sept. 22, 2020, 8:08 p.m. UTC | #2
On Tue, Sep 22, 2020 at 03:56:47PM +0200, Miklos Szeredi wrote:
> On Wed, Sep 16, 2020 at 6:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > But if this is non-truncate setattr then server will not kill suid/sgid.
> > So continue to send ATTR_MODE to kill suid/sgid for non-truncate setattr,
> > even if ->handle_killpriv_v2 is enabled.
> 
> Sending ATTR_MODE doesn't make sense, since that is racy.   The
> refresh-recalculate makes the race window narrower, but it doesn't
> eliminate it.

Hi Miklos,

Agreed that it does not eliminate that race.

> 
> I think I suggested sending write synchronously if suid/sgid/caps are
> set.  Do you see a problem with this?

Sorry, I might have missed it. So you are saying that for the case of
->writeback_cache, force a synchronous WRITE if suid/sgid is set. But
this will only work if client sees the suid/sgid bits. If client B
set the suid/sgid which client A does not see then all the WRITEs
will be cached in client A and not clear suid/sgid bits.

Also another problem is that if client sees suid/sgid and we make
WRITE synchronous, client's suid/sgid attrs are still cached till
next refresh (both for ->writeback_cache and non writeback_cache
case). So server is clearing suid/sgid bits but client still
keeps them cached. I hope none of the code paths end up using
this stale value and refresh attrs before using suid/sgid.

Shall we refresh attrs after WRITE if suid/sgid is set and client
expects it to clear after WRITE finishes to solve this problem. Or
this is something which is actually not a real problem and I am
overdesigning.

Thanks
Vivek

> 
> Does this affect anything other than cached writes?
> 
> Thanks,
> Miklos
>
Miklos Szeredi Sept. 22, 2020, 9:25 p.m. UTC | #3
On Tue, Sep 22, 2020 at 10:08 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Sep 22, 2020 at 03:56:47PM +0200, Miklos Szeredi wrote:
> > On Wed, Sep 16, 2020 at 6:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > > But if this is non-truncate setattr then server will not kill suid/sgid.
> > > So continue to send ATTR_MODE to kill suid/sgid for non-truncate setattr,
> > > even if ->handle_killpriv_v2 is enabled.
> >
> > Sending ATTR_MODE doesn't make sense, since that is racy.   The
> > refresh-recalculate makes the race window narrower, but it doesn't
> > eliminate it.
>
> Hi Miklos,
>
> Agreed that it does not eliminate that race.
>
> >
> > I think I suggested sending write synchronously if suid/sgid/caps are
> > set.  Do you see a problem with this?
>
> Sorry, I might have missed it. So you are saying that for the case of
> ->writeback_cache, force a synchronous WRITE if suid/sgid is set. But
> this will only work if client sees the suid/sgid bits. If client B
> set the suid/sgid which client A does not see then all the WRITEs
> will be cached in client A and not clear suid/sgid bits.

Unless the attributes are invalidated (either by timeout or
explicitly) there's no way that in that situation the suid/sgid bits
can be cleared.  That's true of your patch as well.

>
> Also another problem is that if client sees suid/sgid and we make
> WRITE synchronous, client's suid/sgid attrs are still cached till
> next refresh (both for ->writeback_cache and non writeback_cache
> case). So server is clearing suid/sgid bits but client still
> keeps them cached. I hope none of the code paths end up using
> this stale value and refresh attrs before using suid/sgid.
>
> Shall we refresh attrs after WRITE if suid/sgid is set and client
> expects it to clear after WRITE finishes to solve this problem. Or
> this is something which is actually not a real problem and I am
> overdesigning.

The fuse_perform_write() path already has the attribute invalidation,
which will trigger GETATTR from fuse_update_attributes() in the next
write.

So I think all that should work fine.

Thanks,
Miklos
Vivek Goyal Sept. 22, 2020, 9:31 p.m. UTC | #4
On Tue, Sep 22, 2020 at 11:25:30PM +0200, Miklos Szeredi wrote:
> On Tue, Sep 22, 2020 at 10:08 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Tue, Sep 22, 2020 at 03:56:47PM +0200, Miklos Szeredi wrote:
> > > On Wed, Sep 16, 2020 at 6:18 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > > But if this is non-truncate setattr then server will not kill suid/sgid.
> > > > So continue to send ATTR_MODE to kill suid/sgid for non-truncate setattr,
> > > > even if ->handle_killpriv_v2 is enabled.
> > >
> > > Sending ATTR_MODE doesn't make sense, since that is racy.   The
> > > refresh-recalculate makes the race window narrower, but it doesn't
> > > eliminate it.
> >
> > Hi Miklos,
> >
> > Agreed that it does not eliminate that race.
> >
> > >
> > > I think I suggested sending write synchronously if suid/sgid/caps are
> > > set.  Do you see a problem with this?
> >
> > Sorry, I might have missed it. So you are saying that for the case of
> > ->writeback_cache, force a synchronous WRITE if suid/sgid is set. But
> > this will only work if client sees the suid/sgid bits. If client B
> > set the suid/sgid which client A does not see then all the WRITEs
> > will be cached in client A and not clear suid/sgid bits.
> 
> Unless the attributes are invalidated (either by timeout or
> explicitly) there's no way that in that situation the suid/sgid bits
> can be cleared.  That's true of your patch as well.

Right. And that's why I mentioned that handle_killpriv_v2 is not fully
compatible with ->writeback_cache.

> 
> >
> > Also another problem is that if client sees suid/sgid and we make
> > WRITE synchronous, client's suid/sgid attrs are still cached till
> > next refresh (both for ->writeback_cache and non writeback_cache
> > case). So server is clearing suid/sgid bits but client still
> > keeps them cached. I hope none of the code paths end up using
> > this stale value and refresh attrs before using suid/sgid.
> >
> > Shall we refresh attrs after WRITE if suid/sgid is set and client
> > expects it to clear after WRITE finishes to solve this problem. Or
> > this is something which is actually not a real problem and I am
> > overdesigning.
> 
> The fuse_perform_write() path already has the attribute invalidation,
> which will trigger GETATTR from fuse_update_attributes() in the next
> write.

Ok. So if there is any path which potentially can make use of cached
suid/sgid, we just need to make sure fuse_update_attributes() has been
called in that path.

> 
> So I think all that should work fine.

Sounds good. I will give it a try and see if I notice any other issues.

Thanks
Vivek
diff mbox series

Patch

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index ecdb7895c156..4b0fe0828e36 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1655,6 +1655,21 @@  static int fuse_setattr(struct dentry *entry, struct iattr *attr)
 		return -EACCES;
 
 	if (attr->ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID)) {
+		bool kill_sugid = true;
+		bool is_truncate = !!(attr->ia_valid & ATTR_SIZE);
+
+		if (fc->handle_killpriv ||
+		    (fc->handle_killpriv_v2 && is_truncate)) {
+			/*
+			 * If this is truncate and ->handle_killpriv_v2 is
+			 * enabled, we don't have to send ATTR_MODE to
+			 * kill suid/sgid as server will do it anyway as
+			 * part of truncate. But if this is not truncate
+			 * then kill suid/sgid by sending ATTR_MODE.
+			 */
+			kill_sugid = false;
+		}
+
 		attr->ia_valid &= ~(ATTR_KILL_SUID | ATTR_KILL_SGID |
 				    ATTR_MODE);
 
@@ -1664,7 +1679,7 @@  static int fuse_setattr(struct dentry *entry, struct iattr *attr)
 		 *
 		 * This should be done on write(), truncate() and chown().
 		 */
-		if (!fc->handle_killpriv) {
+		if (kill_sugid) {
 			/*
 			 * ia_mode calculation may have used stale i_mode.
 			 * Refresh and recalculate.