On Wed, May 03, 2023 at 06:54:36PM +0800, Herbert Xu wrote: > On Fri, Apr 21, 2023 at 04:52:13PM +0200, Dmitry Vyukov wrote: > > > > Here this: > > > > size = min_t(unsigned int, size, vi->data_avail); > > memcpy(buf, vi->data + vi->data_idx, size); > > vi->data_idx += size; > > vi->data_avail -= size; > > > > runs concurrently with: > > > > if (!virtqueue_get_buf(vi->vq, &vi->data_avail)) > > return; > > vi->data_idx = 0; > > > > I did not fully grasp how/where vi->data is populated, but it looks > > like it can lead to use of uninit/stale random data, or even to out of > > bounds access, say if vi->data_avail is already updated, but > > vi->data_idx is not yet reset to 0. Then concurrent reading will read > > not where it's supposed to read. > > Yes this is a real race. This bug appears to have been around > forever. > > ---8<--- > 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. > > Reported-by: syzbot+726dc8c62c3536431ceb@xxxxxxxxxxxxxxxxxxxxxxxxx > Fixes: f7f510ec1957 ("virtio: An entropy device, as suggested by hpa.") > Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> > > diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c > index f7690e0f92ed..e41a84e6b4b5 100644 > --- a/drivers/char/hw_random/virtio-rng.c > +++ b/drivers/char/hw_random/virtio-rng.c > @@ -4,6 +4,7 @@ > * Copyright (C) 2007, 2008 Rusty Russell IBM Corporation > */ > > +#include <asm/barrier.h> > #include <linux/err.h> > #include <linux/hw_random.h> > #include <linux/scatterlist.h> > @@ -37,13 +38,13 @@ struct virtrng_info { > static void random_recv_done(struct virtqueue *vq) > { > struct virtrng_info *vi = vq->vdev->priv; > + unsigned int len; > > /* 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; > > - vi->data_idx = 0; > - On the surface of it, it looks like you removed this store which isn't described in the commit log. I do not, offhand, remember why we stored 0 in data_idx here when we also zero it in request_entropy. It was added with commit 5c8e933050044d6dd2a000f9a5756ae73cbe7c44 Author: Laurent Vivier <lvivier@xxxxxxxxxx> Date: Thu Oct 28 12:11:10 2021 +0200 hwrng: virtio - don't waste entropy if we don't use all the entropy available in the buffer, keep it and use it later. Signed-off-by: Laurent Vivier <lvivier@xxxxxxxxxx> Link: https://lore.kernel.org/r/20211028101111.128049-4-lvivier@xxxxxxxxxx Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx> > + smp_store_release(&vi->data_avail, len); > complete(&vi->have_data); > } > > @@ -52,7 +53,6 @@ static void request_entropy(struct virtrng_info *vi) > struct scatterlist sg; > > reinit_completion(&vi->have_data); > - vi->data_avail = 0; > vi->data_idx = 0; > > 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) > read = 0; > > /* copy available data */ > - if (vi->data_avail) { > + if (smp_load_acquire(&vi->data_avail)) { > chunk = copy_data(vi, buf, size); > size -= chunk; > read += chunk; > -- > Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt