[PATCH] fuse: virtio-fs: don't process completions of aborted requests

[PATCH] fuse: virtio-fs: don't process completions of aborted requests

[Qihang]

fuse_chan_abort() ends every request on the processing lists with
-ECONNABORTED, but virtio-fs keeps the request itself as the
used-buffer token of the virtqueue.  When the host stalls a request
until the request timeout aborts the connection and then completes the
descriptor, virtio_fs_requests_done_work() gets the already freed
request from virtqueue_get_buf() and moves its list node
(list_move_tail()) and reads its arguments (copy_args_from_argbuf()),
a use-after-free in the guest kernel.

Let the transport know about the abort and skip completions of requests
that the abort has already ended.  This is safe because any request
still represented by a virtqueue token when the abort starts must have
been queued before virtio_fs_abort() set aborted:
virtio_fs_enqueue_req() checks connected, adds the request to the
virtqueue, and puts it on the processing list while holding fsvq->lock.
virtio_fs_abort() takes the same lock before setting connected = false
and aborted = true, and fuse_chan_abort() only splices and ends the
processing lists after the transport callback returned.  A racing
enqueue therefore either completes before the abort and is subsequently
ended by it, or observes connected == false and returns -ENOTCONN
without creating a token.

in_flight is transport-owned accounting: it is incremented when a
token is queued and must still be dropped when the virtqueue completion
is consumed, even though the FUSE request itself has already been ended
by the abort.

Fixes: a62a8ef9d97d ("virtio-fs: add virtiofs filesystem")
Cc: stable@vger.kernel.org
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 fs/fuse/dev.c        |  4 ++++
 fs/fuse/fuse_dev_i.h |  5 +++++
 fs/fuse/virtio_fs.c  | 26 ++++++++++++++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 4fec31fc0b84..90b30c5034c4 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -2178,6 +2178,10 @@ void fuse_chan_abort(struct fuse_chan *fch, bool abort_with_err)
 		spin_unlock(&fch->bg_lock);
 
 		fuse_chan_set_initialized(fch, NULL);
+
+		if (fiq->ops->abort)
+			fiq->ops->abort(fiq);
+
 		list_for_each_entry(fud, &fch->devices, entry) {
 			struct fuse_pqueue *fpq = &fud->pq;
 
diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
index 4b412a76225f..47697adf0eb6 100644
--- a/fs/fuse/fuse_dev_i.h
+++ b/fs/fuse/fuse_dev_i.h
@@ -148,6 +148,11 @@ struct fuse_iqueue_ops {
 	 */
 	void (*send_req)(struct fuse_iqueue *fiq, struct fuse_req *req);
 
+	/**
+	 * @abort: Abort transport-specific state
+	 */
+	void (*abort)(struct fuse_iqueue *fiq);
+
 	/**
 	 * @release: Clean up when fuse_iqueue is destroyed
 	 */
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index f15e516ebcb5..2db5faf5d865 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -55,6 +55,7 @@ struct virtio_fs_vq {
 	struct work_struct dispatch_work;
 	struct fuse_dev *fud;
 	bool connected;
+	bool aborted;
 	long in_flight;
 	struct completion in_flight_zero; /* No inflight requests */
 	struct kobject *kobj;
@@ -842,6 +843,15 @@ static void virtio_fs_requests_done_work(struct work_struct *work)
 		virtqueue_disable_cb(vq);
 
 		while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
+			if (fsvq->aborted) {
+				/*
+				 * fuse_chan_abort() ended every request this
+				 * queue still owns; the token may already be
+				 * freed, do not touch it.
+				 */
+				dec_in_flight_req(fsvq);
+				continue;
+			}
 			if (!virtio_fs_verify_response(req, len)) {
 				req->out.h.error = -EIO;
 				req->out.h.len = sizeof(struct fuse_out_header);
@@ -1541,10 +1551,26 @@ static void virtio_fs_send_req(struct fuse_iqueue *fiq, struct fuse_req *req)
 	}
 }
 
+static void virtio_fs_abort(struct fuse_iqueue *fiq)
+{
+	struct virtio_fs *fs = fiq->priv;
+	int i;
+
+	for (i = 0; i < fs->nvqs; i++) {
+		struct virtio_fs_vq *fsvq = &fs->vqs[i];
+
+		spin_lock(&fsvq->lock);
+		fsvq->connected = false;
+		fsvq->aborted = true;
+		spin_unlock(&fsvq->lock);
+	}
+}
+
 static const struct fuse_iqueue_ops virtio_fs_fiq_ops = {
 	.send_forget	= virtio_fs_send_forget,
 	.send_interrupt	= virtio_fs_send_interrupt,
 	.send_req	= virtio_fs_send_req,
+	.abort		= virtio_fs_abort,
 	.release	= virtio_fs_fiq_release,
 };
 

base-commit: ac86e1112a6a
-- 
2.51.0

Re: [PATCH] fuse: virtio-fs: don't process completions of aborted requests

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] fuse: virtio-fs: DMA use-after-free on aborted requests in fuse_chan_abort()
- [High] fuse: virtio-fs: fsvq->aborted never cleared in virtio_fs_start_all_queues()
- [High] fuse: virtio-fs: queued_reqs not aborted or drained in virtio_fs_abort()
- [Medium] fuse: virtio-fs: req->argbuf leak on aborted requests in done_work
--

commit 0a788f32822f61f6e1ab378f337037e0c6397f67
Author: Qihang <q.h.hack.winter@gmail.com>

fuse: virtio-fs: don't process completions of aborted requests

This commit adds an abort callback to struct fuse_iqueue_ops so virtio-fs can
mark its virtqueues as aborted when fuse_chan_abort() runs. In
virtio_fs_requests_done_work(), completions on an aborted virtqueue are
skipped after decrementing in_flight to avoid dereferencing requests already
freed by fuse_chan_abort().

> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index ba8d79c4bcac6..ae4a14aaaba98 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -55,6 +55,7 @@ struct virtio_fs_vq {
>  	struct work_struct dispatch_work;
>  	struct fuse_dev *fud;
>  	bool connected;
> +	bool aborted;
>  	long in_flight;
>  	struct completion in_flight_zero; /* No inflight requests */
>  	struct kobject *kobj;
> @@ -871,6 +872,15 @@ static void virtio_fs_requests_done_work(struct work_struct *work)
>  		virtqueue_disable_cb(vq);
>  
>  		while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
> +			if (fsvq->aborted) {
> +				/*
> +				 * fuse_chan_abort() ended every request this
> +				 * queue still owns; the token may already be
> +				 * freed, do not touch it.
> +				 */
> +				dec_in_flight_req(fsvq);
> +				continue;
> +			}

[Severity: Critical]
Can this still result in a DMA use-after-free on struct fuse_req and
ap->folios before virtqueue_get_buf() even returns?

In virtio_fs_enqueue_req(), the scatter-gather list passed to
virtqueue_add_sgs() maps &req->in.h, req->argbuf, and input ap->folios as
device-readable buffers, and maps &req->out.h (embedded inside struct
fuse_req), req->argbuf + argbuf_used, and output ap->folios as
device-writable buffers:

fs/fuse/virtio_fs.c:virtio_fs_enqueue_req() {
	...
	/* Request elements */
	sg_init_one(&sg[out_sgs++], &req->in.h, sizeof(req->in.h));
	out_sgs += sg_init_fuse_args(&sg[out_sgs], req,
				     (struct fuse_arg *)args->in_args,
				     args->in_numargs, args->in_pages,
				     req->argbuf, &argbuf_used);

	/* Reply elements */
	if (test_bit(FR_ISREPLY, &req->flags)) {
		sg_init_one(&sg[out_sgs + in_sgs++],
			    &req->out.h, sizeof(req->out.h));
		in_sgs += sg_init_fuse_args(&sg[out_sgs + in_sgs], req,
					    args->out_args, args->out_numargs,
					    args->out_pages,
					    req->argbuf + argbuf_used, NULL);
	}
	...
}

When fuse_chan_abort() runs while a request is in flight in the virtqueue,
virtio_fs_abort() sets fsvq->aborted = true without resetting the virtio
device, and fuse_chan_abort() immediately splices fpq->processing onto
to_end and calls fuse_dev_end_requests() -> fuse_request_end(). That invokes
req->args->end() (unlocking and releasing ap->folios) and frees struct
fuse_req back to fuse_req_cachep via fuse_request_free() while the
descriptor chain remains active in the virtqueue.

When the host later completes the stalled descriptor, won't the device (and
any DMA unmapping in virtqueue_get_buf()) write into &req->out.h inside the
already-freed struct fuse_req and into the released ap->folios before
reaching the fsvq->aborted check here?

[Severity: Medium]
Does skipping virtio_fs_request_complete() here also leak req->argbuf for
every aborted in-flight request?

When virtio_fs_enqueue_req() prepares a request, copy_args_to_argbuf()
allocates req->argbuf:

fs/fuse/virtio_fs.c:copy_args_to_argbuf() {
	...
	req->argbuf = kmalloc(len, gfp);
	if (!req->argbuf)
		return -ENOMEM;
	...
}

Normally req->argbuf is freed in copy_args_from_argbuf() called from
virtio_fs_request_complete(). When fuse_chan_abort() ends requests on
fpq->processing via fuse_dev_end_requests() -> fuse_request_end() ->
fuse_put_request() -> fuse_request_free(), struct fuse_req is freed to
fuse_req_cachep without freeing req->argbuf:

fs/fuse/dev.c:fuse_request_free() {
	WARN_ON(!list_empty(&req->intr_entry));
	kmem_cache_free(fuse_req_cachep, req);
}

Since virtio_fs_requests_done_work() skips virtio_fs_request_complete() when
fsvq->aborted is true (and req is already freed at that point), where is
req->argbuf freed for aborted requests?

>  			if (!virtio_fs_verify_response(req, len)) {
>  				req->out.h.error = -EIO;
>  				req->out.h.len = sizeof(struct fuse_out_header);
> @@ -1570,10 +1580,26 @@ static void virtio_fs_send_req(struct fuse_iqueue *fiq, struct fuse_req *req)
>  	}
>  }
>  
> +static void virtio_fs_abort(struct fuse_iqueue *fiq)
> +{
> +	struct virtio_fs *fs = fiq->priv;
> +	int i;
> +
> +	for (i = 0; i < fs->nvqs; i++) {
> +		struct virtio_fs_vq *fsvq = &fs->vqs[i];
> +
> +		spin_lock(&fsvq->lock);
> +		fsvq->connected = false;
> +		fsvq->aborted = true;
> +		spin_unlock(&fsvq->lock);
> +	}
> +}

