On Wed, Oct 13, 2021 at 5:51 PM Michael S. Tsirkin <mst@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
>
>
> 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
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
