Re: Use of uninitialized memory with CONFIG_HW_RANDOM_VIRTIO

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Sep 17, 2021 at 02:57:02PM +0200, Laurent Vivier wrote:
> On 17/09/2021 00:58, Michael S. Tsirkin wrote:
> > On Thu, Sep 16, 2021 at 10:52:59AM +0200, Laurent Vivier wrote:
> > > On 13/09/2021 10:25, Alexander Potapenko wrote:
> > > > Hi Laurent,
> > > > 
> > > > I took the latest kernel (5.15-rc1,
> > > > 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f) and a slightly modified
> > > > config from syzbot (see attached)
> > > > The latter has a lot of unnecessary debug checks, but those should not
> > > > affect the RNG.
> > > > 
> > > > You then need to apply the following patch to the kernel:
> > > > 
> > > > ====================================================
> > > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > > > index a3db27916256d..a4cba9f0ff8cb 100644
> > > > --- a/drivers/char/hw_random/core.c
> > > > +++ b/drivers/char/hw_random/core.c
> > > > @@ -433,8 +433,11 @@ static int hwrng_fillfn(void *unused)
> > > >                   if (IS_ERR(rng) || !rng)
> > > >                           break;
> > > >                   mutex_lock(&reading_mutex);
> > > > +               memset(rng_fillbuf, 'A', rng_buffer_size());
> > > > +               rng_fillbuf[rng_buffer_size()-1] = 0;
> > > >                   rc = rng_get_data(rng, rng_fillbuf,
> > > >                                     rng_buffer_size(), 1);
> > > > +               pr_err("rng_fillbuf: %s\n", rng_fillbuf);
> > > >                   mutex_unlock(&reading_mutex);
> > > >                   put_rng(rng);
> > > >                   if (rc <= 0) {
> > > > ====================================================
> > > > 
> > > > and run the kernel under QEMU.
> > > > 
> > > > On my machine I'm seeing the following output:
> > > > 
> > > > $ cat log | strings | grep rng_fillbuf
> > > > [    4.901931][  T897] rng_fillbuf:
> > > > AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
> > > > [    4.903104][  T897] rng_fillbuf: >
> > > > [    4.903641][  T897] rng_fillbuf:
> > > > [    4.904846][  T897] rng_fillbuf: ?
> > > > [    4.913442][  T897] rng_fillbuf: [
> > > > 
> > > > , which denotes that the first call to rng_get_data() leaves
> > > > rng_fillbuf uninitialized.
> > > 
> > > 
> > > Thank you for the detailed steps.
> > > 
> > > The problem happens because we mix two different buffers:
> > > - in add_early_randomness() we provide rng_buffer but don't wait it is full (see [1])
> > > - in hwrng_fillfn() we provide rng_fillbuf, and we wait data here, but we
> > > received the signal from QEMU that there are data, but these data are in
> > > rng_buffer while we expect them in rng_fillbuf.
> > > 
> > > There are several ways to fix/workaround that:
> > > 
> > > 1- ignore the read when wait=0 :
> > > 
> > > diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> > > index a90001e02bf7..8466d76566fd 100644
> > > --- a/drivers/char/hw_random/virtio-rng.c
> > > +++ b/drivers/char/hw_random/virtio-rng.c
> > > @@ -59,15 +59,15 @@ static int virtio_read(struct hwrng *rng, void *buf,
> > > size_t size, bool wait)
> > >          if (vi->hwrng_removed)
> > >                  return -ENODEV;
> > > 
> > > +       if (!wait)
> > > +               return 0;
> > > +
> > >          if (!vi->busy) {
> > >                  vi->busy = true;
> > >                  reinit_completion(&vi->have_data);
> > >                  register_buffer(vi, buf, size);
> > >          }
> > > 
> > > -       if (!wait)
> > > -               return 0;
> > > -
> > >          ret = wait_for_completion_killable(&vi->have_data);
> > >          if (ret < 0)
> > >                  return ret;
> > > 
> > > 
> > > 2- Use an internal intermediate buffer in virtio-rng, at a cost of a copy,
> > >     I have some patches (somewhere) I can refresh to do that.
> > > 
> > > 3- modify hw_random/core.c to use only one buffer
> > > 
> > > Thanks,
> > > Laurent
> > > 
> > > [1] 78887832e765 ("hwrng: core - don't wait on add_early_randomness()")
> > 
> > 4. actually differentiate between the two
> > using the pointer returned by get_buf.
> 
> Even if it can help I think we should avoid to keep mixing buffers.
> 
> For instance, if we submit a buffer with wait=0, the caller can re-use or
> release the memory while it is queued in the queue of the device.
> 
> Moreover, what to do if buffers differ?
> 
> Wait and use the data in the previous buffer (that can be corrupted by the
> submitter in-between)?
> 
> Or wait and drop, and wait again with the new buffer?
> 
> BTW, I found my patches that introduce an internal buffer in virtio-rng (solution 2):
> 
> https://github.com/vivier/linux/commits/virtio-rng
> 
> Thanks,
> Laurent

Pls go ahead and post them!

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization