[PATCH] virtio: virtio_ring: prevent potential null dereference on per-desc state

[PATCH] virtio: virtio_ring: prevent potential null dereference on per-desc state

[Roi L]

When accessing the per-descriptor state structure, we could overflow
desc_state[] because id can be accessed by the host and the guest
at the same time, as described by the packed implementation.

'commit 32fe1de5c124 ("virtio_ring: Add READ_ONCE annotations for device-writable fields")'

The commit mentions these are legitimate races, so I assume id might
get corrupted? "id" is actually just ""last_used"" in some contexts,
and at the path where last_used gets manipulated, KCSAN reports these races.

I'm not sure if this is the right approach though, maybe we should handle it
differently other than just return?

Signed-off-by: Roi L <roeilev321_@outlook.com>
---
 drivers/virtio/virtio_ring.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b438dc2ce1b8..1d99b75f3bd7 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2025,6 +2025,8 @@ static void detach_buf_packed_in_order(struct vring_virtqueue *vq,
 	unsigned int i, curr;
 
 	state = &vq->packed.desc_state[id];
+	if (unlikely(!state))
+		return;
 
 	/* Clear data ptr. */
 	state->data = NULL;
-- 
2.55.0

Re: [PATCH] virtio: virtio_ring: prevent potential null dereference on per-desc state

[Michael S. Tsirkin]

On Tue, Aug 04, 2026 at 10:21:00PM +0300, Roi L wrote:
> When accessing the per-descriptor state structure, we could overflow
> desc_state[] because id can be accessed by the host and the guest
> at the same time, as described by the packed implementation.

it is validated:

        id = vring_read_packed_desc_id(vq, last_used);
        *len = vring_read_packed_desc_len(vq, last_used);

        if (unlikely(id >= num)) {
                BAD_RING(vq, "id %u out of range\n", id);
                return NULL;        
        }



> 'commit 32fe1de5c124 ("virtio_ring: Add READ_ONCE annotations for device-writable fields")'
> 
> The commit mentions these are legitimate races, so I assume id might
> get corrupted? "id" is actually just ""last_used"" in some contexts,
> and at the path where last_used gets manipulated, KCSAN reports these races.
> 
> I'm not sure if this is the right approach though, maybe we should handle it
> differently other than just return?
> 
> Signed-off-by: Roi L <roeilev321_@outlook.com>

I don't understand what all this means.

> ---
>  drivers/virtio/virtio_ring.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index b438dc2ce1b8..1d99b75f3bd7 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2025,6 +2025,8 @@ static void detach_buf_packed_in_order(struct vring_virtqueue *vq,
>  	unsigned int i, curr;
>  
>  	state = &vq->packed.desc_state[id];
> +	if (unlikely(!state))
> +		return;


I don't see how it can be null even if id is out of range.

>  	/* Clear data ptr. */
>  	state->data = NULL;
> -- 
> 2.55.0