Re: [PATCH] virtiofsd: fix libfuse information leaks

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (address@hidden) wrote:
> Some FUSE message replies contain padding fields that are not
> initialized by libfuse.  This is fine in traditional FUSE applications
> because the kernel is trusted.  virtiofsd does not trust the guest and
> must not expose uninitialized memory.
> 
> Use C struct initializers to automatically zero out memory.  Not all of
> these code changes are strictly necessary but they will prevent future
> information leaks if the structs are extended.
> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>

Reviewed-by: Dr. David Alan Gilbert <address@hidden>

Thanks, merged.

Dave

> ---
>  contrib/virtiofsd/fuse_lowlevel.c | 150 +++++++++++++++---------------
>  1 file changed, 76 insertions(+), 74 deletions(-)
> 
> diff --git a/contrib/virtiofsd/fuse_lowlevel.c 
> b/contrib/virtiofsd/fuse_lowlevel.c
> index 7fa8861f5d..8a4234630d 100644
> --- a/contrib/virtiofsd/fuse_lowlevel.c
> +++ b/contrib/virtiofsd/fuse_lowlevel.c
> @@ -45,21 +45,23 @@ static __attribute__((constructor)) void 
> fuse_ll_init_pagesize(void)
>  
>  static void convert_stat(const struct stat *stbuf, struct fuse_attr *attr)
>  {
> -     attr->ino       = stbuf->st_ino;
> -     attr->mode      = stbuf->st_mode;
> -     attr->nlink     = stbuf->st_nlink;
> -     attr->uid       = stbuf->st_uid;
> -     attr->gid       = stbuf->st_gid;
> -     attr->rdev      = stbuf->st_rdev;
> -     attr->size      = stbuf->st_size;
> -     attr->blksize   = stbuf->st_blksize;
> -     attr->blocks    = stbuf->st_blocks;
> -     attr->atime     = stbuf->st_atime;
> -     attr->mtime     = stbuf->st_mtime;
> -     attr->ctime     = stbuf->st_ctime;
> -     attr->atimensec = ST_ATIM_NSEC(stbuf);
> -     attr->mtimensec = ST_MTIM_NSEC(stbuf);
> -     attr->ctimensec = ST_CTIM_NSEC(stbuf);
> +     *attr = (struct fuse_attr) {
> +             .ino            = stbuf->st_ino,
> +             .mode           = stbuf->st_mode,
> +             .nlink          = stbuf->st_nlink,
> +             .uid            = stbuf->st_uid,
> +             .gid            = stbuf->st_gid,
> +             .rdev           = stbuf->st_rdev,
> +             .size           = stbuf->st_size,
> +             .blksize        = stbuf->st_blksize,
> +             .blocks         = stbuf->st_blocks,
> +             .atime          = stbuf->st_atime,
> +             .mtime          = stbuf->st_mtime,
> +             .ctime          = stbuf->st_ctime,
> +             .atimensec      = ST_ATIM_NSEC(stbuf),
> +             .mtimensec      = ST_MTIM_NSEC(stbuf),
> +             .ctimensec      = ST_CTIM_NSEC(stbuf),
> +     };
>  }
>  
>  static void convert_attr(const struct fuse_setattr_in *attr, struct stat 
> *stbuf)
> @@ -181,16 +183,16 @@ static int fuse_send_msg(struct fuse_session *se, 
> struct fuse_chan *ch,
>  int fuse_send_reply_iov_nofree(fuse_req_t req, int error, struct iovec *iov,
>                              int count)
>  {
> -     struct fuse_out_header out;
> +     struct fuse_out_header out = {
> +             .unique = req->unique,
> +             .error = error,
> +     };
>  
>       if (error <= -1000 || error > 0) {
>               fuse_log(FUSE_LOG_ERR, "fuse: bad error value: %i\n",   error);
>               error = -ERANGE;
>       }
>  
> -     out.unique = req->unique;
> -     out.error = error;
> -
>       iov[0].iov_base = &out;
>       iov[0].iov_len = sizeof(struct fuse_out_header);
>  
> @@ -271,14 +273,16 @@ size_t fuse_add_direntry(fuse_req_t req, char *buf, 
> size_t bufsize,
>  static void convert_statfs(const struct statvfs *stbuf,
>                          struct fuse_kstatfs *kstatfs)
>  {
> -     kstatfs->bsize   = stbuf->f_bsize;
> -     kstatfs->frsize  = stbuf->f_frsize;
> -     kstatfs->blocks  = stbuf->f_blocks;
> -     kstatfs->bfree   = stbuf->f_bfree;
> -     kstatfs->bavail  = stbuf->f_bavail;
> -     kstatfs->files   = stbuf->f_files;
> -     kstatfs->ffree   = stbuf->f_ffree;
> -     kstatfs->namelen = stbuf->f_namemax;
> +     *kstatfs = (struct fuse_kstatfs) {
> +             .bsize   = stbuf->f_bsize,
> +             .frsize  = stbuf->f_frsize,
> +             .blocks  = stbuf->f_blocks,
> +             .bfree   = stbuf->f_bfree,
> +             .bavail  = stbuf->f_bavail,
> +             .files   = stbuf->f_files,
> +             .ffree   = stbuf->f_ffree,
> +             .namelen = stbuf->f_namemax,
> +     };
>  }
>  
>  static int send_reply_ok(fuse_req_t req, const void *arg, size_t argsize)
> @@ -320,12 +324,14 @@ static unsigned int calc_timeout_nsec(double t)
>  static void fill_entry(struct fuse_entry_out *arg,
>                      const struct fuse_entry_param *e)
>  {
> -     arg->nodeid = e->ino;
> -     arg->generation = e->generation;
> -     arg->entry_valid = calc_timeout_sec(e->entry_timeout);
> -     arg->entry_valid_nsec = calc_timeout_nsec(e->entry_timeout);
> -     arg->attr_valid = calc_timeout_sec(e->attr_timeout);
> -     arg->attr_valid_nsec = calc_timeout_nsec(e->attr_timeout);
> +     *arg = (struct fuse_entry_out) {
> +             .nodeid = e->ino,
> +             .generation = e->generation,
> +             .entry_valid = calc_timeout_sec(e->entry_timeout),
> +             .entry_valid_nsec = calc_timeout_nsec(e->entry_timeout),
> +             .attr_valid = calc_timeout_sec(e->attr_timeout),
> +             .attr_valid_nsec = calc_timeout_nsec(e->attr_timeout),
> +     };
>       convert_stat(&e->attr, &arg->attr);
>  }
>  
> @@ -351,10 +357,12 @@ size_t fuse_add_direntry_plus(fuse_req_t req, char 
> *buf, size_t bufsize,
>       fill_entry(&dp->entry_out, e);
>  
>       struct fuse_dirent *dirent = &dp->dirent;
> -     dirent->ino = e->attr.st_ino;
> -     dirent->off = off;
> -     dirent->namelen = namelen;
> -     dirent->type = (e->attr.st_mode & S_IFMT) >> 12;
> +     *dirent = (struct fuse_dirent) {
> +             .ino = e->attr.st_ino,
> +             .off = off,
> +             .namelen = namelen,
> +             .type = (e->attr.st_mode & S_IFMT) >> 12,
> +     };
>       memcpy(dirent->name, name, namelen);
>       memset(dirent->name + namelen, 0, entlen_padded - entlen);
>  
> @@ -504,15 +512,14 @@ int fuse_reply_data(fuse_req_t req, struct fuse_bufvec 
> *bufv,
>                   enum fuse_buf_copy_flags flags)
>  {
>       struct iovec iov[2];
> -     struct fuse_out_header out;
> +     struct fuse_out_header out = {
> +             .unique = req->unique,
> +     };
>       int res;
>  
>       iov[0].iov_base = &out;
>       iov[0].iov_len = sizeof(struct fuse_out_header);
>  
> -     out.unique = req->unique;
> -     out.error = 0;
> -
>       res = fuse_send_data_iov(req->se, req->ch, iov, 1, bufv, flags);
>       if (res <= 0) {
>               fuse_free_req(req);
> @@ -2203,13 +2210,13 @@ static void do_destroy(fuse_req_t req, fuse_ino_t 
> nodeid,
>  static int send_notify_iov(struct fuse_session *se, int notify_code,
>                          struct iovec *iov, int count)
>  {
> -     struct fuse_out_header out;
> +     struct fuse_out_header out = {
> +             .error = notify_code,
> +     };
>  
>       if (!se->got_init)
>               return -ENOTCONN;
>  
> -     out.unique = 0;
> -     out.error = notify_code;
>       iov[0].iov_base = &out;
>       iov[0].iov_len = sizeof(struct fuse_out_header);
>  
> @@ -2219,11 +2226,11 @@ static int send_notify_iov(struct fuse_session *se, 
> int notify_code,
>  int fuse_lowlevel_notify_poll(struct fuse_pollhandle *ph)
>  {
>       if (ph != NULL) {
> -             struct fuse_notify_poll_wakeup_out outarg;
> +             struct fuse_notify_poll_wakeup_out outarg = {
> +                     .kh = ph->kh,
> +             };
>               struct iovec iov[2];
>  
> -             outarg.kh = ph->kh;
> -
>               iov[1].iov_base = &outarg;
>               iov[1].iov_len = sizeof(outarg);
>  
> @@ -2236,7 +2243,11 @@ int fuse_lowlevel_notify_poll(struct fuse_pollhandle 
> *ph)
>  int fuse_lowlevel_notify_inval_inode(struct fuse_session *se, fuse_ino_t ino,
>                                    off_t off, off_t len)
>  {
> -     struct fuse_notify_inval_inode_out outarg;
> +     struct fuse_notify_inval_inode_out outarg = {
> +             .ino = ino,
> +             .off = off,
> +             .len = len,
> +     };
>       struct iovec iov[2];
>  
>       if (!se)
> @@ -2244,10 +2255,6 @@ int fuse_lowlevel_notify_inval_inode(struct 
> fuse_session *se, fuse_ino_t ino,
>  
>       if (se->conn.proto_major < 6 || se->conn.proto_minor < 12)
>               return -ENOSYS;
> -     
> -     outarg.ino = ino;
> -     outarg.off = off;
> -     outarg.len = len;
>  
>       iov[1].iov_base = &outarg;
>       iov[1].iov_len = sizeof(outarg);
> @@ -2258,7 +2265,10 @@ int fuse_lowlevel_notify_inval_inode(struct 
> fuse_session *se, fuse_ino_t ino,
>  int fuse_lowlevel_notify_inval_entry(struct fuse_session *se, fuse_ino_t 
> parent,
>                                    const char *name, size_t namelen)
>  {
> -     struct fuse_notify_inval_entry_out outarg;
> +     struct fuse_notify_inval_entry_out outarg = {
> +             .parent = parent,
> +             .namelen = namelen,
> +     };
>       struct iovec iov[3];
>  
>       if (!se)
> @@ -2267,10 +2277,6 @@ int fuse_lowlevel_notify_inval_entry(struct 
> fuse_session *se, fuse_ino_t parent,
>       if (se->conn.proto_major < 6 || se->conn.proto_minor < 12)
>               return -ENOSYS;
>  
> -     outarg.parent = parent;
> -     outarg.namelen = namelen;
> -     outarg.padding = 0;
> -
>       iov[1].iov_base = &outarg;
>       iov[1].iov_len = sizeof(outarg);
>       iov[2].iov_base = (void *)name;
> @@ -2283,7 +2289,11 @@ int fuse_lowlevel_notify_delete(struct fuse_session 
> *se,
>                               fuse_ino_t parent, fuse_ino_t child,
>                               const char *name, size_t namelen)
>  {
> -     struct fuse_notify_delete_out outarg;
> +     struct fuse_notify_delete_out outarg = {
> +             .parent = parent,
> +             .child = child,
> +             .namelen = namelen,
> +     };
>       struct iovec iov[3];
>  
>       if (!se)
> @@ -2292,11 +2302,6 @@ int fuse_lowlevel_notify_delete(struct fuse_session 
> *se,
>       if (se->conn.proto_major < 6 || se->conn.proto_minor < 18)
>               return -ENOSYS;
>  
> -     outarg.parent = parent;
> -     outarg.child = child;
> -     outarg.namelen = namelen;
> -     outarg.padding = 0;
> -
>       iov[1].iov_base = &outarg;
>       iov[1].iov_len = sizeof(outarg);
>       iov[2].iov_base = (void *)name;
> @@ -2309,10 +2314,15 @@ int fuse_lowlevel_notify_store(struct fuse_session 
> *se, fuse_ino_t ino,
>                              off_t offset, struct fuse_bufvec *bufv,
>                              enum fuse_buf_copy_flags flags)
>  {
> -     struct fuse_out_header out;
> -     struct fuse_notify_store_out outarg;
> +     struct fuse_out_header out = {
> +             .error = FUSE_NOTIFY_STORE,
> +     };
> +     struct fuse_notify_store_out outarg = {
> +             .nodeid = ino,
> +             .offset = offset,
> +             .size = fuse_buf_size(bufv),
> +     };
>       struct iovec iov[3];
> -     size_t size = fuse_buf_size(bufv);
>       int res;
>  
>       if (!se)
> @@ -2321,14 +2331,6 @@ int fuse_lowlevel_notify_store(struct fuse_session 
> *se, fuse_ino_t ino,
>       if (se->conn.proto_major < 6 || se->conn.proto_minor < 15)
>               return -ENOSYS;
>  
> -     out.unique = 0;
> -     out.error = FUSE_NOTIFY_STORE;
> -
> -     outarg.nodeid = ino;
> -     outarg.offset = offset;
> -     outarg.size = size;
> -     outarg.padding = 0;
> -
>       iov[0].iov_base = &out;
>       iov[0].iov_len = sizeof(out);
>       iov[1].iov_base = &outarg;
> -- 
> 2.23.0
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK