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

PATCH In review Thorsten Blum 4 participants 8 messages Oct 16 – Nov 11

Subsystems (4)

Crypto API virtio core Hardware random number generator core virtio rng driver

lore

Share & tools

lore

Thorsten Blum

author

2026-10-16 10:37 UTC #0 lore raw

PATCH 1/1 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. 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 [email protected]

Signed-off-by Thorsten Blum <[email protected]>

---
 drivers/char/hw_random/virtio-rng.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/char/hw_random/virtio-rng.c +7 -0

index 5e83ffa105e4..a9ff520975a7 100644
@@ -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);
 }

[email protected]

AI review

2026-10-16 10:47 UTC #1 lore raw

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 --

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 <[email protected]>

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.

Quoted 4 lines

drivers/char/hw_random/virtio-rng.c +0 -0
index f9982217247dd..45d87c0614a5e 100644

[ ... ]

drivers/char/hw_random/virtio-rng.c static void remove_common(struct virtio_device *vdev) · line 208

Quoted 11 lines

drivers/char/hw_random/virtio-rng.c +5 -0

@@ -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?

Quoted 2 lines

drivers/char/hw_random/virtio-rng.c +0 -0
 	vi->data_idx = 0;
complete(&vi->have_data);

[ ... ]

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

PrasannaKumar Muralidharan

2026-10-18 17:00 UTC #2 lore raw

In reply to Thorsten Blum

Hi Thorsten,

Quoted 31 lines · 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 [email protected]
Signed-off-by Thorsten Blum <[email protected]>
---
 drivers/char/hw_random/virtio-rng.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/char/hw_random/virtio-rng.c +1 -0
index 5e83ffa105e4..a9ff520975a7 100644
@@ -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;
 }

drivers/char/hw_random/virtio-rng.c static void remove_common(struct virtio_device *vdev) · line 206

Quoted 8 lines

drivers/char/hw_random/virtio-rng.c +5 -0

@@ -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;
+       }
+

The virtio_reset_device() is always called before setting vdev->priv to NULL. So the virtio device is already quiesced, no need to do early return. Isn't it?

drivers/char/hw_random/virtio-rng.c static void remove_common(struct virtio_device *vdev) · line 219

Quoted 12 lines

drivers/char/hw_random/virtio-rng.c +1 -0

        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);
 }

The rest of the hunks are fine.

Thanks,
PrasannaKumar

Thorsten Blum

author

2026-10-18 20:37 UTC #3 lore raw

Hi PrasannaKumar,

Quoted 37 lines · PrasannaKumar Muralidharan

Hi Thorsten,
Quoted 34 lines · 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 [email protected]
Signed-off-by Thorsten Blum <[email protected]>
---
 drivers/char/hw_random/virtio-rng.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/char/hw_random/virtio-rng.c +1 -0
index 5e83ffa105e4..a9ff520975a7 100644
@@ -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;
 

Quoted 9 lines

drivers/char/hw_random/virtio-rng.c static void remove_common(struct virtio_device *vdev) · line 206
Quoted 5 lines
drivers/char/hw_random/virtio-rng.c +5 -0
+       if (!vi) {
+               virtio_reset_device(vdev);
+               return;
+       }
+
The virtio_reset_device() is always called before setting vdev->priv to NULL. So the virtio device is already quiesced, no need to do early return. Isn't it?

The check is still needed.

For example, during hibernation or suspend, .freeze (virtrng_freeze() -> remove_common()) frees vi and sets vdev->priv to NULL.

On resume, .restore (virtrng_restore()) calls probe_common(), which could fail and leave vdev->priv NULL. A later .freeze or .remove calls remove_common() again, which would dereference NULL without the check.

Best regards,
Thorsten

PrasannaKumar Muralidharan

Reviewed-by

2026-10-19 13:01 UTC #4 lore raw

Quoted 51 lines · Thorsten Blum

Hi PrasannaKumar,
Quoted 37 lines · PrasannaKumar Muralidharan
Hi Thorsten,
Quoted 34 lines · 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 [email protected]
Signed-off-by Thorsten Blum <[email protected]>
---
 drivers/char/hw_random/virtio-rng.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/char/hw_random/virtio-rng.c +1 -0
index 5e83ffa105e4..a9ff520975a7 100644
@@ -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;
 
Quoted 9 lines
drivers/char/hw_random/virtio-rng.c static void remove_common(struct virtio_device *vdev) · line 206
Quoted 5 lines
drivers/char/hw_random/virtio-rng.c +5 -0
+       if (!vi) {
+               virtio_reset_device(vdev);
+               return;
+       }
+
The virtio_reset_device() is always called before setting vdev->priv to NULL. So the virtio device is already quiesced, no need to do early return. Isn't it?

Quoted 8 lines

The check is still needed.
For example, during hibernation or suspend, .freeze (virtrng_freeze() -> remove_common()) frees vi and sets vdev->priv to NULL.
On resume, .restore (virtrng_restore()) calls probe_common(), which could fail and leave vdev->priv NULL. A later .freeze or .remove calls remove_common() again, which would dereference NULL without the check.

Yes, it makes sense.

Quoted 2 lines

Best regards,
Thorsten

Change looks good to me.

Feel free to add:

Reviewed-by PrasannaKumar Muralidharan <[email protected]>.

Thanks,
PrasannaKumar

Herbert Xu

maintainer

2026-11-06 11:12 UTC #5 lore raw

In reply to Thorsten Blum

Quoted 10 lines · 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.

Quoted 6 lines

Fixes 08e53fbdb85c ("virtio-rng: support multiple virtio-rng devices")
Cc [email protected]
Signed-off-by Thorsten Blum <[email protected]>
---
 drivers/char/hw_random/virtio-rng.c | 7 +++++++
 1 file changed, 7 insertions(+)

Please respond to this comment:

https://sashiko.dev/#/patchset/20261016103640.4989-2-blum%40kernel.org

Thanks,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Thorsten Blum

author

2026-11-06 11:30 UTC #6 lore raw

Hi Herbert,

Quoted 18 lines · Herbert Xu

Quoted 10 lines · 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.
Quoted 6 lines
Fixes 08e53fbdb85c ("virtio-rng: support multiple virtio-rng devices")
Cc [email protected]
Signed-off-by Thorsten Blum <[email protected]>
---
 drivers/char/hw_random/virtio-rng.c | 7 +++++++
 1 file changed, 7 insertions(+)

Quoted 3 lines

Please respond to this comment:
https://sashiko.dev/#/patchset/20261016103640.4989-2-blum%40kernel.org

I think the issue raised by Sashiko is valid, but also pre-existing and could therefore be fixed in a separate follow-up patch.

Thanks,
Thorsten

Herbert Xu

maintainer

2026-11-11 11:12 UTC #7 lore raw

In reply to Thorsten Blum

Quoted 10 lines · 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.

Quoted 6 lines

Fixes 08e53fbdb85c ("virtio-rng: support multiple virtio-rng devices")
Cc [email protected]
Signed-off-by Thorsten Blum <[email protected]>
---
 drivers/char/hw_random/virtio-rng.c | 7 +++++++
 1 file changed, 7 insertions(+)

Patch applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt