Re: [PATCH] hwrng: virtio - prevent use-after-free after a failed restore

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] virtio-rng: data race on data_avail leads to deadlock
--

commit 2904ec0356e25704559279a6336e7fed895c58d2
Author: Thorsten Blum <blum@kernel.org>

hwrng: virtio - prevent use-after-free after a failed restore

remove_common() frees the driver data during freeze, but does not clear
vdev->priv, which points to freed memory. Clear vdev->priv on the error
cleanup path and before freeing the driver data in remove_common(). Also
reset the device and return early from remove_common() if vdev->priv is
already NULL.

> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index f9982217247dd..45d87c0614a5e 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
[ ... ]
> @@ -198,6 +199,11 @@ static void remove_common(struct virtio_device *vdev)
>  {
>  	struct virtrng_info *vi = vdev->priv;
>  
> +	if (!vi) {
> +		virtio_reset_device(vdev);
> +		return;
> +	}
> +
>  	vi->hwrng_removed = true;
>  	vi->data_avail = 0;

[Severity: High]
Does this code race with a reader and cause a deadlock?

In copy_data() in drivers/char/hw_random/virtio-rng.c, vi->data_avail is read,
memcpy() is called, and then size is subtracted:

drivers/char/hw_random/virtio-rng.c:copy_data() {
	...
	memcpy(buf, vi->data + idx, size);
	vi->data_idx += size;
	vi->data_avail -= size;
	if (vi->data_avail == 0)
		request_entropy(vi);
	return size;
}

If remove_common() sets vi->data_avail to 0 before the subtraction, will this
cause an unsigned integer underflow, leaving vi->data_avail as a huge value?

Since vi->data_avail != 0, copy_data() would skip calling request_entropy().
virtio_read() would then consume the single completion from remove_common().

Seeing a huge data_avail instead of 0, virtio_read() would continue looping
and eventually block forever in wait_for_completion_killable() because no
pending entropy request will trigger a new completion:

drivers/char/hw_random/virtio-rng.c:virtio_read() {
	...
	while (size != 0) {
		/* data_avail is 0 but a request is pending */
		ret = wait_for_completion_killable(&vi->have_data);
	...
}

Could this cause hwrng_unregister() in remove_common() to block forever
waiting for the reader to drop its reference, deadlocking device removal?

>  	vi->data_idx = 0;
>  	complete(&vi->have_data);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016103640.4989-2-blum@kernel.org?part=1