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

[Kevin Wolf <kwolf@redhat.com>]

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.

> +
> +    if (to_set & FUSE_SET_ATTR_UID) {
> +        exp->st_uid = statbuf->st_uid;
> +    }
> +
> +    if (to_set & FUSE_SET_ATTR_GID) {
> +        exp->st_gid = statbuf->st_gid;
> +    }
> +
>      fuse_getattr(req, inode, fi);
>  }

Kevin