Re: [PATCH v3] virtio-blk: Add validation for block size in config space

[Yongji Xie]

On Tue, Jul 6, 2021 at 2:23 AM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>
> On Tue, Jun 22, 2021 at 11:11:06AM +0100, Stefan Hajnoczi wrote:
> > On Thu, Jun 17, 2021 at 01:10:04PM +0800, Xie Yongji wrote:
> > > This ensures that we will not use an invalid block size
> > > in config space (might come from an untrusted device).
> > >
> > > Signed-off-by: Xie Yongji <xieyongji@xxxxxxxxxxxxx>
> > > ---
> > >  drivers/block/virtio_blk.c | 29 +++++++++++++++++++++++------
> > >  1 file changed, 23 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > index b9fa3ef5b57c..bbdae989f1ea 100644
> > > --- a/drivers/block/virtio_blk.c
> > > +++ b/drivers/block/virtio_blk.c
> > > @@ -696,6 +696,28 @@ static const struct blk_mq_ops virtio_mq_ops = {
> > >  static unsigned int virtblk_queue_depth;
> > >  module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
> > >
> > > +static int virtblk_validate(struct virtio_device *vdev)
> > > +{
> > > +   u32 blk_size;
> > > +
> > > +   if (!vdev->config->get) {
> > > +           dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > > +                   __func__);
> > > +           return -EINVAL;
> > > +   }
> > > +
> > > +   if (!virtio_has_feature(vdev, VIRTIO_BLK_F_BLK_SIZE))
> > > +           return 0;
> > > +
> > > +   blk_size = virtio_cread32(vdev,
> > > +                   offsetof(struct virtio_blk_config, blk_size));
> > > +
> > > +   if (blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)
> > > +           __virtio_clear_bit(vdev, VIRTIO_BLK_F_BLK_SIZE);
> > > +
> > > +   return 0;
> > > +}
> >
> > I saw Michael asked for .validate() in v2. I would prefer to keep
> > everything in virtblk_probe() instead of adding .validate() because:
> >
> > - There is a race condition that an untrusted device can exploit since
> >   virtblk_probe() fetches the value again.
> >
> > - It's more complex now that .validate() takes a first shot at blk_size
> >   and then virtblk_probe() deals with it again later on.
>
> This is a valid concern.
> But, silently ignoring what hypervisor told us to do is also ungood.
> Let's save it somewhere then.
> And there are more examples like this, e.g. the virtio net mtu.
>
> So if we worry about this stuff, let's do something along the lines of
>
> (note: incomplete, won't build: you need to update all drivers).

How about reusing the priv field in struct virtio_device to avoid
changing the interface of virtio_driver?

Thanks,
Yongji

> ----
>
>
> virtio: allow passing config data from validate callback
>
> To avoid time of check to time of use races on config changes,
> pass config data from validate callback to probe.
>
> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
>
> ---
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 4b15c00c0a0a..d3657a0127d7 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -211,6 +211,7 @@ static int virtio_dev_probe(struct device *_d)
>         u64 device_features;
>         u64 driver_features;
>         u64 driver_features_legacy;
> +       void *config = NULL;
>
>         /* We have a driver! */
>         virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> @@ -249,18 +250,20 @@ static int virtio_dev_probe(struct device *_d)
>                         __virtio_set_bit(dev, i);
>
>         if (drv->validate) {
> -               err = drv->validate(dev);
> -               if (err)
> +               config = drv->validate(dev);
> +               if (IS_ERR(config)) {
> +                       err = PTR_ERR(config);
>                         goto err;
> +               }
>         }
>
>         err = virtio_finalize_features(dev);
>         if (err)
>                 goto err;
>
> -       err = drv->probe(dev);
> +       err = drv->probe(dev, config);
>         if (err)
> -               goto err;
> +               goto probe;
>
>         /* If probe didn't do it, mark device DRIVER_OK ourselves. */
>         if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
> @@ -269,9 +272,12 @@ static int virtio_dev_probe(struct device *_d)
>         if (drv->scan)
>                 drv->scan(dev);
>
> +       kfree(config);
>         virtio_config_enable(dev);
>
>         return 0;
> +probe:
> +       kfree(config);
>  err:
>         virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>         return err;
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index b1894e0323fa..90750567c0cc 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -151,6 +151,8 @@ size_t virtio_max_dma_size(struct virtio_device *vdev);
>   * @feature_table_size: number of entries in the feature table array.
>   * @feature_table_legacy: same as feature_table but when working in legacy mode.
>   * @feature_table_size_legacy: number of entries in feature table legacy array.
> + * @validate: the function to validate feature bits and config.
> + *              Returns a valid config or NULL to be passed to probe or ERR_PTR(-errno).
>   * @probe: the function to call when a device is found.  Returns 0 or -errno.
>   * @scan: optional function to call after successful probe; intended
>   *    for virtio-scsi to invoke a scan.
> @@ -167,8 +169,8 @@ struct virtio_driver {
>         unsigned int feature_table_size;
>         const unsigned int *feature_table_legacy;
>         unsigned int feature_table_size_legacy;
> -       int (*validate)(struct virtio_device *dev);
> -       int (*probe)(struct virtio_device *dev);
> +       void *(*validate)(struct virtio_device *dev);
> +       int (*probe)(struct virtio_device *dev, void *config);
>         void (*scan)(struct virtio_device *dev);
>         void (*remove)(struct virtio_device *dev);
>         void (*config_changed)(struct virtio_device *dev);
> --
> MST
>