Re: [PATCH V2 03/12] virtio-console: switch to use .validate()

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

On Thu, Oct 14, 2021 at 10:28:06AM +0800, Jason Wang wrote:
> On Wed, Oct 13, 2021 at 5:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 12, 2021 at 02:52:18PM +0800, Jason Wang wrote:
> > > This patch switches to use validate() to filter out the features that
> > > is not supported by the rproc.
> >
> > are not supported
> >
> > >
> > > Cc: Amit Shah <amit@kernel.org>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> >
> > Does this have anything to do with hardening?
> >
> > It seems cleaner to not negotiate features we do not use,
> > but given we did this for many years ... should we bother
> > at this point?
> 
> It looks cleaner to do all the validation in one places:
> 
> 1) check unsupported feature for rproc
> 2) validate the max_nr_ports (as has been done in patch 4)
> 
> >
> >
> > > ---
> > >  drivers/char/virtio_console.c | 41 ++++++++++++++++++++++-------------
> > >  1 file changed, 26 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > > index 7eaf303a7a86..daeed31df622 100644
> > > --- a/drivers/char/virtio_console.c
> > > +++ b/drivers/char/virtio_console.c
> > > @@ -1172,9 +1172,7 @@ static void resize_console(struct port *port)
> > >
> > >       vdev = port->portdev->vdev;
> > >
> > > -     /* Don't test F_SIZE at all if we're rproc: not a valid feature! */
> > > -     if (!is_rproc_serial(vdev) &&
> > > -         virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> > > +     if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> > >               hvc_resize(port->cons.hvc, port->cons.ws);
> > >  }
> > >
> > > @@ -1981,6 +1979,29 @@ static void virtcons_remove(struct virtio_device *vdev)
> > >       kfree(portdev);
> > >  }
> > >
> > > +static int virtcons_validate(struct virtio_device *vdev)
> > > +{
> > > +     if (is_rproc_serial(vdev)) {
> > > +             /* Don't test F_SIZE at all if we're rproc: not a
> > > +              * valid feature! */
> >
> >
> > This comment needs to be fixed now. And the format's wrong
> > since you made it a multi-line comment.
> > Should be
> >         /*
> >          * like
> >          * this
> >          */
> 
> Ok.
> 
> >
> > > +             __virtio_clear_bit(vdev, VIRTIO_CONSOLE_F_SIZE);
> > > +             /* Don't test MULTIPORT at all if we're rproc: not a
> > > +              * valid feature! */
> > > +             __virtio_clear_bit(vdev, VIRTIO_CONSOLE_F_MULTIPORT);
> > > +     }
> > > +
> > > +     /* We only need a config space if features are offered */
> > > +     if (!vdev->config->get &&
> > > +         (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)
> > > +          || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT))) {
> > > +             dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > > +                     __func__);
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  /*
> > >   * Once we're further in boot, we get probed like any other virtio
> > >   * device.
> >
> > This switches the order of tests around, so if an rproc device
> > offers VIRTIO_CONSOLE_F_SIZE or VIRTIO_CONSOLE_F_MULTIPORT
> > without get it will now try to work instead of failing.
> 
> Ok, so we can fail the validation in this case.

We can. But if we are going to, then it's easier to just fail probe.
Or if you want to try and work around this case,
that's also reasonable but pls document in the commit log.


> Thanks
> 
> >
> > Which is maybe a worthy goal, but given rproc does not support
> > virtio 1.0 it also risks trying to drive something completely
> > unreasonable.
> >
> > Overall does not feel like hardening which is supposed to make
> > things more strict, not less.
> >
> >
> > > @@ -1996,15 +2017,6 @@ static int virtcons_probe(struct virtio_device *vdev)
> > >       bool multiport;
> > >       bool early = early_put_chars != NULL;
> > >
> > > -     /* We only need a config space if features are offered */
> > > -     if (!vdev->config->get &&
> > > -         (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)
> > > -          || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT))) {
> > > -             dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > > -                     __func__);
> > > -             return -EINVAL;
> > > -     }
> > > -
> > >       /* Ensure to read early_put_chars now */
> > >       barrier();
> > >
> > > @@ -2031,9 +2043,7 @@ static int virtcons_probe(struct virtio_device *vdev)
> > >       multiport = false;
> > >       portdev->max_nr_ports = 1;
> > >
> > > -     /* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
> > > -     if (!is_rproc_serial(vdev) &&
> > > -         virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
> > > +     if (virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
> > >                                struct virtio_console_config, max_nr_ports,
> > >                                &portdev->max_nr_ports) == 0) {
> > >               multiport = true;
> > > @@ -2210,6 +2220,7 @@ static struct virtio_driver virtio_console = {
> > >       .driver.name =  KBUILD_MODNAME,
> > >       .driver.owner = THIS_MODULE,
> > >       .id_table =     id_table,
> > > +     .validate =     virtcons_validate,
> > >       .probe =        virtcons_probe,
> > >       .remove =       virtcons_remove,
> > >       .config_changed = config_intr,
> > > --
> > > 2.25.1
> >