Re: [Qemu-devel] [PATCH 5/5] virtiofsd: prevent races with lo_dirp_put()

[Stefan Hajnoczi]

On Wed, Jul 31, 2019 at 06:44:52PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (address@hidden) wrote:
> > Introduce lo_dirp_put() so that FUSE_RELEASEDIR does not cause
> > use-after-free races with other threads that are accessing lo_dirp.
> > 
> > Also make lo_releasedir() atomic to prevent FUSE_RELEASEDIR racing with
> > itself.  This prevents double-frees.
> > 
> > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > ---
> >  contrib/virtiofsd/passthrough_ll.c | 42 +++++++++++++++++++++++++-----
> >  1 file changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/contrib/virtiofsd/passthrough_ll.c 
> > b/contrib/virtiofsd/passthrough_ll.c
> > index ad3abdd532..f74e7d2d21 100644
> > --- a/contrib/virtiofsd/passthrough_ll.c
> > +++ b/contrib/virtiofsd/passthrough_ll.c
> > @@ -1293,11 +1293,28 @@ static void lo_readlink(fuse_req_t req, fuse_ino_t 
> > ino)
> >  }
> >  
> >  struct lo_dirp {
> > +   gint refcount;
> >     DIR *dp;
> >     struct dirent *entry;
> >     off_t offset;
> >  };
> >  
> > +static void lo_dirp_put(struct lo_dirp **dp)
> > +{
> > +   struct lo_dirp *d = *dp;
> > +
> > +   if (!d) {
> > +           return;
> > +   }
> > +   *dp = NULL;
> > +
> > +   if (g_atomic_int_dec_and_test(&d->refcount)) {
> > +           closedir(d->dp);
> > +           free(d);
> > +   }
> > +}
> > +
> > +/* Call lo_dirp_put() on the return value when no longer needed */
> >  static struct lo_dirp *lo_dirp(fuse_req_t req, struct fuse_file_info *fi)
> >  {
> >     struct lo_data *lo = lo_data(req);
> > @@ -1305,6 +1322,9 @@ static struct lo_dirp *lo_dirp(fuse_req_t req, struct 
> > fuse_file_info *fi)
> >  
> >     pthread_mutex_lock(&lo->mutex);
> >     elem = lo_map_get(&lo->dirp_map, fi->fh);
> > +   if (elem) {
> > +           g_atomic_int_inc(&elem->dirp->refcount);
> 
> I don't understand what protects against reading the elem->dirp
> here at the same time it's free'd by lo_releasedir's call to lo_dirp_put

It is lo->mutex and not the refcount that prevents the race with
lo_releasedir().  Two cases:

1. lo_releasedir() runs before lo_dirp().  lo_map_get() returns NULL and
   lo_dirp() fails.

2. lo_releasedir() runs after lo_dirp().  lo_map_get() succeeds and the
   lo_dirp() caller keeps the object alive until lo_dirp_put(), when we
   finally free it.

There is no third case since lo->mutex ensures that lo_releasedir() and
lo_dirp() are serialized in the dirp_map lookup.

> > +   }
> >     pthread_mutex_unlock(&lo->mutex);
> >     if (!elem)
> >             return NULL;
> > @@ -1335,6 +1355,8 @@ static void lo_opendir(fuse_req_t req, fuse_ino_t 
> > ino, struct fuse_file_info *fi
> >     d->offset = 0;
> >     d->entry = NULL;
> >  
> > +   g_atomic_int_set(&d->refcount, 1); /* paired with lo_releasedir() */
> > +
> >     fh = lo_add_dirp_mapping(req, d);
> >     if (fh == -1)
> >             goto out_err;
> > @@ -1363,7 +1385,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t 
> > ino, size_t size,
> >                       off_t offset, struct fuse_file_info *fi, int plus)
> >  {
> >     struct lo_data *lo = lo_data(req);
> > -   struct lo_dirp *d;
> > +   struct lo_dirp *d = NULL;
> >     struct lo_inode *dinode;
> >     char *buf = NULL;
> >     char *p;
> > @@ -1451,6 +1473,8 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t 
> > ino, size_t size,
> >  
> >      err = 0;
> >  error:
> > +    lo_dirp_put(&d);
> > +
> >      // If there's an error, we can only signal it if we haven't stored
> >      // any entries yet - otherwise we'd end up with wrong lookup
> >      // counts for the entries that are already in the buffer. So we
> > @@ -1477,22 +1501,25 @@ static void lo_readdirplus(fuse_req_t req, 
> > fuse_ino_t ino, size_t size,
> >  static void lo_releasedir(fuse_req_t req, fuse_ino_t ino, struct 
> > fuse_file_info *fi)
> >  {
> >     struct lo_data *lo = lo_data(req);
> > +   struct lo_map_elem *elem;
> >     struct lo_dirp *d;
> >  
> >     (void) ino;
> >  
> > -   d = lo_dirp(req, fi);
> > -   if (!d) {
> > +   pthread_mutex_lock(&lo->mutex);
> > +   elem = lo_map_get(&lo->dirp_map, fi->fh);
> > +   if (!elem) {
> > +           pthread_mutex_unlock(&lo->mutex);
> >             fuse_reply_err(req, EBADF);
> >             return;
> >     }
> >  
> > -   pthread_mutex_lock(&lo->mutex);
> > +   d = elem->dirp;
> >     lo_map_remove(&lo->dirp_map, fi->fh);
> >     pthread_mutex_unlock(&lo->mutex);
> >  
> > -   closedir(d->dp);
> > -   free(d);
> > +   lo_dirp_put(&d); /* paired with lo_opendir() */
> 
> Is the &d really what's intended? That's the local stack variable, so
> lo_dirp_put will set that local to NULL rather than the elem->dirp wont
> it?

Yes, the put(&ptr) pattern prevents user-after-free in the caller.  It's
slightly safer than put(ptr) since that leaves ptr initialized and the
caller might access it later by accident.

elem has already been returned to the freelist by lo_map_remove() and we
must not touch it anymore.

Stefan

signature.asc
Description: PGP signature