thr3ads.net - Virtualization - [PATCH V2 03/12] virtio-console: switch to use .validate() [Oct 2021]

If this information is useful, please help other people find it:
Share via:

Michael S. Tsirkin

2021-Oct-13 09:50 UTC

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

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 at kernel.org>
> Signed-off-by: Jason Wang <jasowang at 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?

> ---
>  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
	 */
> +		__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.

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

Jason Wang

2021-Oct-14 02:28 UTC

head link

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

On Wed, Oct 13, 2021 at 5:51 PM Michael S. Tsirkin <mst at 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 at kernel.org>
> > Signed-off-by: Jason Wang <jasowang at 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
>

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

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

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