Commit ac52578d authored by Herbert Xu's avatar Herbert Xu

hwrng: virtio - Fix race on data_avail and actual data

The virtio rng device kicks off a new entropy request whenever the
data available reaches zero.  When a new request occurs at the end
of a read operation, that is, when the result of that request is
only needed by the next reader, then there is a race between the
writing of the new data and the next reader.

This is because there is no synchronisation whatsoever between the
writer and the reader.

Fix this by writing data_avail with smp_store_release and reading
it with smp_load_acquire when we first enter read.  The subsequent
reads are safe because they're either protected by the first load
acquire, or by the completion mechanism.

Also remove the redundant zeroing of data_idx in random_recv_done
(data_idx must already be zero at this point) and data_avail in
request_entropy (ditto).

Reported-by: syzbot+726dc8c62c3536431ceb@syzkaller.appspotmail.com
Fixes: f7f510ec ("virtio: An entropy device, as suggested by hpa.")
Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
Acked-by: default avatarMichael S. Tsirkin <mst@redhat.com>
Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
parent a4855a8c
...@@ -4,6 +4,7 @@ ...@@ -4,6 +4,7 @@
* Copyright (C) 2007, 2008 Rusty Russell IBM Corporation * Copyright (C) 2007, 2008 Rusty Russell IBM Corporation
*/ */
#include <asm/barrier.h>
#include <linux/err.h> #include <linux/err.h>
#include <linux/hw_random.h> #include <linux/hw_random.h>
#include <linux/scatterlist.h> #include <linux/scatterlist.h>
...@@ -37,13 +38,13 @@ struct virtrng_info { ...@@ -37,13 +38,13 @@ struct virtrng_info {
static void random_recv_done(struct virtqueue *vq) static void random_recv_done(struct virtqueue *vq)
{ {
struct virtrng_info *vi = vq->vdev->priv; struct virtrng_info *vi = vq->vdev->priv;
unsigned int len;
/* We can get spurious callbacks, e.g. shared IRQs + virtio_pci. */ /* We can get spurious callbacks, e.g. shared IRQs + virtio_pci. */
if (!virtqueue_get_buf(vi->vq, &vi->data_avail)) if (!virtqueue_get_buf(vi->vq, &len))
return; return;
vi->data_idx = 0; smp_store_release(&vi->data_avail, len);
complete(&vi->have_data); complete(&vi->have_data);
} }
...@@ -52,7 +53,6 @@ static void request_entropy(struct virtrng_info *vi) ...@@ -52,7 +53,6 @@ static void request_entropy(struct virtrng_info *vi)
struct scatterlist sg; struct scatterlist sg;
reinit_completion(&vi->have_data); reinit_completion(&vi->have_data);
vi->data_avail = 0;
vi->data_idx = 0; vi->data_idx = 0;
sg_init_one(&sg, vi->data, sizeof(vi->data)); sg_init_one(&sg, vi->data, sizeof(vi->data));
...@@ -88,7 +88,7 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait) ...@@ -88,7 +88,7 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
read = 0; read = 0;
/* copy available data */ /* copy available data */
if (vi->data_avail) { if (smp_load_acquire(&vi->data_avail)) {
chunk = copy_data(vi, buf, size); chunk = copy_data(vi, buf, size);
size -= chunk; size -= chunk;
read += chunk; read += chunk;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment