On Fri, Jun 27, 2014 at 03:42:10PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (m...@redhat.com) wrote:
> > On Fri, Jun 27, 2014 at 09:34:38AM +0100, Dr. David Alan Gilbert (git)
> > wrote:
> > > From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
> > >
> > > Commit 'virtio: validate config_len on load' restricted config_len
> > > loaded from the wire to match the config_len that the device had.
> > >
> > > Unfortunately, there are cases where this isn't true, the one
> > > we found it on was the wqe addition in virtio-blk.
> >
> > I think you mean wce.
>
> Oops - yes.
>
> > > Allow mismatched config-lengths:
> > > *) If the version on the wire is shorter then ensure that the
> > > remainder is 0xff filled (as virtio_config_read does on
> > > out of range reads)
> > > *) If the version on the wire is longer, load what we have space
> > > for and skip the rest.
> > >
> > > Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> >
> > Looks good overall, but I am having thoughts about the
> > padding with 0xff.
> > We previously didn't do this (before virtio: validate config_len on
> > load) so it seems safest (at least for 2.1) not to do it now either.
>
> Who allocates that memory? If it's known to be a value then I agree; however
> if it's uninitialised then I think it's best to pick a value rather than
> have behaviour that depends on random junk in the memory.
It's initialized: e.g. for net it includes the mac,
for block the wce value.
> > > ---
> > > hw/virtio/virtio.c | 30 ++++++++++++++++++++++++++----
> > > 1 file changed, 26 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index a3082d5..2b11142 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -927,11 +927,33 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> > > }
> > > config_len = qemu_get_be32(f);
> > > if (config_len != vdev->config_len) {
> > > - error_report("Unexpected config length 0x%x. Expected 0x%zx",
> > > - config_len, vdev->config_len);
> > > - return -1;
> > > + /*
> > > + * Unfortunately the reality is that there are cases where we
> > > + * see mismatched config lengths, so we have to deal with them
> > > + * rather than rejecting them.
> > > + */
> > > +
> >
> > Drop extra line please.
> >
> > > + if (config_len < vdev->config_len) {
> > > + /* This is normal in some devices when they add a new option
> > > */
> > > + memset(vdev->config, 0xff, vdev->config_len);
> > > + qemu_get_buffer(f, vdev->config, config_len);
> > > + } else {
> > > + int32_t diff;
> > > + /* config_len > vdev->config_len
> > > + * This is rarer, but is here to allow us to fix the case
> > > above
> > > + */
> > > + qemu_get_buffer(f, vdev->config, vdev->config_len);
> > > + /*
> > > + * Even though we expect the diff to be small, we can't use
> > > + * qemu_file_skip because it's not safe for a large skip.
> > > + */
> > > + for (diff = config_len - vdev->config_len; diff > 0; diff--)
> > > {
> > > + qemu_get_byte(f);
> > > + }
> > > + }
> > > + } else {
> > > + qemu_get_buffer(f, vdev->config, vdev->config_len);
> > > }
> > > - qemu_get_buffer(f, vdev->config, vdev->config_len);
> > >
> > > num = qemu_get_be32(f);
> >
> >
> > So I would say handle config_len < vdev->config_len and config_len ==
> > vdev->config_len the same:
> >
> > qemu_get_buffer(f, vdev->config, MIN(config_len, vdev->config_len));
> >
> > and then skip the remainder if any
> > while (config_len > vdev->config_len) {
> > qemu_get_byte(f);
> > config_len--;
> > }
>
> That probably still needs that MIN splitting out (int32_t vs size_t); but
> other than that I guess I can redo that.
>
> Dave
>
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
