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

From: Max Reitz <mreitz@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [PATCH 3/4] export/fuse: Let permissions be adjustable
Date: Tue, 22 Jun 2021 17:22:02 +0200	[thread overview]
Message-ID: <8f2a3abb-4b42-6257-51b9-08982cd87cb4@redhat.com> (raw)
In-Reply-To: <YNH7l5V/hAKfBmb8@redhat.com>

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.)

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

Max