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

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

[Thorsten Blum]

remove_common() frees the driver data during freeze, but does not clear
vdev->priv, which points to freed memory. If probe_common() then fails
during restore, a later remove or freeze can dereference the dangling
pointer.

Clear vdev->priv on the probe_common() 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 for a
safe teardown after a failed restore.

Fixes: 08e53fbdb85c ("virtio-rng: support multiple virtio-rng devices")
Cc: stable@vger.kernel.org
Signed-off-by: Thorsten Blum <blum@kernel.org>
---
 drivers/char/hw_random/virtio-rng.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 5e83ffa105e4..a9ff520975a7 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -190,6 +190,7 @@ static int probe_common(struct virtio_device *vdev)
 err_find:
 	ida_free(&rng_index_ida, index);
 err_ida:
+	vdev->priv = NULL;
 	kfree(vi);
 	return err;
 }
@@ -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;
 	vi->data_idx = 0;
@@ -207,6 +213,7 @@ static void remove_common(struct virtio_device *vdev)
 	virtio_reset_device(vdev);
 	vdev->config->del_vqs(vdev);
 	ida_free(&rng_index_ida, vi->index);
+	vdev->priv = NULL;
 	kfree(vi);
 }
 

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

[sashiko-bot]

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