Skip to contentSkip to conversation

virtio-rng entropy leak reporting feature

RFC PATCH NAK Babis Chalios 3 participants 9 messages Jan 31, 2023 – Mar 20, 2023

Subsystems (4)
Crypto API virtio core Hardware random number generator core virtio rng driver
lore
Share & tools
lore

Babis Chalios

author
#0 lore raw

v2 cover virtio-rng entropy leak reporting feature

Recently, a proposal has been published [1] for a new feature in the VirtIO RNG device which will allows the device to report "entropy leaks" to the guest VM. Such an event occurs when, for example, we take a VM snapshot, or when we restore a VM from a snapshot.

The feature allows the guest to request for certain operations to be performed upon an entropy leak event. When such an event occurs, the device will handle the requests and add the request buffers to the used queue. Adding these buffers to the used queue operates as a notification towards the guest about the entropy leak event.

The proposed changes describe two types of requests that can be
performed: (1) fill a buffer in guest memory with random bytes and (2) perform a memory copy between two buffers in guest memory.

The mechanism provides similar functionality to Microsoft's Virtual Machine Generation ID and it can be used to re-seed the kernel's PRNG upon taking a VM snapshot or resuming from one. Additionally, it allows to (1) avoid the race-condition that exists with our VMGENID implementation, between the time a VM is resumed after a "leak event" and the handling of the ACPI notification before adding the new entropy. Finally, it allows building on top of it to provide a mechanism for notifying user-space about such events.

The first patch of this series, extends the current virtio-rng driver to implement the new feature and ensures that there is always a request to get some random bytes from the device in the event of an entropy leak and uses these bytes as entropy through the add_device_randomness.

The second patch adds a copy-on-leak command as well in the queue, implementating the idea of a generation counter that has previously been part of the VMGENID saga. It then exposes the value of the generation counter over a sysfs file. User-space can read, mmap and poll on the file in order to be notified about entropy leak events.

I have performed basic tests of the user-space interfaces using a Firecracker where I implemented virtio-rng with the proposed features. Instructions on how to replicate this can be found here:
https://github.com/bchalios/virtio-snapsafe-example

The patchset does not solve all problems. We do not define an API for other parts of the kernel to be able to use directly the new functionality (add commands to the queue), mainly because I 'm not sure what would the correct API be. I was toying with the idea of extending struct hwrng with two new hooks that would be implemented only by virtio-rng but I'm not sure I like it, so I am open to suggestions.

As a result of the above, the way we use the functionality to add new entropy, i.e. calling add_device_randomness, is as racy as the VMGENID case, since it relies on used buffers been handled by the virtio driver.

As for user-space, the mmap interface *is* race-free. Changes in the generation counter will be observable by user applications the moment VM vcpus resume. However, the poll interface isn't, sysfs_notify is being called as well when the virtio driver handles used buffers. I am not sure I have a solution for this last one.

