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(-)
--
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
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(-)
diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index a6f3a8a2aca6..154f68a1e326 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -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] = ∈
+
+ 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,
diff --git a/include/uapi/linux/virtio_rng.h b/include/uapi/linux/virtio_rng.h
index c4d5de896f0c..d9774951547e 100644
--- a/include/uapi/linux/virtio_rng.h
+++ b/include/uapi/linux/virtio_rng.h
@@ -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 */
--
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
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(-)
diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 154f68a1e326..9fe9da09f202 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -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");
--
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
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]>
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
On 1/31/23 5:27 PM, "Jason A. Donenfeld" <[email protected]> wrote:
> 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:
>
> 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
Hey all,
On Tue, 2023-01-31 at 17:27 +0100, Jason A. Donenfeld wrote:
> 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
Hi Amit,
Thanks for taking the time to look into this.
On 3/2/23 5:55 PM, Amit Shah <[email protected]> wrote:
> 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,
>
> On Tue, 2023-01-31 at 17:27 +0100, Jason A. Donenfeld wrote:
> > 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.
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!
>
> 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
Hey Herbert / Jason / crypto maintainers,
On Mon, 2023-03-13 at 11:42 +0100, [email protected] wrote:
> Hi Amit,
>
> Thanks for taking the time to look into this.
>
> On 3/2/23 5:55 PM, Amit Shah <[email protected]> wrote:
> > 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,
> >
> > On Tue, 2023-01-31 at 17:27 +0100, Jason A. Donenfeld wrote:
> > > 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.
>
> 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.
> 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
On Mon, 2023-03-13 at 19:05 +0100, Amit Shah wrote:
> Hey Herbert / Jason / crypto maintainers,
[...]
> 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