[Severity: High]
Will setting fsvq->aborted = true here without clearing it when queues are
restarted break subsequent mounts of the same virtio-fs filesystem?

A struct virtio_fs instance and its fs->vqs[] array persist across mounts
for the lifetime of the virtio_device. On a normal unmount,
virtio_kill_sb() -> virtio_fs_conn_destroy() -> fuse_conn_destroy() calls
fuse_chan_abort(fc->chan, false):

fs/fuse/inode.c:fuse_conn_destroy() {
	...
	fuse_chan_abort(fc->chan, false);
	fuse_chan_wait_aborted(fc->chan);
	...
}

Because fch->connected is still set during a normal unmount,
fuse_chan_abort() invokes fiq->ops->abort(fiq) -> virtio_fs_abort(fiq),
which sets fsvq->aborted = true on every queue in fs->vqs[].

When the same virtio-fs tag is mounted again, virtio_fs_get_tree() ->
virtio_fs_fill_super() calls virtio_fs_start_all_queues(fs), which sets
fsvq->connected = true but never clears fsvq->aborted:

fs/fuse/virtio_fs.c:virtio_fs_start_all_queues() {
	struct virtio_fs_vq *fsvq;
	int i;

	for (i = 0; i < fs->nvqs; i++) {
		fsvq = &fs->vqs[i];
		spin_lock(&fsvq->lock);
		fsvq->connected = true;
		spin_unlock(&fsvq->lock);
	}
}

On the new mount, won't virtio_fs_requests_done_work() see fsvq->aborted ==
true and drop all completions (starting with FUSE_INIT from
fuse_send_init()), leaking req->argbuf and hanging the mount?

[Severity: High]
What happens to requests on fsvq->queued_reqs when virtio_fs_abort() runs?

When a virtqueue is full, virtio_fs_enqueue_req() drops fsvq->lock and
returns -ENOSPC without adding req to fpq->processing. Both
virtio_fs_send_req() and virtio_fs_request_dispatch_work() then re-acquire
fsvq->lock and place req onto fsvq->queued_reqs without checking
fsvq->connected:

fs/fuse/virtio_fs.c:virtio_fs_send_req() {
	...
	fsvq = &fs->vqs[queue_id];
	ret = virtio_fs_enqueue_req(fsvq, req, false, GFP_ATOMIC);
	if (ret < 0) {
		if (ret == -ENOSPC) {
			/*
			 * Virtqueue full. Retry submission from worker
			 * context as we might be holding fc->chan->bg_lock.
			 */
			spin_lock(&fsvq->lock);
			list_add_tail(&req->list, &fsvq->queued_reqs);
			inc_in_flight_req(fsvq);
			spin_unlock(&fsvq->lock);
			return;
		}
	...
}

Because FR_PENDING was already cleared in virtio_fs_send_req() and req is
not on fpq->processing, fuse_chan_abort() will not find these requests. In
addition, a racing virtio_fs_send_req() that saw fsvq->connected == true
inside virtio_fs_enqueue_req() and got -ENOSPC can acquire fsvq->lock after
virtio_fs_abort() has finished and still add req to fsvq->queued_reqs.

Since virtio_fs_abort() does not drain fsvq->queued_reqs or schedule
fsvq->dispatch_work (which is otherwise only scheduled from
virtio_fs_requests_done_work() when completions arrive), won't requests left
on fsvq->queued_reqs when the host stalls never be ended, hanging tasks in
request_wait_answer(), fuse_chan_wait_aborted(), and
virtio_fs_drain_queue()?

[ ... ]

விரும்பியபடி, இيمெயில் பதிலை மேலே உருவாக்கியுள்ளேன்.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020135350.7751-1-q.h.hack.winter@gmail.com?part=1