| Message ID | 20210614144407.134243-4-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | export/fuse: Allow other users access to the export | expand |
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
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
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
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; + } + + 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); }
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(-)