Re: [Virtio-fs] [PATCH v7 1/6] virtiofsd: add .ioctl() support

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Dec 10, 2021 at 10:51:45AM +0800, JeffleXu wrote:
> 
> 
> On 12/10/21 3:33 AM, Vivek Goyal wrote:
> > On Tue, Nov 02, 2021 at 01:56:41PM +0800, Jeffle Xu wrote:
> >> For passthrough, it passes corresponding ioctls to host directly.
> >>
> >> Currently only these ioctls that handling persistent inode flags, i.e.,
> >> FS_IOC_[G|S]ETFLAGS and FS_IOC_FS[G|S]ETXATTR are supported for security
> >> concern, though it's implemented in a quite general way, so that we can
> >> expand the scope of allowed ioctls if it is really needed later.
> >>
> >> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> >> ---
> >>  tools/virtiofsd/passthrough_ll.c      | 65 +++++++++++++++++++++++++++
> >>  tools/virtiofsd/passthrough_seccomp.c |  1 +
> >>  2 files changed, 66 insertions(+)
> >>
> >> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> >> index b7c1fa71b5..2768497be4 100644
> >> --- a/tools/virtiofsd/passthrough_ll.c
> >> +++ b/tools/virtiofsd/passthrough_ll.c
> >> @@ -47,6 +47,7 @@
> >>  #include <dirent.h>
> >>  #include <pthread.h>
> >>  #include <sys/file.h>
> >> +#include <sys/ioctl.h>
> >>  #include <sys/mount.h>
> >>  #include <sys/prctl.h>
> >>  #include <sys/resource.h>
> >> @@ -54,6 +55,7 @@
> >>  #include <sys/wait.h>
> >>  #include <sys/xattr.h>
> >>  #include <syslog.h>
> >> +#include <linux/fs.h>
> >>  
> >>  #include "qemu/cutils.h"
> >>  #include "passthrough_helpers.h"
> >> @@ -2188,6 +2190,68 @@ out:
> >>      fuse_reply_err(req, saverr);
> >>  }
> >>  
> >> +
> >> +static void lo_ioctl(fuse_req_t req, fuse_ino_t ino, unsigned int cmd,
> >> +                     void *arg, struct fuse_file_info *fi,
> >> +                     unsigned flags, const void *in_buf,
> >> +                     size_t in_bufsz, size_t out_bufsz)
> >> +{
> >> +    int res, dir;
> >> +    int fd = lo_fi_fd(req, fi);
> >> +    int saverr = ENOSYS;
> >> +    void *buf = NULL;
> >> +    size_t size = 0;
> >> +
> >> +    fuse_log(FUSE_LOG_DEBUG, "lo_ioctl(ino=%" PRIu64 ", cmd=0x%x, flags=0x%x, "
> >> +            "in_bufsz = %lu, out_bufsz = %lu)\n",
> >> +            ino, cmd, flags, in_bufsz, out_bufsz);
> >> +
> >> +    if (!(cmd == FS_IOC_SETFLAGS || cmd == FS_IOC_GETFLAGS ||
> >> +          cmd == FS_IOC_FSSETXATTR || cmd == FS_IOC_FSGETXATTR)) {
> >> +        goto out;
> > 
> > Good that you allowed only two operations. Still I think people might
> > have concern about all the inode attrs and whether it is safe to
> > allow that. For example immutable flag. Now even host can't delete
> > that file without first clearing that flag. 
> 
> Yes, this will be an issue.
> 
> > I think it is doable
> > just that involves extra step.
> > 
> > So we probably might have to block certain attrs if it becomes an
> > issue or make it configurable.
> 
> Currently fuse kernel module only requires that virtiofsd shall support
> the set flag path (FS_IOC_SETFLAGS/FS_IOC_FSSETXATTR), so that users
> inside guest are able to set/clear FS_XFLAG_DAX flag. For the set flag
> path, maybe we could support modifying FS_XFLAG_DAX flag first for the
> security concern?

I think right now limiting ioctl operations to FS_XFLAG_DAX is not
a bad idea. That's what we need for now. Others can be opened when
people need it.

> 
> The get flag path(FS_IOC_GETFLAGS/FS_IOC_FSGETXATTR) is not strongly
> required currently, since the DAX flag is totally constructed by
> virtiofsd now. The use case of the get flag path may be like, users
> inside guest may want to query the FS_XFLAG_DAX flag after setting
> FS_XFLAG_DAX flag previously. The get flag path may also expose the file
> attr of the host file to the guest, and thus maybe we shall also only
> support quering FS_XFLAG_DAX flag as the first step.

Querying FS_XFLAG_DAX is fine. And I agreed that it is needed because
guest might want to query it.

So let us support both setting and querying FS_XFLAG_DAX and block other
operations for now.

> 
> 
> > 
> >> +    }
> >> +
> >> +    /* unrestricted ioctl is not supported yet */
> >> +    if (flags & FUSE_IOCTL_UNRESTRICTED) {
> >> +        goto out;
> >> +    }
> >> +
> >> +    dir = _IOC_DIR(cmd);
> >> +
> >> +    if (dir & _IOC_READ) {
> >> +        size = out_bufsz;
> >> +        buf = malloc(size);
> >> +        if (!buf) {
> >> +            goto out_err;
> >> +        }
> >> +
> >> +        if (dir & _IOC_WRITE) {
> >> +            memcpy(buf, in_buf, size);
> >> +        }
> >> +
> >> +        res = ioctl(fd, cmd, buf);
> >> +    } else if (dir & _IOC_WRITE) {
> >> +        res = ioctl(fd, cmd, in_buf);
> >> +    } else {
> >> +        res = ioctl(fd, cmd, arg);
> >> +    }
> > 
> > I do not understand this if block. So if _IOC_READ and _IOC_WRITE
> > is not set, then we just send "arg" as third argument. Can you
> > shed some light on how this third argument works for file attr
> > flags. 
> 
> In theory ioctl could be neither _IOC_READ nor _IOC_WRITE. I implement
> this if block just for the completeness of the logic for ioctl support.
> It doesn't matter with the file attr though.
> 
> 
> > I am assuming that "lsattr" will use the _IOC_READ path,
> > while chattr will use "_IOC_WRITE" path? If that's the case,
> > as of now third condition is not even required. Until and unless
> > we are supporting more ioctls.
> > 
> 
> Sure, I can remove this if block for now.

Sounds good. Let somebody else add it later when need be. For now, we 
can probably focus on FS_XFLAG_DAX attr only. And put a comment in code
explaining this restriction.

Vivek