Virtualization - Dec 2014 - [PATCH RFC v6 12/20] virtio: disallow late feature changes for virtio-1

On Thu, 11 Dec 2014 14:25:14 +0100
Cornelia Huck <cornelia.huck at de.ibm.com> wrote:
> For virtio-1 devices, the driver must not attempt to set feature bits
> after it set FEATURES_OK in the device status. Simply reject it in
> that case.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
> ---
>  hw/virtio/virtio.c         |   16 ++++++++++++++--
>  include/hw/virtio/virtio.h |    2 ++
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 57190ba..a3dd67b 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -978,7 +978,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>      vmstate_save_state(f, &vmstate_virtio, vdev);
>  }
> 
> -int virtio_set_features(VirtIODevice *vdev, uint64_t val)
> +static int __virtio_set_features(VirtIODevice *vdev, uint64_t val)
Maybe avoid the double underscores here? But unfortunately, I also fail
to come up with a better suggestion for a name here ...
>  {
>      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>      VirtioBusClass *vbusk = VIRTIO_BUS_GET_CLASS(qbus);
> @@ -994,6 +994,18 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t
val)
>      return bad ? -1 : 0;
>  }
> 
> +int virtio_set_features(VirtIODevice *vdev, uint64_t val)
> +{
> +   /*
> +     * The driver must not attempt to set features after feature
negotiation
> +     * has finished.
> +     */
> +    if (vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) {
> +        return -EINVAL;
> +    }
Hmm, according to your patch description, the FEATURES_OK check only
applies to virtio-1.0 devices ... so shouldn't there be a check for
virtio-1 here? Or did I miss something?
> +    return __virtio_set_features(vdev, val);
> +}
 Thomas

On Fri, 12 Dec 2014 11:55:38 +0100
Thomas Huth <thuth at linux.vnet.ibm.com> wrote:
> On Thu, 11 Dec 2014 14:25:14 +0100
> Cornelia Huck <cornelia.huck at de.ibm.com> wrote:
> 
> > For virtio-1 devices, the driver must not attempt to set feature bits
> > after it set FEATURES_OK in the device status. Simply reject it in
> > that case.
> > 
> > Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
> > ---
> >  hw/virtio/virtio.c         |   16 ++++++++++++++--
> >  include/hw/virtio/virtio.h |    2 ++
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 57190ba..a3dd67b 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -978,7 +978,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >      vmstate_save_state(f, &vmstate_virtio, vdev);
> >  }
> > 
> > -int virtio_set_features(VirtIODevice *vdev, uint64_t val)
> > +static int __virtio_set_features(VirtIODevice *vdev, uint64_t val)
> 
> Maybe avoid the double underscores here? But unfortunately, I also fail
> to come up with a better suggestion for a name here ...
virtio_set_features_nocheck()?

This function is only called within virtio.c anyway...
> 
> >  {
> >      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> >      VirtioBusClass *vbusk = VIRTIO_BUS_GET_CLASS(qbus);
> > @@ -994,6 +994,18 @@ int virtio_set_features(VirtIODevice *vdev,
uint64_t val)
> >      return bad ? -1 : 0;
> >  }
> > 
> > +int virtio_set_features(VirtIODevice *vdev, uint64_t val)
> > +{
> > +   /*
> > +     * The driver must not attempt to set features after feature
negotiation
> > +     * has finished.
> > +     */
> > +    if (vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) {
> > +        return -EINVAL;
> > +    }
> 
> Hmm, according to your patch description, the FEATURES_OK check only
> applies to virtio-1.0 devices ... so shouldn't there be a check for
> virtio-1 here? Or did I miss something?
A device in legacy mode will never have FEATURES_OK set. But it is a
bit non-obvious - maybe adding a check for VERSION_1 does not hurt.
> 
> > +    return __virtio_set_features(vdev, val);
> > +}
> 
>  Thomas

On Fri, 12 Dec 2014 12:18:25 +0100
Cornelia Huck <cornelia.huck at de.ibm.com> wrote:
> On Fri, 12 Dec 2014 11:55:38 +0100
> Thomas Huth <thuth at linux.vnet.ibm.com> wrote:
> 
> > On Thu, 11 Dec 2014 14:25:14 +0100
> > Cornelia Huck <cornelia.huck at de.ibm.com> wrote:
> > 
> > > For virtio-1 devices, the driver must not attempt to set feature
bits
> > > after it set FEATURES_OK in the device status. Simply reject it
in
> > > that case.
> > > 
> > > Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
> > > ---
> > >  hw/virtio/virtio.c         |   16 ++++++++++++++--
> > >  include/hw/virtio/virtio.h |    2 ++
> > >  2 files changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 57190ba..a3dd67b 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -978,7 +978,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile
*f)
> > >      vmstate_save_state(f, &vmstate_virtio, vdev);
> > >  }
> > > 
> > > -int virtio_set_features(VirtIODevice *vdev, uint64_t val)
> > > +static int __virtio_set_features(VirtIODevice *vdev, uint64_t
val)
> > 
> > Maybe avoid the double underscores here? But unfortunately, I also
fail
> > to come up with a better suggestion for a name here ...
> 
> virtio_set_features_nocheck()?
Sounds ok to me.
> This function is only called within virtio.c anyway...
Right, so the double underscores should be ok here, too. (I still do
not like them very much, but that's just my personal taste in this case)
> > >  {
> > >      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> > >      VirtioBusClass *vbusk = VIRTIO_BUS_GET_CLASS(qbus);
> > > @@ -994,6 +994,18 @@ int virtio_set_features(VirtIODevice *vdev,
uint64_t val)
> > >      return bad ? -1 : 0;
> > >  }
> > > 
> > > +int virtio_set_features(VirtIODevice *vdev, uint64_t val)
> > > +{
> > > +   /*
> > > +     * The driver must not attempt to set features after feature
negotiation
> > > +     * has finished.
> > > +     */
> > > +    if (vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) {
> > > +        return -EINVAL;
> > > +    }
> > 
> > Hmm, according to your patch description, the FEATURES_OK check only
> > applies to virtio-1.0 devices ... so shouldn't there be a check
for
> > virtio-1 here? Or did I miss something?
> 
> A device in legacy mode will never have FEATURES_OK set. But it is a
> bit non-obvious - maybe adding a check for VERSION_1 does not hurt.
Ah, ok, right, and if it is a legacy device and has FEATURES_OK set, it
is certainly a misbehavior wrt the legacy protocol. So it really should
be ok or even good to _not_ check for virtio-1.0 here. So sorry for the
confusion, I think now the patch is good as it is:

Reviewed-by: Thomas Huth <thuth at linux.vnet.ibm.com>