Posting this, I hope we can resume the discussion about solving the above issues (or any other issue that I haven't thought of), especially with regards to providing a mechanism suitable for user-space notifications.

Cheers,
Babis

Changes in v2: fix kbuild warnings

Babis Chalios (2):
virtio-rng: implement entropy leak feature
virtio-rng: add sysfs entries for leak detection

 drivers/char/hw_random/virtio-rng.c | 372 +++++++++++++++++++++++++++-
 include/uapi/linux/virtio_rng.h     |   3 +
 2 files changed, 368 insertions(+), 7 deletions(-)
Signature · 3 lines
2.38.1

Amazon Spain Services sociedad limitada unipersonal, Calle Ramirez de Prado 5, 28045 Madrid. Registro Mercantil de Madrid . Tomo 22458 . Folio 102 . Hoja M-401234 . CIF B84570936
author
#1 lore raw

PATCH v2 1/2 virtio-rng: implement entropy leak feature

Implement the virtio-rng feature that allows a guest driver to request from the device to perform certain operations in the event of an "entropy leak", such as when taking a VM snapshot or restoring a VM from a snapshot. The guest can request one of two operations: (i) fill a buffer with random bytes, or (ii) perform a memory copy between two bytes.

The feature is similar to Microsoft's Virtual Machine Generation ID and it can be used to (1) avoid the race-condition that exists in our current VMGENID implementation, between the time vcpus are resumed and the ACPI notification is being handled and (2) provide mechanisms for notifying user-space about snapshot-related events.

This commit implements the protocol between guest and device. Additionally, it makes sure there is always a request for random bytes in the event of entropy leak in-flight. Once such an event is observed, the driver feeds these bytes to as entropy using add_device_randomness.

Keep in mind that this commit does not solve the race-condition issue, it adds fresh entropy whenever the driver handles the used buffer from the fill-on-leak request. In order to close the race window, we need to expose some API so that other kernel subsystems can request directly notifications from the device.

Signed-off-by Babis Chalios <[email protected]>
---
 drivers/char/hw_random/virtio-rng.c | 200 +++++++++++++++++++++++++++-
 include/uapi/linux/virtio_rng.h     |   3 +
 2 files changed, 196 insertions(+), 7 deletions(-)
drivers/char/hw_random/virtio-rng.c +193 -7
index a6f3a8a2aca6..154f68a1e326 100644
@@ -4,12 +4,12 @@
  *  Copyright (C) 2007, 2008 Rusty Russell IBM Corporation
  */
 
-#include <linux/err.h>
 #include <linux/hw_random.h>
 #include <linux/scatterlist.h>
 #include <linux/spinlock.h>
 #include <linux/virtio.h>
 #include <linux/virtio_rng.h>
+#include <linux/random.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 

@@ -18,6 +18,12 @@ static DEFINE_IDA(rng_index_ida);
 struct virtrng_info {
 	struct hwrng hwrng;
 	struct virtqueue *vq;
+	/* Leak queues */
+	bool has_leakqs;
+	struct virtqueue *leakq[2];
+	spinlock_t lock;
+	int active_leakq;
+
 	char name[25];
 	int index;
 	bool hwrng_register_done;

@@ -29,27 +35,159 @@ struct virtrng_info {
 	/* minimal size returned by rng_buffer_size() */
 #if SMP_CACHE_BYTES < 32
 	u8 data[32];
+	u8 leak_data[32];
 #else
 	u8 data[SMP_CACHE_BYTES];
+	u8 leak_data[SMP_CACHE_BYTES];
 #endif
 };
 
+/* Swaps the queues and returns the new active leak queue. */
+static struct virtqueue *swap_leakqs(struct virtrng_info *vi)
+{
+	vi->active_leakq = 1 - vi->active_leakq;
+	return vi->leakq[vi->active_leakq];
+}
+
+static struct virtqueue *get_active_leakq(struct virtrng_info *vi)
+{
+	return vi->leakq[vi->active_leakq];
+}
+
+static int add_fill_on_leak_request(struct virtrng_info *vi, struct virtqueue *vq, void *data, size_t len)
+{
+	struct scatterlist sg;
+	int ret;
+
+	sg_init_one(&sg, data, len);
+	ret = virtqueue_add_inbuf(vq, &sg, 1, data, GFP_KERNEL);
+	if (ret)
+		goto err;
+
+err:
+	return ret;
+}
+
+static int virtrng_fill_on_leak(struct virtrng_info *vi, void *data, size_t len)
+{
+	struct virtqueue *vq;
+	unsigned long flags;
+	int ret;
+
+	if (!vi->has_leakqs)
+		return -EOPNOTSUPP;
+
+	spin_lock_irqsave(&vi->lock, flags);
+
+	vq = get_active_leakq(vi);
+	ret = add_fill_on_leak_request(vi, vq, data, len);
+	if (ret)
+		virtqueue_kick(vq);
+
+	spin_unlock_irqrestore(&vi->lock, flags);
+
+	return ret;
+}
+
+static int add_copy_on_leak_request(struct virtrng_info *vi, struct virtqueue *vq,
+		void *to, void *from, size_t len)
+{
+	int ret;
+	struct scatterlist out, in, *sgs[2];
+
+	sg_init_one(&out, from, len);
+	sgs[0] = &out;
+	sg_init_one(&in, to, len);
+	sgs[1] = &in;
+
+	ret = virtqueue_add_sgs(vq, sgs, 1, 1, to, GFP_KERNEL);
+	if (ret)
+		goto err;
+
+err:
+	return ret;
+}
+
+static int virtrng_copy_on_leak(struct virtrng_info *vi, void *to, void *from, size_t len)
+{
+	struct virtqueue *vq;
+	unsigned long flags;
+	int ret;
+
+	if (!vi->has_leakqs)
+		return -EOPNOTSUPP;
+
+	spin_lock_irqsave(&vi->lock, flags);
+
+	vq = get_active_leakq(vi);
+	ret = add_copy_on_leak_request(vi, vq, to, from, len);
+	if (ret)
+		virtqueue_kick(vq);
+
+	spin_unlock_irqrestore(&vi->lock, flags);
+
+	return ret;
+}
+
+static void entropy_leak_detected(struct virtqueue *vq)
+{
+	struct virtrng_info *vi = vq->vdev->priv;
+	struct virtqueue *activeq;
+	unsigned int len;
+	unsigned long flags;
+	void *buffer;
+	bool kick_activeq = false;
+
+	spin_lock_irqsave(&vi->lock, flags);
+
+	activeq = get_active_leakq(vi);
+	/* Drain all the used buffers from the queue */
+	while ((buffer = virtqueue_get_buf(vq, &len)) != NULL) {
+		if (vq == activeq) {
+			pr_debug("%s: entropy leak detected!", vi->name);
+			activeq = swap_leakqs(vi);
+		}
+
+		if (buffer == vi->leak_data) {
+			add_device_randomness(vi->leak_data, sizeof(vi->leak_data));
+
+			/* Ensure we always have a pending request for random bytes on entropy
+			 * leak. Do it here, after we have swapped leak queues, so it gets handled
+			 * with the next entropy leak event.
+			 */
+			add_fill_on_leak_request(vi, activeq, vi->leak_data, sizeof(vi->leak_data));
+			kick_activeq = true;
+		}
+	}
+
+	if (kick_activeq)
+		virtqueue_kick(activeq);
+
+	spin_unlock_irqrestore(&vi->lock, flags);
+}
+
 static void random_recv_done(struct virtqueue *vq)
 {
 	struct virtrng_info *vi = vq->vdev->priv;
+	unsigned long flags;
 
+	spin_lock_irqsave(&vi->lock, flags);
 	/* We can get spurious callbacks, e.g. shared IRQs + virtio_pci. */
 	if (!virtqueue_get_buf(vi->vq, &vi->data_avail))
-		return;
+		goto unlock;
 
 	vi->data_idx = 0;
 
 	complete(&vi->have_data);
+
+unlock:
+	spin_unlock_irqrestore(&vi->lock, flags);
 }
 
 static void request_entropy(struct virtrng_info *vi)
 {
 	struct scatterlist sg;
+	unsigned long flags;
 
 	reinit_completion(&vi->have_data);
 	vi->data_avail = 0;

@@ -57,10 +195,12 @@ static void request_entropy(struct virtrng_info *vi)
 
 	sg_init_one(&sg, vi->data, sizeof(vi->data));
 
+	spin_lock_irqsave(&vi->lock, flags);
 	/* There should always be room for one buffer. */
 	virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_KERNEL);
 
 	virtqueue_kick(vi->vq);
+	spin_unlock_irqrestore(&vi->lock, flags);
 }
 
 static unsigned int copy_data(struct virtrng_info *vi, void *buf,

@@ -126,6 +266,40 @@ static void virtio_cleanup(struct hwrng *rng)
 	complete(&vi->have_data);
 }
 
+static int init_virtqueues(struct virtrng_info *vi, struct virtio_device *vdev)
+{
+	int ret = -ENOMEM, total_vqs = 1;
+	struct virtqueue *vqs[3];
+	const char *names[3];
+	vq_callback_t *callbacks[3];
+
+	if (vi->has_leakqs)
+		total_vqs = 3;
+
+	callbacks[0] = random_recv_done;
+	names[0] = "input";
+	if (vi->has_leakqs) {
+		callbacks[1] = entropy_leak_detected;
+		names[1] = "leakq.1";
+		callbacks[2] = entropy_leak_detected;
+		names[2] = "leakq.2";
+	}
+
+	ret = virtio_find_vqs(vdev, total_vqs, vqs, callbacks, names, NULL);
+	if (ret)
+		goto err;
+
+	vi->vq = vqs[0];
+
+	if (vi->has_leakqs) {
+		vi->leakq[0] = vqs[1];
+		vi->leakq[1] = vqs[2];
+	}
+
+err:
+	return ret;
+}
+
 static int probe_common(struct virtio_device *vdev)
 {
 	int err, index;

@@ -152,18 +326,24 @@ static int probe_common(struct virtio_device *vdev)
 	};
 	vdev->priv = vi;
 
-	/* We expect a single virtqueue. */
-	vi->vq = virtio_find_single_vq(vdev, random_recv_done, "input");
-	if (IS_ERR(vi->vq)) {
-		err = PTR_ERR(vi->vq);
-		goto err_find;
+	vi->has_leakqs = virtio_has_feature(vdev, VIRTIO_RNG_F_LEAK);
+	if (vi->has_leakqs) {
+		spin_lock_init(&vi->lock);
+		vi->active_leakq = 0;
 	}
 
+	err = init_virtqueues(vi, vdev);
+	if (err)
+		goto err_find;
+
 	virtio_device_ready(vdev);
 
 	/* we always have a pending entropy request */
 	request_entropy(vi);
 
+	/* we always have a fill_on_leak request pending */
+	virtrng_fill_on_leak(vi, vi->leak_data, sizeof(vi->leak_data));
+
 	return 0;
 
 err_find:

@@ -246,7 +426,13 @@ static const struct virtio_device_id id_table[] = {
 	{ 0 },
 };
 
+static unsigned int features[] = {
+	VIRTIO_RNG_F_LEAK,
+};
+
 static struct virtio_driver virtio_rng_driver = {
+	.feature_table = features,
+	.feature_table_size = ARRAY_SIZE(features),
 	.driver.name =	KBUILD_MODNAME,
 	.driver.owner =	THIS_MODULE,
 	.id_table =	id_table,
include/uapi/linux/virtio_rng.h +3 -0
index c4d5de896f0c..d9774951547e 100644
@@ -5,4 +5,7 @@
 #include <linux/virtio_ids.h>
 #include <linux/virtio_config.h>
 
+/* The feature bitmap for virtio entropy device */
+#define VIRTIO_RNG_F_LEAK 0
+
 #endif /* _LINUX_VIRTIO_RNG_H */
Signature · 3 lines
2.38.1

Amazon Spain Services sociedad limitada unipersonal, Calle Ramirez de Prado 5, 28045 Madrid. Registro Mercantil de Madrid . Tomo 22458 . Folio 102 . Hoja M-401234 . CIF B84570936
author
#2 lore raw

PATCH v2 2/2 virtio-rng: add sysfs entries for leak detection

In reply to Babis Chalios

Make use of the copy-on-leak functionality of the virtio rng driver to expose a mechanism to user space for detecting entropy leak events, such as taking a VM snapshot or restoring from one.

The driver setups a single page of memory where it stores in the first word a counter and queues a copy-on-leak command for increasing the counter every time an entropy leak occurs. It exposes the value of the counter in a binary sysfs file per device. The file can be mmap'ed and read and every time a change on the counter is observed, sysfs_notify is used to notify processes that are polling it.

The mechanism is implemented based on the idea of a VM generation counter that had been before proposed as an extension to the VM Generation ID device, where mmap and poll interfaces can be used on the file containing the counter and changes in its value signal snapshot events.

It is worth noting that using mmap is entirely race-free, since changes in the counter are observable by user-space as soon as vcpus are resumed. Instead, using poll is not race-free. There is a race-window between the moment the vcpus are resumed and the used-buffers are handled by the virtio-rng driver.

Signed-off-by Babis Chalios <[email protected]>
---
 drivers/char/hw_random/virtio-rng.c | 178 +++++++++++++++++++++++++++-
 1 file changed, 175 insertions(+), 3 deletions(-)
drivers/char/hw_random/virtio-rng.c +175 -3
index 154f68a1e326..9fe9da09f202 100644
@@ -4,6 +4,9 @@
  *  Copyright (C) 2007, 2008 Rusty Russell IBM Corporation
  */
 
+#include "linux/gfp.h"
+#include "linux/minmax.h"
+#include "linux/sysfs.h"
 #include <linux/hw_random.h>
 #include <linux/scatterlist.h>
 #include <linux/spinlock.h>

@@ -15,6 +18,10 @@
 
 static DEFINE_IDA(rng_index_ida);
 
+#ifdef CONFIG_SYSFS
+static struct kobject *virtio_rng_kobj;
+#endif
+
 struct virtrng_info {
 	struct hwrng hwrng;
 	struct virtqueue *vq;

@@ -23,6 +30,12 @@ struct virtrng_info {
 	struct virtqueue *leakq[2];
 	spinlock_t lock;
 	int active_leakq;
+#ifdef CONFIG_SYSFS
+	struct kobject *kobj;
+	struct bin_attribute vm_gen_counter_attr;
+	unsigned long map_buffer;
+	unsigned long next_vm_gen_counter;
+#endif
 
 	char name[25];
 	int index;

@@ -42,6 +55,40 @@ struct virtrng_info {
 #endif
 };
 
+#ifdef CONFIG_SYSFS
+static ssize_t virtrng_sysfs_read(struct file *filep, struct kobject *kobj,
+		struct bin_attribute *attr, char *buf, loff_t pos, size_t len)
+{
+	struct virtrng_info *vi = attr->private;
+	unsigned long gen_counter = *(unsigned long *)vi->map_buffer;
+
+	if (!len)
+		return 0;
+
+	len = min(len, sizeof(gen_counter));
+	memcpy(buf, &gen_counter, len);
+
+	return len;
+}
+
+static int virtrng_sysfs_mmap(struct file *filep, struct kobject *kobj,
+		struct bin_attribute *attr, struct vm_area_struct *vma)
+{
+	struct virtrng_info *vi = attr->private;
+
+	if (vma->vm_pgoff || vma_pages(vma) > 1)
+		return -EINVAL;
+
+	if (vma->vm_flags & VM_WRITE)
+		return -EPERM;
+
+	vma->vm_flags |= VM_DONTEXPAND;
+	vma->vm_flags &= ~VM_MAYWRITE;
+
+	return vm_insert_page(vma, vma->vm_start, virt_to_page(vi->map_buffer));
+}
+#endif
+
 /* Swaps the queues and returns the new active leak queue. */
 static struct virtqueue *swap_leakqs(struct virtrng_info *vi)
 {

@@ -81,7 +128,7 @@ static int virtrng_fill_on_leak(struct virtrng_info *vi, void *data, size_t len)
 
 	vq = get_active_leakq(vi);
 	ret = add_fill_on_leak_request(vi, vq, data, len);
-	if (ret)
+	if (!ret)
 		virtqueue_kick(vq);
 
 	spin_unlock_irqrestore(&vi->lock, flags);

@@ -121,7 +168,7 @@ static int virtrng_copy_on_leak(struct virtrng_info *vi, void *to, void *from, s
 
 	vq = get_active_leakq(vi);
 	ret = add_copy_on_leak_request(vi, vq, to, from, len);
-	if (ret)
+	if (!ret)
 		virtqueue_kick(vq);
 
 	spin_unlock_irqrestore(&vi->lock, flags);

@@ -137,6 +184,9 @@ static void entropy_leak_detected(struct virtqueue *vq)
 	unsigned long flags;
 	void *buffer;
 	bool kick_activeq = false;
+#ifdef CONFIG_SYSFS
+	bool notify_sysfs = false;
+#endif
 
 	spin_lock_irqsave(&vi->lock, flags);
 

@@ -158,12 +208,34 @@ static void entropy_leak_detected(struct virtqueue *vq)
 			add_fill_on_leak_request(vi, activeq, vi->leak_data, sizeof(vi->leak_data));
 			kick_activeq = true;
 		}
+
+#ifdef CONFIG_SYSFS
+		if (buffer == (void *)vi->map_buffer) {
+			notify_sysfs = true;
+
+			/* Add a request to bump the generation counter on the next leak event.
+			 * We have already swapped leak queues, so this will get properly handled
+			 * with the next entropy leak event.
+			 */
+			vi->next_vm_gen_counter++;
+			add_copy_on_leak_request(vi, activeq, (void *)vi->map_buffer,
+					&vi->next_vm_gen_counter, sizeof(unsigned long));
+
+			kick_activeq = true;
+		}
+#endif
 	}
 
 	if (kick_activeq)
 		virtqueue_kick(activeq);
 
 	spin_unlock_irqrestore(&vi->lock, flags);
+
+#ifdef CONFIG_SYSFS
+	/* Notify anyone polling on the sysfs file */
+	if (notify_sysfs)
+		sysfs_notify(vi->kobj, NULL, "vm_gen_counter");
+#endif
 }
 
 static void random_recv_done(struct virtqueue *vq)

@@ -300,6 +372,59 @@ static int init_virtqueues(struct virtrng_info *vi, struct virtio_device *vdev)
 	return ret;
 }
 
+#ifdef CONFIG_SYSFS
+static int setup_sysfs(struct virtrng_info *vi)
+{
+	int err;
+
+	vi->next_vm_gen_counter = 1;
+
+	/* We have one binary file per device under /sys/virtio-rng/<device>/vm_gen_counter */
+	vi->vm_gen_counter_attr.attr.name = "vm_gen_counter";
+	vi->vm_gen_counter_attr.attr.mode = 0444;
+	vi->vm_gen_counter_attr.read = virtrng_sysfs_read;
+	vi->vm_gen_counter_attr.mmap = virtrng_sysfs_mmap;
+	vi->vm_gen_counter_attr.private = vi;
+
+	vi->map_buffer = get_zeroed_page(GFP_KERNEL);
+	if (!vi->map_buffer)
+		return -ENOMEM;
+
+	err = -ENOMEM;
+	vi->kobj = kobject_create_and_add(vi->name, virtio_rng_kobj);
+	if (!vi->kobj)
+		goto err_page;
+
+	err = sysfs_create_bin_file(vi->kobj, &vi->vm_gen_counter_attr);
+	if (err)
+		goto err_kobj;
+
+	return 0;
+
+err_kobj:
+	kobject_put(vi->kobj);
+err_page:
+	free_pages(vi->map_buffer, 0);
+	return err;
+}
+
+static void cleanup_sysfs(struct virtrng_info *vi)
+{
+	sysfs_remove_bin_file(vi->kobj, &vi->vm_gen_counter_attr);
+	kobject_put(vi->kobj);
+	free_pages(vi->map_buffer, 0);
+}
+#else
+static int setup_sysfs(struct virtrng_info *vi)
+{
+	return 0;
+}
+
+static void cleanup_sysfs(struct virtrng_info *vi)
+{
+}
+#endif
+
 static int probe_common(struct virtio_device *vdev)
 {
 	int err, index;

@@ -330,11 +455,15 @@ static int probe_common(struct virtio_device *vdev)
 	if (vi->has_leakqs) {
 		spin_lock_init(&vi->lock);
 		vi->active_leakq = 0;
+
+		err = setup_sysfs(vi);
+		if (err)
+			goto err_find;
 	}
 
 	err = init_virtqueues(vi, vdev);
 	if (err)
-		goto err_find;
+		goto err_sysfs;
 
 	virtio_device_ready(vdev);
 

@@ -344,8 +473,18 @@ static int probe_common(struct virtio_device *vdev)
 	/* we always have a fill_on_leak request pending */
 	virtrng_fill_on_leak(vi, vi->leak_data, sizeof(vi->leak_data));
 
+#ifdef CONFIG_SYSFS
+	/* also a copy_on_leak request for the generation counter when we have sysfs
+	 * support.
+	 */
+	virtrng_copy_on_leak(vi, (void *)vi->map_buffer, &vi->next_vm_gen_counter,
+			sizeof(unsigned long));
+#endif
+
 	return 0;
 
+err_sysfs:
+	cleanup_sysfs(vi);
 err_find:
 	ida_simple_remove(&rng_index_ida, index);
 err_ida:

@@ -363,6 +502,8 @@ static void remove_common(struct virtio_device *vdev)
 	complete(&vi->have_data);
 	if (vi->hwrng_register_done)
 		hwrng_unregister(&vi->hwrng);
+	if (vi->has_leakqs)
+		cleanup_sysfs(vi);
 	virtio_reset_device(vdev);
 	vdev->config->del_vqs(vdev);
 	ida_simple_remove(&rng_index_ida, vi->index);

@@ -445,7 +586,38 @@ static struct virtio_driver virtio_rng_driver = {
 #endif
 };
 
+#ifdef CONFIG_SYSFS
+static int __init virtio_rng_init(void)
+{
+	int ret;
+
+	virtio_rng_kobj = kobject_create_and_add("virtio-rng", NULL);
+	if (!virtio_rng_kobj)
+		return -ENOMEM;
+
+	ret = register_virtio_driver(&virtio_rng_driver);
+	if (ret < 0)
+		goto err;
+
+	return 0;
+
+err:
+	kobject_put(virtio_rng_kobj);
+	return ret;
+}
+
+static void __exit virtio_rng_fini(void)
+{
+	kobject_put(virtio_rng_kobj);
+	unregister_virtio_driver(&virtio_rng_driver);
+}
+
+module_init(virtio_rng_init);
+module_exit(virtio_rng_fini);
+#else
 module_virtio_driver(virtio_rng_driver);
+#endif
+
 MODULE_DEVICE_TABLE(virtio, id_table);
 MODULE_DESCRIPTION("Virtio random number driver");
 MODULE_LICENSE("GPL");
Signature · 3 lines
2.38.1

Amazon Spain Services sociedad limitada unipersonal, Calle Ramirez de Prado 5, 28045 Madrid. Registro Mercantil de Madrid . Tomo 22458 . Folio 102 . Hoja M-401234 . CIF B84570936
Nacked-by on the v2 series NAK
#3 lore raw
In reply to Babis Chalios

You sent a v2, but I'm not back until the 11th to provide comments on v1. I still think this isn't the right direction, as this needs tie-ins to the rng to actually be useful. Please stop posting new versions of this for now, so that somebody doesn't accidentally merge it; that'd be a big mistake. I'll paste what I wrote you prior:

| Hi Babis,
| 
| As I mentioned to you privately this week, I'm about to be out of town,
| so I won't be able to look at this until I'm back in a few weeks. I
| appreciate your patience.
| 
| But as a cursory look, I'm happy that you've written the hardware-side
| code for this. That's a great starting point. The plumbing is not so
| nice, though. This needs to be integrated more closely with random.c
| itself, similar to how vmgenid works.
| 
| When I'm back in a few weeks, I'll see if I can either write a
| description of what I have in mind, or simply integrate the useful
| hardware work here into an expanded patch series.
| 
| [Please don't merge anything for now.]

So: you wrote some maybe useful hardware code. The rest is wrong. And we haven't even concluded discussions on whether the virtio interface is the right one. In fact, I had previously asked if we could schedule this all until March. Marco from your team then sent an impatient email, so I said, alright, what about Feb 11 when I'm back. That's annoying for me but I figured I'd just shuffle everything around and prioritize this. Then, instead of waiting for that, you posted v1 of this patchset the next day. I asked you again. And now, while I'm away on the first holiday in a while with very little connectivity and no laptop, you post a v2. So I'm really annoyed. In order to avoid all doubt about this, let me then just NACK this, and I'll lift the nack when I'm back:

Nacked-by Jason A. Donenfeld <[email protected]>
author
#4 lore raw

Hi Jason,

I really didn't mean to interrupt your holidays.

I posted this as an RFC exactly because I don't want this to be merged _as is_, it's just a trigger for discussion. It is v2 just as a response to Michael's comments in the previous version and fixes for the CI.

Also, there are other people that expressed interest in this and they wanted to participate in the coversation, so that's just that.

I hope you enjoy your vacation, get some rest and speak to you once you' re back :)

Cheers,
Babis

Quoted 39 lines · Jason A. Donenfeld

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.

You sent a v2, but I'm not back until the 11th to provide comments on v1. I still think this isn't the right direction, as this needs tie-ins to the rng to actually be useful. Please stop posting new versions of this for now, so that somebody doesn't accidentally merge it; that'd be a big mistake. I'll paste what I wrote you prior:

| Hi Babis,
|
| As I mentioned to you privately this week, I'm about to be out of town,
| so I won't be able to look at this until I'm back in a few weeks. I
| appreciate your patience.
|
| But as a cursory look, I'm happy that you've written the hardware-side
| code for this. That's a great starting point. The plumbing is not so
| nice, though. This needs to be integrated more closely with random.c
| itself, similar to how vmgenid works.
|
| When I'm back in a few weeks, I'll see if I can either write a
| description of what I have in mind, or simply integrate the useful
| hardware work here into an expanded patch series.
|
| [Please don't merge anything for now.]

So: you wrote some maybe useful hardware code. The rest is wrong. And we haven't even concluded discussions on whether the virtio interface is the right one. In fact, I had previously asked if we could schedule this all until March. Marco from your team then sent an impatient email, so I said, alright, what about Feb 11 when I'm back. That's annoying for me but I figured I'd just shuffle everything around and prioritize this. Then, instead of waiting for that, you posted v1 of this patchset the next day. I asked you again. And now, while I'm away on the first holiday in a while with very little connectivity and no laptop, you post a v2. So I'm really annoyed. In order to avoid all doubt about this, let me then just NACK this, and I'll lift the nack when I'm back:

Quoted 2 lines
Nacked-by Jason A. Donenfeld <[email protected]>

Amazon Spain Services sociedad limitada unipersonal, Calle Ramirez de Prado 5, 28045 Madrid. Registro Mercantil de Madrid . Tomo 22458 . Folio 102 . Hoja M-401234 . CIF B84570936

#5 lore raw
In reply to Jason A. Donenfeld

Hey all,

Quoted 5 lines · Jason A. Donenfeld

You sent a v2, but I'm not back until the 11th to provide comments on v1. I still think this isn't the right direction, as this needs tie-ins to the rng to actually be useful. Please stop posting new versions of this for now, so that somebody doesn't accidentally merge it; that'd be a big mistake. I'll paste what I wrote you prior:

I sat down to review the patchset but looks like there's some background I'm not aware of.

It looks like Babis has implemented the guest side here according to the spec change that Michael has posted.

Jason, do you have an alternative in mind? In that case, we should pick this up in the spec update thread instead.

Somehow it feels like I don't have the complete story for this feature set, having missed the discussion during LPC.

Amit

author
#6 lore raw

Hi Amit,

Thanks for taking the time to look into this.

Quoted 13 lines · Amit Shah

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.

Hey all,

Quoted 5 lines · Jason A. Donenfeld

You sent a v2, but I'm not back until the 11th to provide comments on v1. I still think this isn't the right direction, as this needs tie-ins to the rng to actually be useful. Please stop posting new versions of this for now, so that somebody doesn't accidentally merge it; that'd be a big mistake. I'll paste what I wrote you prior:

Quoted 8 lines

I sat down to review the patchset but looks like there's some background I'm not aware of.

It looks like Babis has implemented the guest side here according to the spec change that Michael has posted.

Jason, do you have an alternative in mind? In that case, we should pick this up in the spec update thread instead.

I am not sure what Jason meant here by "This needs to be integrated more closely with random.c itself, similar to how vmgenid works", but here's my take on this.

The point of the patchset is to provide an implementation of Michael's spec on which we can discuss. It implements the HW API and it has some hooks showing how this API could be used. It is mainly directed towards the user-space where we did not have a proper API to consume VMGENID-like functionality. With regards to the random.c components it does exactly what VMGENID does currently, i.e. whenever an entropy-leak is detected it uses the new random bytes provided by the virtio-rng device as entropy. This is as racy as VMGENID, as I mention in the cover letter of the patchset.

However, the new spec does allow us to do things _correctly_, i.e. not rely on asynchronous handling of events to re-seed the kernel. For example, we could achieve something like that by making use of the "copy-on-leak" operation, so that a flag changes value before vCPUs get resumed, so we know when a leak has happened when needed, e.g. before returning random bytes to user-space. At least, that's what I remember us discussing during LPC. Jason, Michael, Alex, please keep me honest here :)

Unfortunately, I am not very familiar with the random.c code and did not want to do something there that would most certainly be wrong, hence I posted this as an RFC, asking for input on how we could achieve this better integration. Hopefully, when Jason is back from his vacation he can share his thoughts on this, but if yourself (or anyone else interested) have any ideas on how we could design this properly, I 'm happy to discuss!

Quoted 6 lines

Somehow it feels like I don't have the complete story for this feature set, having missed the discussion during LPC.

Amit

Cheers,
Babis
Amazon Spain Services sociedad limitada unipersonal, Calle Ramirez de Prado 5, 28045 Madrid. Registro Mercantil de Madrid . Tomo 22458 . Folio 102 . Hoja M-401234 . CIF B84570936

#7 lore raw

Hey Herbert / Jason / crypto maintainers,

Quoted 27 lines · On Mon, 2023-03-13 at 11:42 +0100, [email protected] wrote:

Hi Amit,

Thanks for taking the time to look into this.

Quoted 13 lines · Amit Shah

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.

Hey all,

Quoted 5 lines · Jason A. Donenfeld

You sent a v2, but I'm not back until the 11th to provide comments on v1. I still think this isn't the right direction, as this needs tie-ins to the rng to actually be useful. Please stop posting new versions of this for now, so that somebody doesn't accidentally merge it; that'd be a big mistake. I'll paste what I wrote you prior:

Quoted 8 lines

I sat down to review the patchset but looks like there's some background I'm not aware of.

It looks like Babis has implemented the guest side here according to the spec change that Michael has posted.

Jason, do you have an alternative in mind? In that case, we should pick this up in the spec update thread instead.

Quoted 8 lines

I am not sure what Jason meant here by "This needs to be integrated more closely with random.c itself, similar to how vmgenid works", but here's my take on this.

The point of the patchset is to provide an implementation of Michael's spec on which we can discuss. It implements the HW API and it has some hooks showing how this API could be used. It is mainly directed towards the user-space where we did not have a proper API to consume VMGENID-like functionality. With regards to the random.c components it does exactly what VMGENID does currently, i.e. whenever an entropy-leak is detected it uses the new random bytes provided by the virtio-rng device as entropy. This is as racy as VMGENID, as I mention in the cover letter of the patchset.

Yea, this does solve the race condition from the userspace pov, so does look better. Thanks for the details!

Not sure if Jason's back yet - but Herbert, or other crypto maintainers, can you chime in from the crypto/rng perspective if this looks sane?

Jason has previously NACKed the patch without follow-up, and I don't want the patch to linger without a path to merging, especially when it's not clear what Jason meant.

Quoted 8 lines

However, the new spec does allow us to do things _correctly_, i.e. not rely on asynchronous handling of events to re-seed the kernel. For example, we could achieve something like that by making use of the "copy-on-leak" operation, so that a flag changes value before vCPUs get resumed, so we know when a leak has happened when needed, e.g. before returning random bytes to user-space. At least, that's what I remember us discussing during LPC. Jason, Michael, Alex, please keep me honest here :)

Unfortunately, I am not very familiar with the random.c code and did not want to do something there that would most certainly be wrong, hence I posted this as an RFC, asking for input on how we could achieve this better integration. Hopefully, when Jason is back from his vacation he can share his thoughts on this, but if yourself (or anyone else interested) have any ideas on how we could design this properly, I 'm happy to discuss!

Let's wait a couple more days for responses, otherwise I suggest you resubmit to kickstart a new discussion, with the note that Jason had something else in mind - so that it doesn't appear as though we're trying to override that.

Thanks for the patience,

Amit

#8 lore raw
Quoted 1 line · Amit Shah

Hey Herbert / Jason / crypto maintainers,

[...]

Quoted 4 lines

Let's wait a couple more days for responses, otherwise I suggest you resubmit to kickstart a new discussion, with the note that Jason had something else in mind - so that it doesn't appear as though we're trying to override that.

I reached out to Jason on IRC, and he mentioned he will follow up with a patch that incorporates ideas from your patch plus hooking into random.c. Sounds promising!

Amit
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization