On Thu, May 04, 2023 at 11:59:32AM +0800, Herbert Xu wrote:
> On Wed, May 03, 2023 at 07:37:00AM -0400, Michael S. Tsirkin wrote:
> >
> > 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
>
> Yes I removed because it's redundant. But you're right I'll add
> a note about it in the log:
>
> ---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.
>
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: f7f510ec1957 ("virtio: An entropy device, as suggested by hpa.")
> Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Acked-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
feel free ro merge, thanks!
>
> 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;
> -
> + 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
