Re: [PATCH 3/4] export/fuse: Let permissions be adjustable

[Kevin Wolf <kwolf@redhat.com>]

Am 22.06.2021 um 17:22 hat Max Reitz geschrieben:
> On 22.06.21 17:02, Kevin Wolf wrote:
> > Am 14.06.2021 um 16:44 hat Max Reitz geschrieben:
> > > Allow changing the file mode, UID, and GID through SETATTR.
> > > 
> > > This only really makes sense with allow-other, though (because without
> > > it, the effective access mode is fixed to be 0600 (u+rw) with qemu's
> > > user being the file's owner), so changing these stat fields is not
> > > allowed without allow-other.
> > > 
> > > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > > ---
> > >   block/export/fuse.c | 48 ++++++++++++++++++++++++++++++++++-----------
> > >   1 file changed, 37 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/block/export/fuse.c b/block/export/fuse.c
> > > index 1d54286d90..742e0af657 100644
> > > --- a/block/export/fuse.c
> > > +++ b/block/export/fuse.c
> > > @@ -47,6 +47,10 @@ typedef struct FuseExport {
> > >       bool writable;
> > >       bool growable;
> > >       bool allow_other;
> > > +
> > > +    mode_t st_mode;
> > > +    uid_t st_uid;
> > > +    gid_t st_gid;
> > >   } FuseExport;
> > >   static GHashTable *exports;
> > > @@ -120,6 +124,13 @@ static int fuse_export_create(BlockExport *blk_exp,
> > >       exp->growable = args->growable;
> > >       exp->allow_other = args->allow_other;
> > > +    exp->st_mode = S_IFREG | S_IRUSR;
> > > +    if (exp->writable) {
> > > +        exp->st_mode |= S_IWUSR;
> > > +    }
> > > +    exp->st_uid = getuid();
> > > +    exp->st_gid = getgid();
> > > +
> > >       ret = setup_fuse_export(exp, args->mountpoint, errp);
> > >       if (ret < 0) {
> > >           goto fail;
> > > @@ -329,7 +340,6 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
> > >       int64_t length, allocated_blocks;
> > >       time_t now = time(NULL);
> > >       FuseExport *exp = fuse_req_userdata(req);
> > > -    mode_t mode;
> > >       length = blk_getlength(exp->common.blk);
> > >       if (length < 0) {
> > > @@ -344,17 +354,12 @@ static void fuse_getattr(fuse_req_t req, fuse_ino_t inode,
> > >           allocated_blocks = DIV_ROUND_UP(allocated_blocks, 512);
> > >       }
> > > -    mode = S_IFREG | S_IRUSR;
> > > -    if (exp->writable) {
> > > -        mode |= S_IWUSR;
> > > -    }
> > > -
> > >       statbuf = (struct stat) {
> > >           .st_ino     = inode,
> > > -        .st_mode    = mode,
> > > +        .st_mode    = exp->st_mode,
> > >           .st_nlink   = 1,
> > > -        .st_uid     = getuid(),
> > > -        .st_gid     = getgid(),
> > > +        .st_uid     = exp->st_uid,
> > > +        .st_gid     = exp->st_gid,
> > >           .st_size    = length,
> > >           .st_blksize = blk_bs(exp->common.blk)->bl.request_alignment,
> > >           .st_blocks  = allocated_blocks,
> > > @@ -400,15 +405,23 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size,
> > >   }
> > >   /**
> > > - * Let clients set file attributes.  Only resizing is supported.
> > > + * Let clients set file attributes.  With allow_other, only resizing and
> > > + * changing permissions (st_mode, st_uid, st_gid) is allowed.  Without
> > > + * allow_other, only resizing is supported.
> > >    */
> > >   static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
> > >                            int to_set, struct fuse_file_info *fi)
> > >   {
> > >       FuseExport *exp = fuse_req_userdata(req);
> > > +    int supported_attrs;
> > >       int ret;
> > > -    if (to_set & ~FUSE_SET_ATTR_SIZE) {
> > > +    supported_attrs = FUSE_SET_ATTR_SIZE;
> > > +    if (exp->allow_other) {
> > > +        supported_attrs |= FUSE_SET_ATTR_MODE | FUSE_SET_ATTR_UID |
> > > +            FUSE_SET_ATTR_GID;
> > > +    }
> > > +    if (to_set & ~supported_attrs) {
> > >           fuse_reply_err(req, ENOTSUP);
> > >           return;
> > >       }
> > > @@ -426,6 +439,19 @@ static void fuse_setattr(fuse_req_t req, fuse_ino_t inode, struct stat *statbuf,
> > >           }
> > >       }
> > > +    if (to_set & FUSE_SET_ATTR_MODE) {
> > > +        /* Only allow changing the file mode, not the type */
> > > +        exp->st_mode = (statbuf->st_mode & 07777) | S_IFREG;
> > > +    }
> > Should we check that the mode actually makes sense? Not sure if making
> > an image executable has a good use case, and making it writable in the
> > permissions for a read-only export isn't a good idea either.
> 
> I mean, I don’t mind what the user does.  It doesn’t really faze us, I
> believe.  If the image contains an executable ELF and the user wants to run
> it directly from FUSE...  I don’t mind.
> 
> As for +w on RO exports, I’m not sure.  This reminds me of `sudo chattr +i
> $file`, which effectively makes any regular file read-only, too, and it can
> still keep +w.  So the file permissions are basically just ACLs, getting
> permission for something doesn’t mean you can actually do it.
> 
> OTOH, the difference to `chattr +i` is that we’d allow opening the export
> R/W, only writing would then fail.  `chattr +i` does give EPERM when opening
> the file.
> 
> So I’m not quite sure.  I don’t really want to prevent the user from setting
> any access restrictions they want, but on the other hand, if writing can
> never work, then there really is no point in allowing +w.  (Then I’m
> wondering, if we don’t allow +w, should we silently drop it or return an
> error?  I guess returning success but not actually succeeding is weird, so
> we probably should return EROFS.)

Yes, EROFS seems best.

> But +x can technically work, so I wouldn’t disallow it.

Fair enough.

Kevin