* [PATCH v3] virtio-blk: Add validation for block size in config space @ 2021-06-17 5:10 Xie Yongji 2021-06-22 10:11 ` Stefan Hajnoczi 0 siblings, 1 reply; 5+ messages in thread From: Xie Yongji @ 2021-06-17 5:10 UTC (permalink / raw) To: mst, jasowang, stefanha, axboe; +Cc: virtualization, linux-block, linux-kernel 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; +} + 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, -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] virtio-blk: Add validation for block size in config space 2021-06-17 5:10 [PATCH v3] virtio-blk: Add validation for block size in config space Xie Yongji @ 2021-06-22 10:11 ` Stefan Hajnoczi 2021-06-22 11:49 ` Yongji Xie 2021-07-05 18:23 ` Michael S. Tsirkin 0 siblings, 2 replies; 5+ messages in thread From: Stefan Hajnoczi @ 2021-06-22 10:11 UTC (permalink / raw) To: Xie Yongji Cc: mst, jasowang, axboe, virtualization, linux-block, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1690 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Re: [PATCH v3] virtio-blk: Add validation for block size in config space 2021-06-22 10:11 ` Stefan Hajnoczi @ 2021-06-22 11:49 ` Yongji Xie 2021-07-05 18:23 ` Michael S. Tsirkin 1 sibling, 0 replies; 5+ messages in thread From: Yongji Xie @ 2021-06-22 11:49 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Michael S. Tsirkin, Jason Wang, Jens Axboe, virtualization, linux-block, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] virtio-blk: Add validation for block size in config space 2021-06-22 10:11 ` Stefan Hajnoczi 2021-06-22 11:49 ` Yongji Xie @ 2021-07-05 18:23 ` Michael S. Tsirkin 2021-07-06 3:57 ` Yongji Xie 1 sibling, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2021-07-05 18:23 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Xie Yongji, jasowang, axboe, virtualization, linux-block, linux-kernel 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); -- MST ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] virtio-blk: Add validation for block size in config space 2021-07-05 18:23 ` Michael S. Tsirkin @ 2021-07-06 3:57 ` Yongji Xie 0 siblings, 0 replies; 5+ messages in thread From: Yongji Xie @ 2021-07-06 3:57 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Stefan Hajnoczi, Jason Wang, Jens Axboe, virtualization, linux-block, linux-kernel 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-06 3:58 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-17 5:10 [PATCH v3] virtio-blk: Add validation for block size in config space Xie Yongji 2021-06-22 10:11 ` Stefan Hajnoczi 2021-06-22 11:49 ` Yongji Xie 2021-07-05 18:23 ` Michael S. Tsirkin 2021-07-06 3:57 ` Yongji Xie
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).