Patchwork [2/4] virtio-rng-pci: create a default backend if none exists

login
register
mail settings
Submitter Anthony Liguori
Date Oct. 26, 2012, 5:21 p.m.
Message ID <1351272088-7942-3-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/194540/
State New
Headers show

Comments

Anthony Liguori - Oct. 26, 2012, 5:21 p.m.
This allows you to specify:

  $ qemu -device virtio-rng-pci

And things will Just Work with a reasonable default.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/virtio-pci.c | 13 +++++++++++++
 hw/virtio-rng.h |  2 ++
 2 files changed, 15 insertions(+)
Paolo Bonzini - Oct. 26, 2012, 6:59 p.m.
Il 26/10/2012 19:21, Anthony Liguori ha scritto:
> This allows you to specify:
> 
>   $ qemu -device virtio-rng-pci
> 
> And things will Just Work with a reasonable default.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/virtio-pci.c | 13 +++++++++++++
>  hw/virtio-rng.h |  2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 0dc2a06..cfdb779 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -885,6 +885,19 @@ static int virtio_rng_init_pci(PCIDevice *pci_dev)
>      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>      VirtIODevice *vdev;
>  
> +    if (proxy->rng.rng == NULL) {
> +        proxy->rng.default_backend = RNG_RANDOM(object_new(TYPE_RNG_RANDOM));
> +
> +        object_property_add_child(OBJECT(pci_dev),
> +                                  "default-backend",
> +                                  OBJECT(proxy->rng.default_backend),
> +                                  NULL);
> +
> +        object_property_set_link(OBJECT(pci_dev),
> +                                 OBJECT(proxy->rng.default_backend),
> +                                 "rng", NULL);
> +    }
> +
>      vdev = virtio_rng_init(&pci_dev->qdev, &proxy->rng);
>      if (!vdev) {
>          return -1;
> diff --git a/hw/virtio-rng.h b/hw/virtio-rng.h
> index fbb0104..63ddb96 100644
> --- a/hw/virtio-rng.h
> +++ b/hw/virtio-rng.h
> @@ -13,12 +13,14 @@
>  #define _QEMU_VIRTIO_RNG_H
>  
>  #include "qemu/rng.h"
> +#include "qemu/rng-random.h"
>  
>  /* The Virtio ID for the virtio rng device */
>  #define VIRTIO_ID_RNG    4
>  
>  struct VirtIORNGConf {
>      RngBackend *rng;
> +    RndRandom *default_backend;
>  };
>  
>  #endif
> 

NACK.  Starting a guest that runs rngd (or just a malicious guest) will
completely deprive the host of entropy.

If you make the default /dev/hwrng, however, that would be ok.

Paolo
Anthony Liguori - Oct. 26, 2012, 7:51 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 26/10/2012 19:21, Anthony Liguori ha scritto:
>> This allows you to specify:
>> 
>>   $ qemu -device virtio-rng-pci
>> 
>> And things will Just Work with a reasonable default.
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  hw/virtio-pci.c | 13 +++++++++++++
>>  hw/virtio-rng.h |  2 ++
>>  2 files changed, 15 insertions(+)
>> 
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index 0dc2a06..cfdb779 100644
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -885,6 +885,19 @@ static int virtio_rng_init_pci(PCIDevice *pci_dev)
>>      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>>      VirtIODevice *vdev;
>>  
>> +    if (proxy->rng.rng == NULL) {
>> +        proxy->rng.default_backend = RNG_RANDOM(object_new(TYPE_RNG_RANDOM));
>> +
>> +        object_property_add_child(OBJECT(pci_dev),
>> +                                  "default-backend",
>> +                                  OBJECT(proxy->rng.default_backend),
>> +                                  NULL);
>> +
>> +        object_property_set_link(OBJECT(pci_dev),
>> +                                 OBJECT(proxy->rng.default_backend),
>> +                                 "rng", NULL);
>> +    }
>> +
>>      vdev = virtio_rng_init(&pci_dev->qdev, &proxy->rng);
>>      if (!vdev) {
>>          return -1;
>> diff --git a/hw/virtio-rng.h b/hw/virtio-rng.h
>> index fbb0104..63ddb96 100644
>> --- a/hw/virtio-rng.h
>> +++ b/hw/virtio-rng.h
>> @@ -13,12 +13,14 @@
>>  #define _QEMU_VIRTIO_RNG_H
>>  
>>  #include "qemu/rng.h"
>> +#include "qemu/rng-random.h"
>>  
>>  /* The Virtio ID for the virtio rng device */
>>  #define VIRTIO_ID_RNG    4
>>  
>>  struct VirtIORNGConf {
>>      RngBackend *rng;
>> +    RndRandom *default_backend;
>>  };
>>  
>>  #endif
>> 
>
> NACK.  Starting a guest that runs rngd (or just a malicious guest) will
> completely deprive the host of entropy.

That's why this is a separate series...  Still don't understand what the
default entropy source should be.

> If you make the default /dev/hwrng, however, that would be ok.

/dev/hwrng may be (and stay) empty which seems unfortunate.

I was thinking /dev/urandom would be a good pragmatic choice though.

Regards,

Anthony Liguori
>
> Paolo
Paolo Bonzini - Oct. 26, 2012, 7:53 p.m.
Il 26/10/2012 20:59, Paolo Bonzini ha scritto:
> Il 26/10/2012 19:21, Anthony Liguori ha scritto:
>> This allows you to specify:
>>
>>   $ qemu -device virtio-rng-pci
>>
>> And things will Just Work with a reasonable default.
>>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  hw/virtio-pci.c | 13 +++++++++++++
>>  hw/virtio-rng.h |  2 ++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index 0dc2a06..cfdb779 100644
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -885,6 +885,19 @@ static int virtio_rng_init_pci(PCIDevice *pci_dev)
>>      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>>      VirtIODevice *vdev;
>>  
>> +    if (proxy->rng.rng == NULL) {
>> +        proxy->rng.default_backend = RNG_RANDOM(object_new(TYPE_RNG_RANDOM));
>> +
>> +        object_property_add_child(OBJECT(pci_dev),
>> +                                  "default-backend",
>> +                                  OBJECT(proxy->rng.default_backend),
>> +                                  NULL);
>> +
>> +        object_property_set_link(OBJECT(pci_dev),
>> +                                 OBJECT(proxy->rng.default_backend),
>> +                                 "rng", NULL);
>> +    }
>> +
>>      vdev = virtio_rng_init(&pci_dev->qdev, &proxy->rng);
>>      if (!vdev) {
>>          return -1;
>> diff --git a/hw/virtio-rng.h b/hw/virtio-rng.h
>> index fbb0104..63ddb96 100644
>> --- a/hw/virtio-rng.h
>> +++ b/hw/virtio-rng.h
>> @@ -13,12 +13,14 @@
>>  #define _QEMU_VIRTIO_RNG_H
>>  
>>  #include "qemu/rng.h"
>> +#include "qemu/rng-random.h"
>>  
>>  /* The Virtio ID for the virtio rng device */
>>  #define VIRTIO_ID_RNG    4
>>  
>>  struct VirtIORNGConf {
>>      RngBackend *rng;
>> +    RndRandom *default_backend;
>>  };
>>  
>>  #endif
>>
> 
> NACK.  Starting a guest that runs rngd (or just a malicious guest) will
> completely deprive the host of entropy.
> 
> If you make the default /dev/hwrng, however, that would be ok.

Also, does this break non-Linux?  What if the default was changed to
/dev/hwrng but an older Linux distro didn't have the device file at all?

Paolo
Anthony Liguori - Oct. 26, 2012, 8:16 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 26/10/2012 20:59, Paolo Bonzini ha scritto:
>> Il 26/10/2012 19:21, Anthony Liguori ha scritto:
>>> This allows you to specify:
>>>
>>>   $ qemu -device virtio-rng-pci
>>>
>>> And things will Just Work with a reasonable default.
>>>
>>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>> ---
>>>  hw/virtio-pci.c | 13 +++++++++++++
>>>  hw/virtio-rng.h |  2 ++
>>>  2 files changed, 15 insertions(+)
>>>
>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>> index 0dc2a06..cfdb779 100644
>>> --- a/hw/virtio-pci.c
>>> +++ b/hw/virtio-pci.c
>>> @@ -885,6 +885,19 @@ static int virtio_rng_init_pci(PCIDevice *pci_dev)
>>>      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>>>      VirtIODevice *vdev;
>>>  
>>> +    if (proxy->rng.rng == NULL) {
>>> +        proxy->rng.default_backend = RNG_RANDOM(object_new(TYPE_RNG_RANDOM));
>>> +
>>> +        object_property_add_child(OBJECT(pci_dev),
>>> +                                  "default-backend",
>>> +                                  OBJECT(proxy->rng.default_backend),
>>> +                                  NULL);
>>> +
>>> +        object_property_set_link(OBJECT(pci_dev),
>>> +                                 OBJECT(proxy->rng.default_backend),
>>> +                                 "rng", NULL);
>>> +    }
>>> +
>>>      vdev = virtio_rng_init(&pci_dev->qdev, &proxy->rng);
>>>      if (!vdev) {
>>>          return -1;
>>> diff --git a/hw/virtio-rng.h b/hw/virtio-rng.h
>>> index fbb0104..63ddb96 100644
>>> --- a/hw/virtio-rng.h
>>> +++ b/hw/virtio-rng.h
>>> @@ -13,12 +13,14 @@
>>>  #define _QEMU_VIRTIO_RNG_H
>>>  
>>>  #include "qemu/rng.h"
>>> +#include "qemu/rng-random.h"
>>>  
>>>  /* The Virtio ID for the virtio rng device */
>>>  #define VIRTIO_ID_RNG    4
>>>  
>>>  struct VirtIORNGConf {
>>>      RngBackend *rng;
>>> +    RndRandom *default_backend;
>>>  };
>>>  
>>>  #endif
>>>
>> 
>> NACK.  Starting a guest that runs rngd (or just a malicious guest) will
>> completely deprive the host of entropy.
>> 
>> If you make the default /dev/hwrng, however, that would be ok.
>
> Also, does this break non-Linux?

It should fail gracefully.

If you do:

qemu -device virtio-rng-pci

Before this series you'd get:

qemu: Invalid value for parameter 'rng', expects a valid object

Now on !Linux you would get:

qemu -device virtio-rng-pci

qemu: Failed to open /dev/random

So it's still a failure, just a different message.

But this does suggest that we shouldn't add it to the default machine on
!Linux because we don't want the default machine failing.

Perhaps we can find better default backends on !Linux...

> What if the default was changed to /dev/hwrng but an older Linux
> distro didn't have the device file at all?

It would throw an error gracefully.

Regards,

Anthony Liguori

>
> Paolo
Paolo Bonzini - Oct. 26, 2012, 8:20 p.m.
Il 26/10/2012 21:51, Anthony Liguori ha scritto:
>> > If you make the default /dev/hwrng, however, that would be ok.
> /dev/hwrng may be (and stay) empty which seems unfortunate.

Unfortunate, but at least not wrong.

> I was thinking /dev/urandom would be a good pragmatic choice though.

No.

/dev/urandom is actively wrong because it provides the guest with the
illusion of an infinite source of entropy, while the guest is really
being fed with an infinite source of pseudo-random numbers.

/dev/random as a default is bad because on hosts without neither hwrng
nor rdrand it completely depletes the host's entropy pool.  Thus it
denies access to entropy to other programs running in the host.

I thought /dev/random with some throttling would be good, especially if
somehow the guest can be told to run rngd in skip-test mode, e.g. via a
virtio-rng feature bit.  Peter's last messages make me wonder if this is
correct.  If it is, the throttling can be implemented either in QEMU or
outside it (via a daemon that speaks the same protocol as egd).

/dev/random might be good in the special case where rngd is being run in
the host, and there is an hwrng or rdrand to feed rngd.  In this case
the guest can also be run in skip-test mode.  However, I don't have a
machine at hand (it's Friday evening here) to test whether rngd could
keep up, or a malicious guest would instead also deplete the host's
entropy tool too badly.

/dev/null makes the guest behave exactly as if no virtio-rng-pci is
present, so it is at least not wrong.

rdrand and /dev/hwrng seem to be the best choice at least to me.  Peter
seemed to agree initially, then said "This is surreal.  Output from
/dev/hwrng turns into output for /dev/random... it us guaranteed worse;
period, end of story".  I'm confused.

I hope the above is not too inaccurate and at least a decent way to
reset the discussion.

Paolo
Paolo Bonzini - Oct. 26, 2012, 8:22 p.m.
Il 26/10/2012 22:16, Anthony Liguori ha scritto:
> So it's still a failure, just a different message.
> 
> But this does suggest that we shouldn't add it to the default machine on
> !Linux because we don't want the default machine failing.

We need to add it to all machines, because the machine signature should
not depend on the host OS.  However, it does suggest we need a null
backend as a fallback.

> Perhaps we can find better default backends on !Linux...

Yes, rdrand would be one.

>> > What if the default was changed to /dev/hwrng but an older Linux
>> > distro didn't have the device file at all?
> It would throw an error gracefully.

Perhaps the default backend can be created with a function that tries
multiple things and at worst settles on rng-null.

Paolo

Patch

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 0dc2a06..cfdb779 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -885,6 +885,19 @@  static int virtio_rng_init_pci(PCIDevice *pci_dev)
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
     VirtIODevice *vdev;
 
+    if (proxy->rng.rng == NULL) {
+        proxy->rng.default_backend = RNG_RANDOM(object_new(TYPE_RNG_RANDOM));
+
+        object_property_add_child(OBJECT(pci_dev),
+                                  "default-backend",
+                                  OBJECT(proxy->rng.default_backend),
+                                  NULL);
+
+        object_property_set_link(OBJECT(pci_dev),
+                                 OBJECT(proxy->rng.default_backend),
+                                 "rng", NULL);
+    }
+
     vdev = virtio_rng_init(&pci_dev->qdev, &proxy->rng);
     if (!vdev) {
         return -1;
diff --git a/hw/virtio-rng.h b/hw/virtio-rng.h
index fbb0104..63ddb96 100644
--- a/hw/virtio-rng.h
+++ b/hw/virtio-rng.h
@@ -13,12 +13,14 @@ 
 #define _QEMU_VIRTIO_RNG_H
 
 #include "qemu/rng.h"
+#include "qemu/rng-random.h"
 
 /* The Virtio ID for the virtio rng device */
 #define VIRTIO_ID_RNG    4
 
 struct VirtIORNGConf {
     RngBackend *rng;
+    RndRandom *default_backend;
 };
 
 #endif