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 <mre...@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
