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

From: Jason Wang <jasowang@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization <virtualization@lists.linux-foundation.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	"Hetzelt, Felicitas" <f.hetzelt@tu-berlin.de>,
	"kaplan, david" <david.kaplan@amd.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Amit Shah <amit@kernel.org>
Subject: Re: [PATCH V2 03/12] virtio-console: switch to use .validate()
Date: Thu, 14 Oct 2021 10:28:06 +0800	[thread overview]
Message-ID: <CACGkMEvYu4rMnhLtQfPo-BKME+cT9Sk1b39++f3BXZm1fTQHMQ@mail.gmail.com> (raw)
In-Reply-To: <20211013054334-mutt-send-email-mst@kernel.org>

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.

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
>