| Message ID | 20210617051004.146-1-xieyongji@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] virtio-blk: Add validation for block size in config space | expand |
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@bytedance.com> > --- > 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.
On Tue, Jun 22, 2021 at 6:11 PM Stefan Hajnoczi <stefanha@redhat.com> 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@bytedance.com> > > --- > > 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. > Good point! I agree that it's better to add the validation in virtblk_probe(). Thanks, Yongji
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@bytedance.com> > > --- > > 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). ---- 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@redhat.com> --- 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);
On Tue, Jul 6, 2021 at 2:23 AM Michael S. Tsirkin <mst@redhat.com> 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@bytedance.com> > > > --- > > > 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@redhat.com> > > --- > > 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 >
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; +} + static int virtblk_probe(struct virtio_device *vdev) { struct virtio_blk *vblk; @@ -707,12 +729,6 @@ static int virtblk_probe(struct virtio_device *vdev) u8 physical_block_exp, alignment_offset; unsigned int queue_depth; - if (!vdev->config->get) { - dev_err(&vdev->dev, "%s failure: config access disabled\n", - __func__); - return -EINVAL; - } - err = ida_simple_get(&vd_index_ida, 0, minor_to_index(1 << MINORBITS), GFP_KERNEL); if (err < 0) @@ -994,6 +1010,7 @@ static struct virtio_driver virtio_blk = { .driver.name = KBUILD_MODNAME, .driver.owner = THIS_MODULE, .id_table = id_table, + .validate = virtblk_validate, .probe = virtblk_probe, .remove = virtblk_remove, .config_changed = virtblk_config_changed,
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@bytedance.com> --- drivers/block/virtio_blk.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)