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 <[email protected]> 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/[email protected]?part=1
