Jason Wang
2021-Aug-10 03:05 UTC
[PATCH v5] virtio-blk: Add validation for block size in config space
? 2021/8/9 ??6:16, Xie Yongji ??:> An untrusted device might presents an invalid block size > in configuration space. This tries to add validation for it > in the validate callback and clear the VIRTIO_BLK_F_BLK_SIZE > feature bit if the value is out of the supported range. > > And we also double check the value in virtblk_probe() in > case that it's changed after the validation. > > Signed-off-by: Xie Yongji <xieyongji at bytedance.com> > --- > drivers/block/virtio_blk.c | 39 +++++++++++++++++++++++++++++++++------ > 1 file changed, 33 insertions(+), 6 deletions(-) > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index 4b49df2dfd23..afb37aac09e8 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -692,6 +692,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);I wonder if it's better to just fail here as what we did for probe(). Thanks> + > + return 0; > +} > + > static int virtblk_probe(struct virtio_device *vdev) > { > struct virtio_blk *vblk; > @@ -703,12 +725,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) > @@ -823,6 +839,14 @@ static int virtblk_probe(struct virtio_device *vdev) > else > blk_size = queue_logical_block_size(q); > > + if (unlikely(blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)) { > + dev_err(&vdev->dev, > + "block size is changed unexpectedly, now is %u\n", > + blk_size); > + err = -EINVAL; > + goto err_cleanup_disk; > + } > + > /* Use topology information if available */ > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY, > struct virtio_blk_config, physical_block_exp, > @@ -881,6 +905,8 @@ static int virtblk_probe(struct virtio_device *vdev) > device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups); > return 0; > > +err_cleanup_disk: > + blk_cleanup_disk(vblk->disk); > out_free_tags: > blk_mq_free_tag_set(&vblk->tag_set); > out_free_vq: > @@ -983,6 +1009,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,