Re: [PATCH v3 3/6] remoteproc: implement synchronize_cbs() for virtio devices

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] idr_find() is called without rcu_read_lock() in preemptible contexts, leading to potential use-after-free of radix tree nodes.
- [High] The virtqueue pointer is published before its initialization is complete and without necessary memory barriers, exposing uninitialized memory to concurrent readers.
- [High] The added SRCU synchronization leaves the rvring structure vulnerable to Use-After-Free during device teardown.
- [High] Error path in rp_find_vq() erroneously removes the vring IDR entry without clearing it, permanently breaking the vring and causing a double-free on teardown.
- [High] rproc_add() error path causes a use-after-free and a double-free of the rproc structure.
- [High] Missing cancel_work_sync() for the crash handler work item before freeing the rproc struct.
--

commit c12071a358959f8d763c10c9e9b78779109dae3f
Author: Karl Mehltretter [off-list ref]

remoteproc: implement synchronize_cbs() for virtio devices

Platform drivers invoke rproc_vq_interrupt() from hard-IRQ handlers,
threaded handlers, and work items. Because rpmsg callbacks may sleep,
the virtio core's synchronize_rcu() fallback does not synchronize with
callbacks across all these contexts. A device reset can therefore
complete while a callback is still running.

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of a
use-after-free here if the crash handler work item is still pending?

rproc_alloc() initializes the rproc->crash_handler work item. If the remote
processor is deleted and freed, neither rproc_del() nor rproc_type_release()
calls cancel_work_sync(&rproc->crash_handler). If the work executes after this
structure is freed, the thread will dereference freed memory.

[Severity: High]
This is a pre-existing issue, but can the error path in rproc_add() cause a
use-after-free and a subsequent double-free?

If device_add(dev) fails, the error path erroneously calls put_device(dev):

drivers/remoteproc/remoteproc_core.c:rproc_add() {
    ...
    ret = device_add(dev);
    if (ret < 0) {
        put_device(dev);
        goto rproc_remove_cdev;
    }
    ...
rproc_remove_cdev:
    rproc_char_device_remove(rproc);
    return ret;
}

Since the initial refcount is 1, put_device() drops it to 0, immediately
freeing the rproc structure. Calling rproc_char_device_remove(rproc) then
accesses the cdev within the already freed memory. Returning the error to the
caller typically results in rproc_free() being called, causing a double-free.

 	idr_destroy(&rproc->notifyids);
 
 	if (rproc->index >= 0)

[ ... ]

[ ... ]

[Severity: High]
This is a pre-existing issue, but might calling idr_find() here without
standard RCU read-side protection risk a use-after-free of internal radix tree
nodes?

This function runs in a preemptible context (threaded IRQ handler). idr_find()
relies on rcu_read_lock() for safe lockless traversal. While this patch wraps
it in srcu_read_lock(), SRCU does not block standard RCU grace periods.

If a concurrent idr_remove() runs (e.g., during device teardown) and a standard
RCU grace period completes while the thread is preempted inside idr_find(), the
internal nodes could be freed and then dereferenced when the thread resumes.

[Severity: High]
This isn't a bug introduced by this patch, but does the added SRCU
synchronization leave the rvring structure vulnerable to a use-after-free
during device teardown?

While rproc_vq_interrupt() is now protected by an SRCU critical section, there
does not appear to be a corresponding synchronize_srcu() in the platform device
teardown path:

drivers/remoteproc/remoteproc_virtio.c:rproc_virtio_remove() {
    ...
    for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
        rvring = &rvdev->vring[id];
        rproc_free_vring(rvring);
    }
    ...
}

The IDR entries are removed, but the function returns without waiting for SRCU
readers. The core driver then cleans up devres, instantly freeing the rvdev
(which embeds rvring). If rproc_vq_interrupt() is preempted after locating
rvring but before accessing rvring->vq, it will dereference freed memory upon
resumption.

[Severity: High]
This isn't a bug introduced by this patch, but would publishing the virtqueue
pointer here without a release barrier expose uninitialized memory to
concurrent readers?

In vring_new_virtqueue(), vq is allocated and initialized, but vq->priv is not
set. The pointer is then published to concurrent readers via WRITE_ONCE.

Since WRITE_ONCE does not provide release semantics on weakly ordered
architectures, a concurrent rproc_vq_interrupt() could observe a non-NULL vq
before vq->priv is set or before the internal state of vq is visible. The
handler would then call vring_interrupt(0, vq) on an improperly initialized
structure.

[Severity: High]
This is a pre-existing issue, but could the error path in rp_find_vq()
permanently break the vring and cause a double-free on teardown?

If vring_new_virtqueue() fails, the error path calls rproc_free_vring(rvring),
which executes idr_remove(), but it does not reset rvring->notifyid:

drivers/remoteproc/remoteproc_virtio.c:rp_find_vq() {
    ...
    vq = vring_new_virtqueue(...);
    if (!vq) {
        dev_err(dev, "vring_new_virtqueue %s failed\n", name);
        rproc_free_vring(rvring);
        return ERR_PTR(-ENOMEM);
    }
    ...
}

If the driver successfully binds later, rp_find_vq() might succeed, but the
notifyid is no longer in the IDR, silently breaking interrupts. Furthermore,
when the device is unbound, rproc_virtio_remove() loops over all vrings and
calls rproc_free_vring() again, executing a second idr_remove() for the same
notifyid. If the ID was reused by another vring, its entry will be erroneously
deleted.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908053817.26065-1-kmehltretter@gmail.com?part=3