Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 (3) · Medium: 1 · Low: 1
- [Medium] The newly published `->admin_cmd_exec` transport op has no
lifecycle precondition check or documentation:…
- [Low] The kernel-doc added for `@admin_cmd_exec` in
include/linux/virtio_config.h states no calling-context constraint,…
Pre-existing issues:
- [High] Free-while-in-flight / use-after-free on the admin-command kick-
failure path.
- [High] The admin-command path accesses `admin_vq->info` with no
synchronization against admin-queue teardown, and the newly published…
- [High] `struct virtio_admin_cmd::result_sg_size`, now part of the
generic `->admin_cmd_exec` contract, is filled in…
--
virtio: Add config_op for admin commands
This adds a new optional admin_cmd_exec callback to struct
virtio_config_ops and points it at vp_modern_admin_cmd_exec() in both
modern virtio-pci config op tables, so that virtio device drivers can
issue admin commands through the transport.
No Link: tags in the commit header.
A few questions about the newly published op below.
> diff --git a/drivers/virtio/virtio_pci_modern.c
> b/drivers/virtio/virtio_pci_modern.c
> index dab63b044a312..3b2515e29728a 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -1248,6 +1248,7 @@ static const struct virtio_config_ops
> virtio_pci_config_nodev_ops = {
> .get_shm_region = vp_get_shm_region,
> .disable_vq_and_reset = vp_modern_disable_vq_and_reset,
> .enable_vq_after_reset = vp_modern_enable_vq_after_reset,
> + .admin_cmd_exec = vp_modern_admin_cmd_exec,
> };
[Severity: Medium]
Now that any bound driver can reach this path, is there anything that keeps
admin_vq.info valid at the entry point? virtqueue_exec_admin_cmd() starts
with:
vq = admin_vq->info->vq;
if (!vq)
return -EIO;
The guard tests the inner vq pointer, so it does not catch info == NULL.
The only precondition tested by vp_modern_admin_cmd_exec() is:
if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
return -EOPNOTSUPP;
That feature bit is set by vp_finalize_features() before the driver's probe
runs and it stays set across del_vqs(), while vp_del_vq() in
virtio_pci_common.c does:
if (vp_is_avq(vq->vdev, vq->index)) {
cancel_work_sync(&vp_dev->admin_vq.work);
vp_dev->admin_vq.info = NULL;
}
vp_dev->del_vq(info);
kfree(info);
So admin_vq.info is NULL before find_vqs() creates the admin queue, on the
find_vqs() error path, and after del_vqs(). Can a call made outside the
find_vqs()..del_vqs() window dereference NULL here?
The virtio_net consumers added later in this series do appear to stay inside
that window, so this may not be reachable in-tree today. Would it still be
worth checking admin_vq->info and spelling out the required device/VQ state
in the new kernel-doc?
[Severity: High]
This isn't a bug introduced by this patch, but the kick-failure path in
virtqueue_exec_admin_cmd() looks like it leaves a descriptor in flight:
mutex_lock(&admin_vq->lock);
ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
...
if (!virtqueue_kick(vq))
goto unlock_err;
...
unlock_err:
mutex_unlock(&admin_vq->lock);
return -EIO;
virtqueue_kick() returns false when the vring is broken, and
virtio_break_device() can be called asynchronously, for example from
virtio_pci_remove():
if (!pci_device_is_present(pci_dev))
virtio_break_device(&vp_dev->vdev);
so the state can change between the virtqueue_is_broken() pre-check and the
kick. The descriptor added by virtqueue_add_sgs() is not reclaimed on that
path, and vp_modern_admin_cmd_exec() then frees the buffers it published:
err_cmd_exec:
kfree(va_hdr);
err_alloc:
kfree(va_status);
return ret;
The token handed to virtqueue_add_sgs() is the caller's struct
virtio_admin_cmd, which goes out of scope when the caller returns. When the
following reset runs vp_modern_avq_cleanup():
while ((cmd = virtqueue_detach_unused_buf(vq))) {
cmd->ret = -EIO;
complete(&cmd->completion);
}
does this write through a stale token belonging to another task's stack
frame, and can the device still DMA into the freed, device-writable
va_status? Should the error path detach the descriptor (or defer the frees)
before this op is exposed to more drivers?
[Severity: High]
This is a pre-existing issue, but the admin queue accesses do not share a
lock with admin queue teardown, and this patch widens the set of contexts
that can submit commands without adding a serialization rule.
Reader side, with no lock held:
virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
vq = admin_vq->info->vq;
if (!vq)
return -EIO;
}
virtio_pci_modern.c:vp_modern_avq_cleanup() {
vq = vp_dev->admin_vq.info->vq;
if (!vq)
return;
while ((cmd = virtqueue_detach_unused_buf(vq))) {
}
vp_modern_avq_work() also reads admin_vq->info->vq before taking the mutex.
Writer side, holding only vp_dev->lock for the list removal:
virtio_pci_common.c:vp_del_vq() {
if (vp_is_avq(vq->vdev, vq->index)) {
cancel_work_sync(&vp_dev->admin_vq.work);
vp_dev->admin_vq.info = NULL;
}
vp_dev->del_vq(info);
kfree(info);
}
Since admin_vq->lock is not held by the cleanup or the teardown, can a
command submitted from an unrelated driver context concurrently with
del_vqs() or reset dereference NULL or the freed struct
virtio_pci_vq_info, and then operate on a destroyed vring?
The concrete unsynchronized submitter today is the existing
virtio_pci_admin_* path used by virtio-vfio-pci, which reaches a PF admin
queue while the PF's virtio driver can be unbound independently. Should the
new kernel-doc carry an analogue of the @reset rule about not running in
parallel with virtqueues being added or removed?
>
> static const struct virtio_config_ops virtio_pci_config_ops = {
> @@ -1268,6 +1269,7 @@ static const struct virtio_config_ops
> virtio_pci_config_ops = {
> .get_shm_region = vp_get_shm_region,
> .disable_vq_and_reset = vp_modern_disable_vq_and_reset,
> .enable_vq_after_reset = vp_modern_enable_vq_after_reset,
> + .admin_cmd_exec = vp_modern_admin_cmd_exec,
> };
>
> /* the PCI probing function */
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 69f84ea85d71a..e36a32e0a20c8 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -108,6 +108,10 @@ struct virtqueue_info {
> * Returns 0 on success or error status
> * If disable_vq_and_reset is set, then enable_vq_after_reset must also be
> * set.
> + * @admin_cmd_exec: Execute an admin VQ command (optional).
> + * vdev: the virtio_device
> + * cmd: the command to execute
> + * Returns 0 on success or error status
[Severity: Low]
Could this kernel-doc also state the calling context? The implementation
sleeps in several places: two kzalloc_obj() GFP_KERNEL allocations in
vp_modern_admin_cmd_exec(), and in virtqueue_exec_admin_cmd():
mutex_lock(&admin_vq->lock);
ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
...
wait_for_completion(&cmd->completion);
The wait has no timeout and is only woken by vp_modern_avq_work() or by
vp_modern_avq_cleanup() on reset.
The header of this struct is otherwise explicit about context, for example
that @get/@set, @get_status/@set_status and @get_features/@finalize_features
are not safe from atomic context, and that a device must not be reset from
its vq or config callbacks.
No in-tree caller violates this today. Would it help to note that
admin_cmd_exec may sleep, must be called from process context, and can block
until the device completes the command or the device is reset?
[Severity: High]
This is a pre-existing issue, but the cmd->result_sg_size field that this
contract now exposes to arbitrary drivers is filled straight from the
device-written used ring length. In vp_modern_avq_work():
while ((cmd = virtqueue_get_buf(vq, &len))) {
if (len < status_size)
cmd->result_sg_size = 0;
else
cmd->result_sg_size = len - status_size;
virtio_ring does not clamp that length against the posted buffer; in
virtqueue_get_buf_ctx_split() and virtqueue_get_buf_ctx_packed() only the
descriptor id is range-checked while the length is taken verbatim:
*len = vring_read_split_used_len(vq, last_used);
Is result_sg_size therefore fully device-controlled and able to exceed the
caller's result scatterlist? The existing consumer already trusts it:
virtio_pci_admin_dev_parts_get() does *res_size = cmd.result_sg_size; and
drivers/vfio/pci/virtio/migrate.c does buf->length = res_size; for a buffer
whose real size is ctx_size, which the migration file then reads from.
Should the transport clamp result_sg_size to the total length of the
caller's result_sg before completing the command, and should the new
kernel-doc say that this field is untrusted device input?
> */
> struct virtio_config_ops {
> void (*get)(struct virtio_device *vdev, unsigned offset,
> @@ -137,6 +141,8 @@ struct virtio_config_ops {
> struct virtio_shm_region *region, u8 id);
> int (*disable_vq_and_reset)(struct virtqueue *vq);
> int (*enable_vq_after_reset)(struct virtqueue *vq);
> + int (*admin_cmd_exec)(struct virtio_device *vdev,
> + struct virtio_admin_cmd *cmd);
> };
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com
