[Qemu-devel] [PATCH] virtiofsd: fix req use-after-free in lo_flush()

Stefan Hajnoczi posted 1 patch 4 years, 8 months ago
Failed in applying to current master (apply log)
contrib/virtiofsd/passthrough_ll.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
[Qemu-devel] [PATCH] virtiofsd: fix req use-after-free in lo_flush()
Posted by Stefan Hajnoczi 4 years, 8 months ago
Isn't it funny that a commit trying to fix use-after-free introduced its
own use-after-free?

fuse_reply_err() frees the request so we cannot call lo_data(req)
afterwards.

Fixes: a5081fc6fc3603671383616127b3a5e169cf64ed
       ("virtiofsd: introduce inode refcount to prevent use-after-free")
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/virtiofsd/passthrough_ll.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index ee8627446a..a81c01d0d1 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -1969,9 +1969,8 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
 	pthread_mutex_unlock(&inode->plock_mutex);
 
 	res = close(dup(lo_fi_fd(req, fi)));
-	fuse_reply_err(req, res == -1 ? errno : 0);
-
 	lo_inode_put(lo_data(req), &inode);
+	fuse_reply_err(req, res == -1 ? errno : 0);
 }
 
 static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync,
-- 
2.21.0


Re: [Qemu-devel] [PATCH] virtiofsd: fix req use-after-free in lo_flush()
Posted by Dr. David Alan Gilbert 4 years, 8 months ago
* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> Isn't it funny that a commit trying to fix use-after-free introduced its
> own use-after-free?
> 
> fuse_reply_err() frees the request so we cannot call lo_data(req)
> afterwards.
> 
> Fixes: a5081fc6fc3603671383616127b3a5e169cf64ed
>        ("virtiofsd: introduce inode refcount to prevent use-after-free")
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Squashed into the top.

Dave

> ---
>  contrib/virtiofsd/passthrough_ll.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index ee8627446a..a81c01d0d1 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1969,9 +1969,8 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>  	pthread_mutex_unlock(&inode->plock_mutex);
>  
>  	res = close(dup(lo_fi_fd(req, fi)));
> -	fuse_reply_err(req, res == -1 ? errno : 0);
> -
>  	lo_inode_put(lo_data(req), &inode);
> +	fuse_reply_err(req, res == -1 ? errno : 0);
>  }
>  
>  static void lo_fsync(fuse_req_t req, fuse_ino_t ino, int datasync,
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK