On Wed, 6 Apr 2022 at 15:18, Pierre-Clément Tosi wrote:
>
> Hi,
>
> On Thu, Mar 31, 2022 at 10:09:48AM +, Andrew Scull wrote:
> > Check the length of data written by the device is consistent with the
> > size of the buffers to avoid out-of-bounds memory accesses in case
> > values aren't consistent.
> >
> > Signed-off-by: Andrew Scull
> > Cc: Sughosh Ganu
> > ---
> > drivers/virtio/virtio_rng.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_rng.c b/drivers/virtio/virtio_rng.c
> > index 9314c0a03e..b85545c2ee 100644
> > --- a/drivers/virtio/virtio_rng.c
> > +++ b/drivers/virtio/virtio_rng.c
> > @@ -41,6 +41,9 @@ static int virtio_rng_read(struct udevice *dev, void
> > *data, size_t len)
> > while (!virtqueue_get_buf(priv->rng_vq, ))
> > ;
> >
> > + if (rsize > sg.length)
> > + return -EIO;
> > +
>
> Although this patch addresses a legitimate concern, could we instead aim for
> strengthening the lower-level virtio building blocks (e.g.
> virtqueue_get_buf())
> so that higher-level virtio device drivers such as
> virtio-{rng,console,net,...}
> don't have to be littered with checks of this nature? Could this be achieved
> by
> using the shadow copy introduced in [PATCH 03/11]?
There could certainly be _a_ bounds check in the vring driver, to
check that the total size written doesn't exceed the cumulative size
of the writable buffers in the descriptor chain. That would be
satisfactory for this rng driver, since there is only one buffer being
used, but if there is more than one buffer then the device driver will
still need to do checks as it accesses each of them. So it still feels
like the device driver's responsibility to do the checking, given the
current API.
> > memcpy(ptr, buf, rsize);
> > len -= rsize;
> > ptr += rsize;
> > --
> > 2.35.1.1094.g7c7d902a7c-goog
> >
>
> Thanks,
>
> --
> Pierre
