[PATCH] virtio_rng: dont use vmalloced addresses for virtio

[PATCH] virtio_rng: dont use vmalloced addresses for virtio

[Christian Borntraeger]

If virtio_rng is build as a module, random_data is an address in vmalloc
space. As virtio expects guest real addresses, this can cause any kind of 
funny behaviour, so lets allocate random_data dynamically with kmalloc.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

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

Index: kvm/drivers/char/hw_random/virtio-rng.c
===================================================================
--- kvm.orig/drivers/char/hw_random/virtio-rng.c
+++ kvm/drivers/char/hw_random/virtio-rng.c
@@ -27,7 +27,7 @@
  * give it 64 bytes at a time, and the hwrng framework takes it 4 bytes at a
  * time. */
 static struct virtqueue *vq;
-static u32 random_data[16];
+static u32 *random_data;
 static unsigned int data_left;
 static DECLARE_COMPLETION(have_data);
 
@@ -47,9 +47,9 @@ static void register_buffer(void)
 {
 	struct scatterlist sg;
 
-	sg_init_one(&sg, &random_data, sizeof(random_data));
+	sg_init_one(&sg, random_data, 64);
 	/* There should always be room for one buffer. */
-	if (vq->vq_ops->add_buf(vq, &sg, 0, 1, &random_data) != 0)
+	if (vq->vq_ops->add_buf(vq, &sg, 0, 1, random_data) != 0)
 		BUG();
 	vq->vq_ops->kick(vq);
 }
@@ -128,11 +128,15 @@ static struct virtio_driver virtio_rng =
 
 static int __init init(void)
 {
+	random_data = kmalloc(64, GFP_KERNEL);
+	if (!random_data)
+		return -ENOMEM;
 	return register_virtio_driver(&virtio_rng);
 }
 
 static void __exit fini(void)
 {
+	kfree(random_data);
 	unregister_virtio_driver(&virtio_rng);
 }
 module_init(init);

Re: [PATCH] virtio_rng: dont use vmalloced addresses for virtio

[Rusty Russell]

On Tuesday 27 May 2008 00:25:28 Christian Borntraeger wrote:
> If virtio_rng is build as a module, random_data is an address in vmalloc
> space. As virtio expects guest real addresses, this can cause any kind of
> funny behaviour, so lets allocate random_data dynamically with kmalloc.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Erk, nasty trap...

> -	sg_init_one(&sg, &random_data, sizeof(random_data));
> +	sg_init_one(&sg, random_data, 64);

Constant in two places, I've moved to a #define.

>  static int __init init(void)
>  {
> +	random_data = kmalloc(64, GFP_KERNEL);
> +	if (!random_data)
> +		return -ENOMEM;
>  	return register_virtio_driver(&virtio_rng);

And added cleanup on register_virtio_driver fail (a tiny leak. but still).

Cheers,
Rusty.

Subject: virtio_rng: dont use vmalloced addresses for virtio
Date: Mon, 26 May 2008 16:25:28 +0200
From: Christian Borntraeger <borntraeger@de.ibm.com>

If virtio_rng is build as a module, random_data is an address in vmalloc
space. As virtio expects guest real addresses, this can cause any kind of
funny behaviour, so lets allocate random_data dynamically with kmalloc.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/char/hw_random/virtio-rng.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff -r 82ce499b3321 drivers/char/hw_random/virtio-rng.c
--- a/drivers/char/hw_random/virtio-rng.c	Tue May 27 11:22:18 2008 +1000
+++ b/drivers/char/hw_random/virtio-rng.c	Tue May 27 11:25:32 2008 +1000
@@ -26,8 +26,10 @@
 /* The host will fill any buffer we give it with sweet, sweet randomness.  We
  * give it 64 bytes at a time, and the hwrng framework takes it 4 bytes at a
  * time. */
+#define RANDOM_DATA_SIZE 64
+
 static struct virtqueue *vq;
-static u32 random_data[16];
+static u32 *random_data;
 static unsigned int data_left;
 static DECLARE_COMPLETION(have_data);
 
@@ -47,9 +49,9 @@ static void register_buffer(void)
 {
 	struct scatterlist sg;
 
-	sg_init_one(&sg, &random_data, sizeof(random_data));
+	sg_init_one(&sg, random_data, RANDOM_DATA_SIZE);
 	/* There should always be room for one buffer. */
-	if (vq->vq_ops->add_buf(vq, &sg, 0, 1, &random_data) != 0)
+	if (vq->vq_ops->add_buf(vq, &sg, 0, 1, random_data) != 0)
 		BUG();
 	vq->vq_ops->kick(vq);
 }
@@ -128,11 +130,21 @@ static struct virtio_driver virtio_rng =
 
 static int __init init(void)
 {
-	return register_virtio_driver(&virtio_rng);
+	int err;
+
+	random_data = kmalloc(RANDOM_DATA_SIZE, GFP_KERNEL);
+	if (!random_data)
+		return -ENOMEM;
+
+	err = register_virtio_driver(&virtio_rng);
+	if (err)
+		kfree(random_data);
+	return err;
 }
 
 static void __exit fini(void)
 {
+	kfree(random_data);
 	unregister_virtio_driver(&virtio_rng);
 }
 module_init(init);

Re: [PATCH] virtio_rng: dont use vmalloced addresses for virtio

[Christian Borntraeger]

Am Dienstag, 27. Mai 2008 schrieb Rusty Russell:
> Constant in two places, I've moved to a #define.
[...]
> And added cleanup on register_virtio_driver fail (a tiny leak. but still).

looks good. Thanks