[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

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

[Qihang]

Hi Sashiko,

Thanks for the review.  I did not extend that patch: tagging
completions was the wrong layer, and it introduced the sticky-state
problem you found in #2.  I replaced it with a v2 patch, which
prevents the request from becoming device-visible in the first place:

  [PATCH v2] fuse: virtio-fs: stop new requests before draining queues

One new per-vq flag, fsvq->aborting, is set by
virtio_fs_stop_new_requests() before the first drain in
virtio_fs_conn_destroy().  It is set per queue, not with a global
barrier, and that is enough: each queue is drained after its own flag
is set, and for a request that reaches the device, admission,
virtqueue_add_sgs() and the in_flight increment happen under the same
fsvq->lock.  During teardown a request therefore has only two
possible states:

  1. it observes aborting under fsvq->lock and is rejected with
     -ENOTCONN before virtqueue_add_sgs(); or
  2. it passes admission, is added to the virtqueue and is covered by
     in_flight - either incremented under the same lock on first
     submission, or already counted when it was queued - so the
     drain waits for it.

There is no state observable by the drain in which the request is
device-visible but not accounted for by in_flight.

FUSE_DESTROY is the one request intentionally submitted after the
first drain: it is the teardown request itself, issued with
args.force, and fuse_simple_request() waits for its completion before
fuse_chan_abort() is called, so its lifetime cannot overlap the
abort.

#1 (post-drain request -> DMA UAF; Sashiko's "in-flight at abort")

I agree the normal entry points cannot reach fuse_chan_abort() with an
in-flight request: virtio-fs has no userspace /dev/fuse fd, disables
fusectl (no_control), and sets no_force_umount.  The only path is
fuse_conn_destroy(), preceded by a drain.

That drain only covers requests counted when it starts.  A request
submitted after it returns is added to the virtqueue and counted, and
is then freed by fuse_chan_abort() while the device still owns the
descriptors:

  CPU0 (umount)                    CPU1
  drain returns (in_flight == 0)
                                   enqueue_req()
                                     virtqueue_add_sgs()
                                     inc_in_flight_req() -> 1
  fuse_conn_destroy()
    fuse_send_destroy()
    fuse_chan_abort()
      ends and frees the request
                                   device still DMA-writes into the
                                   freed request and argbuf

This is the reachable variant of #1, and what the patch closes.
Unpatched, I could land the post-drain submission in the virtqueue
and get a KASAN slab-use-after-free in
virtio_fs_requests_done_work(), with umount hanging afterwards;
patched, the same submission returns -ENOTCONN and umount completes.
(Full dmesg is in the reproducer.)

#2 (aborted never cleared, second mount's FUSE_INIT dropped)

The v2 patch has no aborted/completion-suppression state.  aborting is
reinitialized by virtio_fs_start_all_queues() on every mount, so it
cannot leak into a subsequent mount.  I verified mount/read/umount
across repeated remounts; FUSE_INIT completed normally.

#3 (queued_reqs not handled by abort/drain)

Requests on the dispatch queue are already accounted for by
in_flight, so the drain covers them even though they have not reached
the virtqueue.  If the dispatch worker retries after teardown starts,
aborting makes virtio_fs_enqueue_req() return -ENOTCONN before
virtqueue_add_sgs(); the retry passes in_flight=true, so it is not
counted twice, and flush_work(dispatch_work) then waits for the
worker to finish.

Measured with a forced -ENOSPC so the request really takes the queued
path:

  in_flight seen=1 after 2 ms
  VF_INV: q=requests.0 in_flight=0 queued=0 connected=1 aborting=1
  injected STATFS returned -107
  VF_INV: q=requests.0 in_flight=0 queued=0 connected=0 aborting=1

#4 (argbuf leak on aborted requests)

There is no skipped completion, so there is no new argbuf owner.
Rejected requests free argbuf in the existing error path of
virtio_fs_enqueue_req(), the same way the !connected path does, and
the stock abort path is untouched.

I also checked the two teardown interactions.  remove() only narrows
admission: connected=false is set under fsvq->lock before the
virtqueues are torn down, and enqueue_req() checks connected before
dereferencing fsvq->vq, so it cannot race into a cleaned-up
virtqueue.  For a new mount, sget_fc() keeps finding the extant
superblock until SB_DEAD is set, and SB_DEAD is set after kill_sb()
returns; conn_destroy() runs inside kill_sb(), so start_all_queues()
cannot clear aborting during teardown.  Both were confirmed by
running umount against unbind and against a concurrent mount.

Tested with KASAN, prove-locking, DEBUG_LIST, DEBUG_SPINLOCK and
DEBUG_MUTEXES: 23 mount/umount cycles with read/write load, the A/B
above, and the queued/remove/mount window tests; no WARN or KASAN
reports.  The tests used one normal request queue plus the
high-priority queue (nvqs=2); the multi-queue loop is the same
per-queue code but was not exercised.  The test kernels are
instrumented (injected STATFS, widened teardown window), so their
timings only show ordering.  A FUSE_DESTROY that is never answered
still blocks umount; that is pre-existing.

v2 also words the /dev/fuse point precisely: virtio-fs exposes no
userspace fd at all, and no_control disables fusectl.

Thanks,
Qihang