Re: [PATCH v5 8/8] virtio: rng: Add a random number generator(rng) driver

On 12/27/19 12:29 PM, Sughosh Ganu wrote:

On Fri, 27 Dec 2019 at 12:42, Heinrich Schuchardt <xypron.glpk@gmx.de mailto:xypron.glpk@gmx.de> wrote:

On 12/26/19 6:25 PM, Sughosh Ganu wrote:
 > Add a driver for the virtio-rng device on the qemu platform. The
 > device uses pci as a transport medium. The driver can be enabled with
 > the following configs
 >
 > CONFIG_VIRTIO
 > CONFIG_DM_RNG
 > CONFIG_VIRTIO_PCI
 > CONFIG_VIRTIO_RNG
 >
 > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
<mailto:sughosh.ganu@linaro.org>>
 > ---
 > Changes since V4:
 > * Return number of bytes read on a successful read, and a -ve
value on
 >    error.
 >
 >   drivers/virtio/Kconfig         |  6 +++
 >   drivers/virtio/Makefile        |  1 +
 >   drivers/virtio/virtio-uclass.c |  1 +
 >   drivers/virtio/virtio_rng.c    | 85
++++++++++++++++++++++++++++++++++++++++++
 >   include/virtio.h               |  4 +-
 >   5 files changed, 96 insertions(+), 1 deletion(-)
 >   create mode 100644 drivers/virtio/virtio_rng.c
 >

<snip>

 > diff --git a/drivers/virtio/virtio_rng.c
b/drivers/virtio/virtio_rng.c
 > new file mode 100644
 > index 0000000..fda8d04
 > --- /dev/null
 > +++ b/drivers/virtio/virtio_rng.c
 > @@ -0,0 +1,85 @@
 > +// SPDX-License-Identifier: GPL-2.0+
 > +/*
 > + * Copyright (c) 2019, Linaro Limited
 > + */
 > +
 > +#include <common.h>
 > +#include <dm.h>
 > +#include <rng.h>
 > +#include <virtio_types.h>
 > +#include <virtio.h>
 > +#include <virtio_ring.h>
 > +
 > +struct virtio_rng_priv {
 > +     struct virtqueue *rng_vq;
 > +};
 > +
 > +static int virtio_rng_read(struct udevice *dev, void *data,
size_t len)
 > +{
 > +     int ret;
 > +     unsigned int rsize = 0;
 > +     struct virtio_sg sg;
 > +     struct virtio_sg *sgs[1];
 > +     struct virtio_rng_priv *priv = dev_get_priv(dev);
 > +
 > +     /*
 > +      * Only INT_MAX number of bytes can be returned. If more
 > +      * bytes need to be read, the caller must do it in a loop.
 > +      */
 > +     if (len > INT_MAX)
 > +             len = INT_MAX;
 > +
 > +     sg.addr = data;
 > +     sg.length = len;
 > +     sgs[0] = &sg;
 > +
 > +     ret = virtqueue_add(priv->rng_vq, sgs, 0, 1);
 > +     if (ret)
 > +             return ret;
 > +
 > +     virtqueue_kick(priv->rng_vq);
 > +
 > +     while (!virtqueue_get_buf(priv->rng_vq, &rsize))

This code assumes that data is 4 byte aligned and may lead to a crash
due to missing alignment on ARM in function le32_to_cpu().

Are you referring to the virtqueue_get_buf function. Not sure I understand. In any case, I will be sending an updated version which moves the loop into the driver, as you had commented on my other patch.

virtqueue_get_buf() calls virtio32_to_cpu() which calls le32_to_cpu() which assumes that the destination is properly aligned.

Best regards

Heinrich