Re: [PATCH 036/104] virtiofsd: passthrough_ll: add fd_map to hide file descriptors

[Masayoshi Mizuma]

On Thu, Dec 12, 2019 at 04:37:56PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: Stefan Hajnoczi <address@hidden>
> 
> Do not expose file descriptor numbers to clients.  This prevents the
> abuse of internal file descriptors (like stdin/stdout).
> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> dgilbert:
>   Added lseek
> ---
>  tools/virtiofsd/passthrough_ll.c | 114 +++++++++++++++++++++++++------
>  1 file changed, 93 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c 
> b/tools/virtiofsd/passthrough_ll.c
> index face8910b0..93e74cce21 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -60,6 +60,7 @@ struct lo_map_elem {
>      union {
>          struct lo_inode *inode;
>          struct lo_dirp *dirp;
> +        int fd;
>          ssize_t freelist;
>      };
>      bool in_use;
> @@ -107,6 +108,7 @@ struct lo_data {
>      struct lo_inode root; /* protected by lo->mutex */
>      struct lo_map ino_map; /* protected by lo->mutex */
>      struct lo_map dirp_map; /* protected by lo->mutex */
> +    struct lo_map fd_map; /* protected by lo->mutex */
>  };
>  
>  static const struct fuse_opt lo_opts[] = {
> @@ -236,6 +238,20 @@ static void lo_map_remove(struct lo_map *map, size_t key)
>      map->freelist = key;
>  }
>  
> +/* Assumes lo->mutex is held */
> +static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
> +{
> +    struct lo_map_elem *elem;
> +
> +    elem = lo_map_alloc_elem(&lo_data(req)->fd_map);
> +    if (!elem) {
> +        return -1;
> +    }
> +
> +    elem->fd = fd;
> +    return elem - lo_data(req)->fd_map.elems;
> +}
> +
>  /* Assumes lo->mutex is held */
>  static ssize_t lo_add_dirp_mapping(fuse_req_t req, struct lo_dirp *dirp)
>  {
> @@ -350,6 +366,22 @@ static int utimensat_empty_nofollow(struct lo_inode 
> *inode,
>      return utimensat(AT_FDCWD, procname, tv, 0);
>  }
>  
> +static int lo_fi_fd(fuse_req_t req, struct fuse_file_info *fi)
> +{
> +    struct lo_data *lo = lo_data(req);
> +    struct lo_map_elem *elem;
> +
> +    pthread_mutex_lock(&lo->mutex);
> +    elem = lo_map_get(&lo->fd_map, fi->fh);
> +    pthread_mutex_unlock(&lo->mutex);
> +
> +    if (!elem) {
> +        return -1;
> +    }
> +
> +    return elem->fd;
> +}
> +
>  static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
>                         int valid, struct fuse_file_info *fi)
>  {
> @@ -358,6 +390,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, 
> struct stat *attr,
>      struct lo_inode *inode;
>      int ifd;
>      int res;
> +    int fd;
>  
>      inode = lo_inode(req, ino);
>      if (!inode) {
> @@ -367,9 +400,14 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, 
> struct stat *attr,
>  
>      ifd = inode->fd;
>  
> +    /* If fi->fh is invalid we'll report EBADF later */
> +    if (fi) {
> +        fd = lo_fi_fd(req, fi);
> +    }
> +
>      if (valid & FUSE_SET_ATTR_MODE) {
>          if (fi) {
> -            res = fchmod(fi->fh, attr->st_mode);
> +            res = fchmod(fd, attr->st_mode);
>          } else {
>              sprintf(procname, "/proc/self/fd/%i", ifd);
>              res = chmod(procname, attr->st_mode);
> @@ -389,7 +427,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, 
> struct stat *attr,
>      }
>      if (valid & FUSE_SET_ATTR_SIZE) {
>          if (fi) {
> -            res = ftruncate(fi->fh, attr->st_size);
> +            res = ftruncate(fd, attr->st_size);
>          } else {
>              sprintf(procname, "/proc/self/fd/%i", ifd);
>              res = truncate(procname, attr->st_size);
> @@ -419,7 +457,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, 
> struct stat *attr,
>          }
>  
>          if (fi) {
> -            res = futimens(fi->fh, tv);
> +            res = futimens(fd, tv);
>          } else {
>              res = utimensat_empty_nofollow(inode, tv);
>          }
> @@ -1079,7 +1117,18 @@ static void lo_create(fuse_req_t req, fuse_ino_t 
> parent, const char *name,
>      lo_restore_cred(&old);
>  
>      if (!err) {
> -        fi->fh = fd;
> +        ssize_t fh;
> +
> +        pthread_mutex_lock(&lo->mutex);
> +        fh = lo_add_fd_mapping(req, fd);
> +        pthread_mutex_unlock(&lo->mutex);
> +        if (fh == -1) {
> +            close(fd);
> +            fuse_reply_err(req, ENOMEM);
> +            return;
> +        }
> +
> +        fi->fh = fh;
>          err = lo_do_lookup(req, parent, name, &e);
>      }
>      if (lo->cache == CACHE_NEVER) {
> @@ -1123,6 +1172,7 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, 
> int datasync,
>  static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info 
> *fi)
>  {
>      int fd;
> +    ssize_t fh;
>      char buf[64];
>      struct lo_data *lo = lo_data(req);
>  
> @@ -1158,7 +1208,16 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, 
> struct fuse_file_info *fi)
>          return (void)fuse_reply_err(req, errno);
>      }
>  
> -    fi->fh = fd;
> +    pthread_mutex_lock(&lo->mutex);
> +    fh = lo_add_fd_mapping(req, fd);
> +    pthread_mutex_unlock(&lo->mutex);
> +    if (fh == -1) {
> +        close(fd);
> +        fuse_reply_err(req, ENOMEM);
> +        return;
> +    }
> +
> +    fi->fh = fh;
>      if (lo->cache == CACHE_NEVER) {
>          fi->direct_io = 1;
>      } else if (lo->cache == CACHE_ALWAYS) {
> @@ -1170,9 +1229,18 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, 
> struct fuse_file_info *fi)
>  static void lo_release(fuse_req_t req, fuse_ino_t ino,
>                         struct fuse_file_info *fi)
>  {
> +    struct lo_data *lo = lo_data(req);
> +    int fd;
> +
>      (void)ino;
>  
> -    close(fi->fh);
> +    fd = lo_fi_fd(req, fi);
> +
> +    pthread_mutex_lock(&lo->mutex);
> +    lo_map_remove(&lo->fd_map, fi->fh);
> +    pthread_mutex_unlock(&lo->mutex);
> +
> +    close(fd);
>      fuse_reply_err(req, 0);
>  }
>  
> @@ -1180,7 +1248,7 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, 
> struct fuse_file_info *fi)
>  {
>      int res;
>      (void)ino;
> -    res = close(dup(fi->fh));
> +    res = close(dup(lo_fi_fd(req, fi)));
>      fuse_reply_err(req, res == -1 ? errno : 0);
>  }
>  
> @@ -1207,7 +1275,7 @@ static void lo_fsync(fuse_req_t req, fuse_ino_t ino, 
> int datasync,
>              return (void)fuse_reply_err(req, errno);
>          }
>      } else {
> -        fd = fi->fh;
> +        fd = lo_fi_fd(req, fi);
>      }
>  
>      if (datasync) {
> @@ -1234,7 +1302,7 @@ static void lo_read(fuse_req_t req, fuse_ino_t ino, 
> size_t size, off_t offset,
>      }
>  
>      buf.buf[0].flags = FUSE_BUF_IS_FD | FUSE_BUF_FD_SEEK;
> -    buf.buf[0].fd = fi->fh;
> +    buf.buf[0].fd = lo_fi_fd(req, fi);
>      buf.buf[0].pos = offset;
>  
>      fuse_reply_data(req, &buf, FUSE_BUF_SPLICE_MOVE);
> @@ -1249,7 +1317,7 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino,
>      struct fuse_bufvec out_buf = FUSE_BUFVEC_INIT(fuse_buf_size(in_buf));
>  
>      out_buf.buf[0].flags = FUSE_BUF_IS_FD | FUSE_BUF_FD_SEEK;
> -    out_buf.buf[0].fd = fi->fh;
> +    out_buf.buf[0].fd = lo_fi_fd(req, fi);
>      out_buf.buf[0].pos = off;
>  
>      if (lo_debug(req)) {
> @@ -1297,7 +1365,7 @@ static void lo_fallocate(fuse_req_t req, fuse_ino_t 
> ino, int mode, off_t offset,
>          return;
>      }
>  
> -    err = posix_fallocate(fi->fh, offset, length);
> +    err = posix_fallocate(lo_fi_fd(req, fi), offset, length);
>  #endif
>  
>      fuse_reply_err(req, err);
> @@ -1309,7 +1377,7 @@ static void lo_flock(fuse_req_t req, fuse_ino_t ino, 
> struct fuse_file_info *fi,
>      int res;
>      (void)ino;
>  
> -    res = flock(fi->fh, op);
> +    res = flock(lo_fi_fd(req, fi), op);
>  
>      fuse_reply_err(req, res == -1 ? errno : 0);
>  }
> @@ -1534,17 +1602,19 @@ static void lo_copy_file_range(fuse_req_t req, 
> fuse_ino_t ino_in, off_t off_in,
>                                 off_t off_out, struct fuse_file_info *fi_out,
>                                 size_t len, int flags)
>  {
> +    int in_fd, out_fd;
>      ssize_t res;
>  
> -    if (lo_debug(req))
> -        fuse_log(FUSE_LOG_DEBUG,
> -                 "lo_copy_file_range(ino=%" PRIu64 "/fd=%lu, "
> -                 "off=%lu, ino=%" PRIu64 "/fd=%lu, "
> -                 "off=%lu, size=%zd, flags=0x%x)\n",
> -                 ino_in, fi_in->fh, off_in, ino_out, fi_out->fh, off_out, 
> len,
> -                 flags);
> +    in_fd = lo_fi_fd(req, fi_in);
> +    out_fd = lo_fi_fd(req, fi_out);
> +
> +    fuse_log(FUSE_LOG_DEBUG,
> +             "lo_copy_file_range(ino=%" PRIu64 "/fd=%d, "
> +             "off=%lu, ino=%" PRIu64 "/fd=%d, "
> +             "off=%lu, size=%zd, flags=0x%x)\n",
> +             ino_in, in_fd, off_in, ino_out, out_fd, off_out, len, flags);
>  
> -    res = copy_file_range(fi_in->fh, &off_in, fi_out->fh, &off_out, len, 
> flags);
> +    res = copy_file_range(in_fd, &off_in, out_fd, &off_out, len, flags);
>      if (res < 0) {
>          fuse_reply_err(req, -errno);
>      } else {
> @@ -1559,7 +1629,7 @@ static void lo_lseek(fuse_req_t req, fuse_ino_t ino, 
> off_t off, int whence,
>      off_t res;
>  
>      (void)ino;
> -    res = lseek(fi->fh, off, whence);
> +    res = lseek(lo_fi_fd(req, fi), off, whence);
>      if (res != -1) {
>          fuse_reply_lseek(req, res);
>      } else {
> @@ -1644,6 +1714,7 @@ int main(int argc, char *argv[])
>      root_elem->inode = &lo.root;
>  
>      lo_map_init(&lo.dirp_map);
> +    lo_map_init(&lo.fd_map);
>  
>      if (fuse_parse_cmdline(&args, &opts) != 0) {
>          return 1;
> @@ -1741,6 +1812,7 @@ err_out2:
>  err_out1:
>      fuse_opt_free_args(&args);
>  
> +    lo_map_destroy(&lo.fd_map);
>      lo_map_destroy(&lo.dirp_map);
>      lo_map_destroy(&lo.ino_map);
>  
> -- 

Looks good to me.

Reviewed-by: Masayoshi Mizuma <address@hidden>