RE: [PATCH 093/104] virtiofsd: introduce inode refcount to prevent use-after-free

[address@hidden]

> > On Thu, Jan 16, 2020 at 09:25:42PM +0900, Misono Tomohiro wrote:
> > > > From: Stefan Hajnoczi <address@hidden>
> > > >
> > > > If thread A is using an inode it must not be deleted by thread B
> > > > when processing a FUSE_FORGET request.
> > > >
> > > > The FUSE protocol itself already has a counter called nlookup that
> > > > is used in FUSE_FORGET messages.  We cannot trust this counter
> > > > since the untrusted client can manipulate it via FUSE_FORGET messages.
> > > >
> > > > Introduce a new refcount to keep inodes alive for the required lifespan.
> > > > lo_inode_put() must be called to release a reference.  FUSE's
> > > > nlookup counter holds exactly one reference so that the inode
> > > > stays alive as long as the client still wants to remember it.
> > > >
> > > > Note that the lo_inode->is_symlink field is moved to avoid
> > > > creating a hole in the struct due to struct field alignment.
> > > >
> > > > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > > > ---
> > > >  tools/virtiofsd/passthrough_ll.c | 168
> > > > ++++++++++++++++++++++++++-----
> > > >  1 file changed, 145 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/tools/virtiofsd/passthrough_ll.c
> > > > b/tools/virtiofsd/passthrough_ll.c
> > > > index b19c9ee328..8f4ab8351c 100644
> > > > --- a/tools/virtiofsd/passthrough_ll.c
> > > > +++ b/tools/virtiofsd/passthrough_ll.c
> > > > @@ -99,7 +99,13 @@ struct lo_key {
> > > >
> > > >  struct lo_inode {
> > > >      int fd;
> > > > -    bool is_symlink;
> > > > +
> > > > +    /*
> > > > +     * Atomic reference count for this object.  The nlookup field 
> > > > holds a
> > > > +     * reference and release it when nlookup reaches 0.
> > > > +     */
> > > > +    gint refcount;
> > > > +
> > > >      struct lo_key key;
> > > >
> > > >      /*
> > > > @@ -118,6 +124,8 @@ struct lo_inode {
> > > >      fuse_ino_t fuse_ino;
> > > >      pthread_mutex_t plock_mutex;
> > > >      GHashTable *posix_locks; /* protected by
> > > > lo_inode->plock_mutex */
> > > > +
> > > > +    bool is_symlink;
> > > >  };
> > > >
> > > >  struct lo_cred {
> > > > @@ -473,6 +481,23 @@ static ssize_t lo_add_inode_mapping(fuse_req_t 
> > > > req, struct lo_inode *inode)
> > > >      return elem - lo_data(req)->ino_map.elems;  }
> > > >
> > > > +static void lo_inode_put(struct lo_data *lo, struct lo_inode
> > > > +**inodep) {
> > > > +    struct lo_inode *inode = *inodep;
> > > > +
> > > > +    if (!inode) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    *inodep = NULL;
> > > > +
> > > > +    if (g_atomic_int_dec_and_test(&inode->refcount)) {
> > > > +        close(inode->fd);
> > > > +        free(inode);
> > > > +    }
> > > > +}
> > > > +
> > > > +/* Caller must release refcount using lo_inode_put() */
> > > >  static struct lo_inode *lo_inode(fuse_req_t req, fuse_ino_t ino)
> > > > {
> > > >      struct lo_data *lo = lo_data(req); @@ -480,6 +505,9 @@ static
> > > > struct lo_inode *lo_inode(fuse_req_t req, fuse_ino_t ino)
> > > >
> > > >      pthread_mutex_lock(&lo->mutex);
> > > >      elem = lo_map_get(&lo->ino_map, ino);
> > > > +    if (elem) {
> > > > +        g_atomic_int_inc(&elem->inode->refcount);
> > > > +    }
> > > >      pthread_mutex_unlock(&lo->mutex);
> > > >
> > > >      if (!elem) {
> > > > @@ -489,10 +517,23 @@ static struct lo_inode *lo_inode(fuse_req_t req, 
> > > > fuse_ino_t ino)
> > > >      return elem->inode;
> > > >  }
> > > >
> > > > +/*
> > > > + * TODO Remove this helper and force callers to hold an inode
> > > > +refcount until
> > > > + * they are done with the fd.  This will be done in a later patch
> > > > +to make
> > > > + * review easier.
> > > > + */
> > > >  static int lo_fd(fuse_req_t req, fuse_ino_t ino)  {
> > > >      struct lo_inode *inode = lo_inode(req, ino);
> > > > -    return inode ? inode->fd : -1;
> > > > +    int fd;
> > > > +
> > > > +    if (!inode) {
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    fd = inode->fd;
> > > > +    lo_inode_put(lo_data(req), &inode);
> > > > +    return fd;
> > > >  }
> > > >
> > > >  static void lo_init(void *userdata, struct fuse_conn_info *conn)
> > > > @@ -547,6 +588,10 @@ static void lo_getattr(fuse_req_t req, fuse_ino_t 
> > > > ino,
> > > >      fuse_reply_attr(req, &buf, lo->timeout);  }
> > > >
> > > > +/*
> > > > + * Increments parent->nlookup and caller must release refcount
> > > > +using
> > > > + * lo_inode_put(&parent).
> > > > + */
> > > >  static int lo_parent_and_name(struct lo_data *lo, struct lo_inode 
> > > > *inode,
> > > >                                char path[PATH_MAX], struct
> > > > lo_inode **parent)  { @@ -584,6 +629,7 @@ retry:
> > > >          p = &lo->root;
> > > >          pthread_mutex_lock(&lo->mutex);
> > > >          p->nlookup++;
> > > > +        g_atomic_int_inc(&p->refcount);
> > > >          pthread_mutex_unlock(&lo->mutex);
> > > >      } else {
> > > >          *last = '\0';
> > >
> > > We need lo_ionde_put() in error path, right?:
> > > https://gitlab.com/virtio-fs/qemu/blob/virtio-fs-as-posted-2019-12-1
> > > 2/tools/virtiofsd/passthrough_ll.c#L680
> >
> > Yes, thanks for spotting this bug!  The lo_parent_and_name() code
> > should look like this:
> >
> >   fail_unref:
> >       unref_inode_lolocked(lo, p, 1);
> >       lo_inode_put(lo, &p);
> >   ...
> 
> I've merged that one in.

Thanks, so with that:
 Reviewed-by: Misono Tomohiro <address@hidden>

> 
> > > nit: if yes, unref_inode_lolocked() is always paired with lo_inode_put().
> > > So how about combine them in one function? As p->nloockup and
> > > p->refcount are both incremented in one place
> > > (lo_find/lo_parent_and_name) in these case, it seems natural for me to 
> > > decrement them in one function as well.
> >
> > Nice idea.  I would also drop the nlookup argument - this function
> > will only be used with nlookup=1.
> 
> I'll leave that to you if you want to send a patch on top.
> 
> Dave
> 
> > Stefan
> 
> 
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK