* [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-08-09 10:16 Xie Yongji 2021-08-10 3:05 ` Jason Wang ` (2 more replies) 0 siblings, 3 replies; 48+ messages in thread From: Xie Yongji @ 2021-08-09 10:16 UTC (permalink / raw) To: mst, jasowang, stefanha; +Cc: virtualization, linux-block, linux-kernel 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@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); + + 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, -- 2.11.0 ^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-09 10:16 [PATCH v5] virtio-blk: Add validation for block size in config space Xie Yongji @ 2021-08-10 3:05 ` Jason Wang 2021-08-22 23:17 ` Max Gurtovoy 2021-10-04 15:27 ` Michael S. Tsirkin 2 siblings, 0 replies; 48+ messages in thread From: Jason Wang @ 2021-08-10 3:05 UTC (permalink / raw) To: Xie Yongji, mst, stefanha; +Cc: virtualization, linux-block, linux-kernel 在 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@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, ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-08-10 3:05 ` Jason Wang 0 siblings, 0 replies; 48+ messages in thread From: Jason Wang @ 2021-08-10 3:05 UTC (permalink / raw) To: Xie Yongji, mst, stefanha; +Cc: linux-block, linux-kernel, virtualization 在 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@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, _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-10 3:05 ` Jason Wang (?) @ 2021-08-10 4:59 ` Yongji Xie 2021-08-10 6:59 ` Jason Wang -1 siblings, 1 reply; 48+ messages in thread From: Yongji Xie @ 2021-08-10 4:59 UTC (permalink / raw) To: Jason Wang Cc: Michael S. Tsirkin, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Tue, Aug 10, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 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@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(). > Looks like we don't need to do that since we already clear the VIRTIO_BLK_F_BLK_SIZE to tell the device that we don't use the block size in configuration space. Just like what we did in virtnet_validate(). Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-10 4:59 ` Yongji Xie @ 2021-08-10 6:59 ` Jason Wang 0 siblings, 0 replies; 48+ messages in thread From: Jason Wang @ 2021-08-10 6:59 UTC (permalink / raw) To: Yongji Xie Cc: Michael S. Tsirkin, Stefan Hajnoczi, virtualization, linux-block, linux-kernel 在 2021/8/10 下午12:59, Yongji Xie 写道: > On Tue, Aug 10, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote: >> >> 在 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@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(). >> > Looks like we don't need to do that since we already clear the > VIRTIO_BLK_F_BLK_SIZE to tell the device that we don't use the block > size in configuration space. Just like what we did in > virtnet_validate(). > > Thanks, > Yongji Ok, so Acked-by: Jason Wang <jasowang@redhat.com> > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-08-10 6:59 ` Jason Wang 0 siblings, 0 replies; 48+ messages in thread From: Jason Wang @ 2021-08-10 6:59 UTC (permalink / raw) To: Yongji Xie Cc: linux-block, virtualization, linux-kernel, Stefan Hajnoczi, Michael S. Tsirkin 在 2021/8/10 下午12:59, Yongji Xie 写道: > On Tue, Aug 10, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote: >> >> 在 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@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(). >> > Looks like we don't need to do that since we already clear the > VIRTIO_BLK_F_BLK_SIZE to tell the device that we don't use the block > size in configuration space. Just like what we did in > virtnet_validate(). > > Thanks, > Yongji Ok, so Acked-by: Jason Wang <jasowang@redhat.com> > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-09 10:16 [PATCH v5] virtio-blk: Add validation for block size in config space Xie Yongji 2021-08-10 3:05 ` Jason Wang @ 2021-08-22 23:17 ` Max Gurtovoy 2021-08-23 4:31 ` Yongji Xie 2021-10-04 15:27 ` Michael S. Tsirkin 2 siblings, 1 reply; 48+ messages in thread From: Max Gurtovoy @ 2021-08-22 23:17 UTC (permalink / raw) To: Xie Yongji, mst, jasowang, stefanha Cc: virtualization, linux-block, linux-kernel On 8/9/2021 1:16 PM, Xie Yongji wrote: > 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. This is not clear to me. What is untrusted device ? is it a buggy device ? What is the return value for the blk_size in this case that you try to override ? > > 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@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); is it PAGE_SIZE or SZ_4K ? Do we support a 64K blk size (PPC PAGE_SIZE) > + > + 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, ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-22 23:17 ` Max Gurtovoy @ 2021-08-23 4:31 ` Yongji Xie 2021-08-23 8:07 ` Max Gurtovoy 0 siblings, 1 reply; 48+ messages in thread From: Yongji Xie @ 2021-08-23 4:31 UTC (permalink / raw) To: Max Gurtovoy Cc: Michael S. Tsirkin, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > > > On 8/9/2021 1:16 PM, Xie Yongji wrote: > > 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. > > This is not clear to me. What is untrusted device ? is it a buggy device ? > A buggy device, the devices in an encrypted VM, or a userspace device created by VDUSE [1]. [1] https://lore.kernel.org/kvm/20210818120642.165-1-xieyongji@bytedance.com/ > What is the return value for the blk_size in this case that you try to > override ? > The value that is larger than PAGE_SIZE. I think the block layer can not handle the block size that is larger than PAGE_SIZE correctly, e.g. in block_read_full_page(). > > > > > 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@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); > > is it PAGE_SIZE or SZ_4K ? > > Do we support a 64K blk size (PPC PAGE_SIZE) > I think PAGE_SIZE should be OK here. I didn't see a hard 4K limitation in the kernel. NBD did the same check: static int nbd_set_size(struct nbd_device *nbd, loff_t bytesize, loff_t blksize) { if (!blksize) blksize = NBD_DEF_BLKSIZE; if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) return -EINVAL; Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 4:31 ` Yongji Xie @ 2021-08-23 8:07 ` Max Gurtovoy 2021-08-23 8:35 ` Yongji Xie 0 siblings, 1 reply; 48+ messages in thread From: Max Gurtovoy @ 2021-08-23 8:07 UTC (permalink / raw) To: Yongji Xie Cc: Michael S. Tsirkin, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On 8/23/2021 7:31 AM, Yongji Xie wrote: > On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >> >> On 8/9/2021 1:16 PM, Xie Yongji wrote: >>> 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. >> This is not clear to me. What is untrusted device ? is it a buggy device ? >> > A buggy device, the devices in an encrypted VM, or a userspace device > created by VDUSE [1]. > > [1] https://lore.kernel.org/kvm/20210818120642.165-1-xieyongji@bytedance.com/ if it's a userspace device, why don't you fix its control path code instead of adding workarounds in the kernel driver ? > >> What is the return value for the blk_size in this case that you try to >> override ? >> > The value that is larger than PAGE_SIZE. I think the block layer can > not handle the block size that is larger than PAGE_SIZE correctly, > e.g. in block_read_full_page(). > >>> 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@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); >> is it PAGE_SIZE or SZ_4K ? >> >> Do we support a 64K blk size (PPC PAGE_SIZE) >> > I think PAGE_SIZE should be OK here. I didn't see a hard 4K limitation > in the kernel. NBD did the same check: > > static int nbd_set_size(struct nbd_device *nbd, loff_t bytesize, loff_t blksize) > { > if (!blksize) > blksize = NBD_DEF_BLKSIZE; > if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) > return -EINVAL; > > Thanks, > Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 8:07 ` Max Gurtovoy @ 2021-08-23 8:35 ` Yongji Xie 2021-08-23 9:04 ` Max Gurtovoy 0 siblings, 1 reply; 48+ messages in thread From: Yongji Xie @ 2021-08-23 8:35 UTC (permalink / raw) To: Max Gurtovoy Cc: Michael S. Tsirkin, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > > > On 8/23/2021 7:31 AM, Yongji Xie wrote: > > On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >> > >> On 8/9/2021 1:16 PM, Xie Yongji wrote: > >>> 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. > >> This is not clear to me. What is untrusted device ? is it a buggy device ? > >> > > A buggy device, the devices in an encrypted VM, or a userspace device > > created by VDUSE [1]. > > > > [1] https://lore.kernel.org/kvm/20210818120642.165-1-xieyongji@bytedance.com/ > > if it's a userspace device, why don't you fix its control path code > instead of adding workarounds in the kernel driver ? > VDUSE kernel module would not touch (be aware of) the device specific configuration space. It should be more reasonable to fix it in the device driver. There is also some existing interface (.validate()) for doing that. And regardless of userspace device, we still need to fix it for other cases. Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 8:35 ` Yongji Xie @ 2021-08-23 9:04 ` Max Gurtovoy 2021-08-23 9:27 ` Yongji Xie 0 siblings, 1 reply; 48+ messages in thread From: Max Gurtovoy @ 2021-08-23 9:04 UTC (permalink / raw) To: Yongji Xie Cc: Michael S. Tsirkin, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On 8/23/2021 11:35 AM, Yongji Xie wrote: > On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >> >> On 8/23/2021 7:31 AM, Yongji Xie wrote: >>> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >>>> On 8/9/2021 1:16 PM, Xie Yongji wrote: >>>>> 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. >>>> This is not clear to me. What is untrusted device ? is it a buggy device ? >>>> >>> A buggy device, the devices in an encrypted VM, or a userspace device >>> created by VDUSE [1]. >>> >>> [1] https://lore.kernel.org/kvm/20210818120642.165-1-xieyongji@bytedance.com/ >> if it's a userspace device, why don't you fix its control path code >> instead of adding workarounds in the kernel driver ? >> > VDUSE kernel module would not touch (be aware of) the device specific > configuration space. It should be more reasonable to fix it in the > device driver. There is also some existing interface (.validate()) for > doing that. who is emulating the device configuration space ? > And regardless of userspace device, we still need to fix it for other cases. which cases ? Do you know that there is a buggy HW we need to workaround ? > Thanks, > Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 9:04 ` Max Gurtovoy @ 2021-08-23 9:27 ` Yongji Xie 2021-08-23 9:38 ` Max Gurtovoy 0 siblings, 1 reply; 48+ messages in thread From: Yongji Xie @ 2021-08-23 9:27 UTC (permalink / raw) To: Max Gurtovoy Cc: Michael S. Tsirkin, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Mon, Aug 23, 2021 at 5:04 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > > > On 8/23/2021 11:35 AM, Yongji Xie wrote: > > On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >> > >> On 8/23/2021 7:31 AM, Yongji Xie wrote: > >>> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >>>> On 8/9/2021 1:16 PM, Xie Yongji wrote: > >>>>> 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. > >>>> This is not clear to me. What is untrusted device ? is it a buggy device ? > >>>> > >>> A buggy device, the devices in an encrypted VM, or a userspace device > >>> created by VDUSE [1]. > >>> > >>> [1] https://lore.kernel.org/kvm/20210818120642.165-1-xieyongji@bytedance.com/ > >> if it's a userspace device, why don't you fix its control path code > >> instead of adding workarounds in the kernel driver ? > >> > > VDUSE kernel module would not touch (be aware of) the device specific > > configuration space. It should be more reasonable to fix it in the > > device driver. There is also some existing interface (.validate()) for > > doing that. > > who is emulating the device configuration space ? > A userspace daemon will initialize the device configuration space and pass the contents to the VDUSE kernel module. The VDUSE kernel module will handle the access of the config space from the virtio device driver, but it doesn't need to know the contents (although we can know that). > > And regardless of userspace device, we still need to fix it for other cases. > > which cases ? Do you know that there is a buggy HW we need to workaround ? > No, there isn't now. But this could be a potential attack surface if the host doesn't trust the device. Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 9:27 ` Yongji Xie @ 2021-08-23 9:38 ` Max Gurtovoy 2021-08-23 10:33 ` Yongji Xie 0 siblings, 1 reply; 48+ messages in thread From: Max Gurtovoy @ 2021-08-23 9:38 UTC (permalink / raw) To: Yongji Xie Cc: Michael S. Tsirkin, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On 8/23/2021 12:27 PM, Yongji Xie wrote: > On Mon, Aug 23, 2021 at 5:04 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >> >> On 8/23/2021 11:35 AM, Yongji Xie wrote: >>> On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >>>> On 8/23/2021 7:31 AM, Yongji Xie wrote: >>>>> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >>>>>> On 8/9/2021 1:16 PM, Xie Yongji wrote: >>>>>>> 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. >>>>>> This is not clear to me. What is untrusted device ? is it a buggy device ? >>>>>> >>>>> A buggy device, the devices in an encrypted VM, or a userspace device >>>>> created by VDUSE [1]. >>>>> >>>>> [1] https://lore.kernel.org/kvm/20210818120642.165-1-xieyongji@bytedance.com/ >>>> if it's a userspace device, why don't you fix its control path code >>>> instead of adding workarounds in the kernel driver ? >>>> >>> VDUSE kernel module would not touch (be aware of) the device specific >>> configuration space. It should be more reasonable to fix it in the >>> device driver. There is also some existing interface (.validate()) for >>> doing that. >> who is emulating the device configuration space ? >> > A userspace daemon will initialize the device configuration space and > pass the contents to the VDUSE kernel module. The VDUSE kernel module > will handle the access of the config space from the virtio device > driver, but it doesn't need to know the contents (although we can know > that). So you add a workaround in the guest kernel drivers instead of checking these quirks in the hypervisor ? VDUSE kernel should enforce the security for the devices it emulates/presents to the VM. > >>> And regardless of userspace device, we still need to fix it for other cases. >> which cases ? Do you know that there is a buggy HW we need to workaround ? >> > No, there isn't now. But this could be a potential attack surface if > the host doesn't trust the device. If the host doesn't trust a device, why it continues using it ? Do you suggest we do these workarounds in all device drivers in the kernel ? > > Thanks, > Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 9:38 ` Max Gurtovoy @ 2021-08-23 10:33 ` Yongji Xie 2021-08-23 10:45 ` Max Gurtovoy 0 siblings, 1 reply; 48+ messages in thread From: Yongji Xie @ 2021-08-23 10:33 UTC (permalink / raw) To: Max Gurtovoy Cc: Michael S. Tsirkin, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Mon, Aug 23, 2021 at 5:38 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > > > On 8/23/2021 12:27 PM, Yongji Xie wrote: > > On Mon, Aug 23, 2021 at 5:04 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >> > >> On 8/23/2021 11:35 AM, Yongji Xie wrote: > >>> On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >>>> On 8/23/2021 7:31 AM, Yongji Xie wrote: > >>>>> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >>>>>> On 8/9/2021 1:16 PM, Xie Yongji wrote: > >>>>>>> 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. > >>>>>> This is not clear to me. What is untrusted device ? is it a buggy device ? > >>>>>> > >>>>> A buggy device, the devices in an encrypted VM, or a userspace device > >>>>> created by VDUSE [1]. > >>>>> > >>>>> [1] https://lore.kernel.org/kvm/20210818120642.165-1-xieyongji@bytedance.com/ > >>>> if it's a userspace device, why don't you fix its control path code > >>>> instead of adding workarounds in the kernel driver ? > >>>> > >>> VDUSE kernel module would not touch (be aware of) the device specific > >>> configuration space. It should be more reasonable to fix it in the > >>> device driver. There is also some existing interface (.validate()) for > >>> doing that. > >> who is emulating the device configuration space ? > >> > > A userspace daemon will initialize the device configuration space and > > pass the contents to the VDUSE kernel module. The VDUSE kernel module > > will handle the access of the config space from the virtio device > > driver, but it doesn't need to know the contents (although we can know > > that). > > So you add a workaround in the guest kernel drivers instead of checking > these quirks in the hypervisor ? > I didn't see any problem adding this validation in the device driver. > VDUSE kernel should enforce the security for the devices it > emulates/presents to the VM. > I agree that the VDUSE kernel should enforce the security for the emulated devices. But I still think the virtio device driver should handle this case since nobody can make sure the device can always set the correct value. Adding this validation would be helpful. > >>> And regardless of userspace device, we still need to fix it for other cases. > >> which cases ? Do you know that there is a buggy HW we need to workaround ? > >> > > No, there isn't now. But this could be a potential attack surface if > > the host doesn't trust the device. > > If the host doesn't trust a device, why it continues using it ? > IIUC this is the case for the encrypted VMs. > Do you suggest we do these workarounds in all device drivers in the kernel ? > Isn't it the driver's job to validate some unreasonable configuration? Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 10:33 ` Yongji Xie @ 2021-08-23 10:45 ` Max Gurtovoy 2021-08-23 11:41 ` Yongji Xie 2021-08-23 12:13 ` Michael S. Tsirkin 0 siblings, 2 replies; 48+ messages in thread From: Max Gurtovoy @ 2021-08-23 10:45 UTC (permalink / raw) To: Yongji Xie Cc: Michael S. Tsirkin, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On 8/23/2021 1:33 PM, Yongji Xie wrote: > On Mon, Aug 23, 2021 at 5:38 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >> >> On 8/23/2021 12:27 PM, Yongji Xie wrote: >>> On Mon, Aug 23, 2021 at 5:04 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >>>> On 8/23/2021 11:35 AM, Yongji Xie wrote: >>>>> On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >>>>>> On 8/23/2021 7:31 AM, Yongji Xie wrote: >>>>>>> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >>>>>>>> On 8/9/2021 1:16 PM, Xie Yongji wrote: >>>>>>>>> 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. >>>>>>>> This is not clear to me. What is untrusted device ? is it a buggy device ? >>>>>>>> >>>>>>> A buggy device, the devices in an encrypted VM, or a userspace device >>>>>>> created by VDUSE [1]. >>>>>>> >>>>>>> [1] https://lore.kernel.org/kvm/20210818120642.165-1-xieyongji@bytedance.com/ >>>>>> if it's a userspace device, why don't you fix its control path code >>>>>> instead of adding workarounds in the kernel driver ? >>>>>> >>>>> VDUSE kernel module would not touch (be aware of) the device specific >>>>> configuration space. It should be more reasonable to fix it in the >>>>> device driver. There is also some existing interface (.validate()) for >>>>> doing that. >>>> who is emulating the device configuration space ? >>>> >>> A userspace daemon will initialize the device configuration space and >>> pass the contents to the VDUSE kernel module. The VDUSE kernel module >>> will handle the access of the config space from the virtio device >>> driver, but it doesn't need to know the contents (although we can know >>> that). >> So you add a workaround in the guest kernel drivers instead of checking >> these quirks in the hypervisor ? >> > I didn't see any problem adding this validation in the device driver. > >> VDUSE kernel should enforce the security for the devices it >> emulates/presents to the VM. >> > I agree that the VDUSE kernel should enforce the security for the > emulated devices. But I still think the virtio device driver should > handle this case since nobody can make sure the device can always set > the correct value. Adding this validation would be helpful. It helpful if there is a justification for this. In this case, no such HW device exist and the only device that can cause this trouble today is user space VDUSE device that must be validated by the emulation VDUSE kernel driver. Otherwise, will can create 1000 commit like this in the virtio level (for example for each feature for each virtio device). > >>>>> And regardless of userspace device, we still need to fix it for other cases. >>>> which cases ? Do you know that there is a buggy HW we need to workaround ? >>>> >>> No, there isn't now. But this could be a potential attack surface if >>> the host doesn't trust the device. >> If the host doesn't trust a device, why it continues using it ? >> > IIUC this is the case for the encrypted VMs. what do you mean encrypted VM ? And how this small patch causes a VM to be 100% encryption supported ? >> Do you suggest we do these workarounds in all device drivers in the kernel ? >> > Isn't it the driver's job to validate some unreasonable configuration? The check should be in different layer. Virtio blk driver should not cover on some strange VDUSE stuff. > > Thanks, > Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 10:45 ` Max Gurtovoy @ 2021-08-23 11:41 ` Yongji Xie 2021-08-23 12:13 ` Michael S. Tsirkin 1 sibling, 0 replies; 48+ messages in thread From: Yongji Xie @ 2021-08-23 11:41 UTC (permalink / raw) To: Max Gurtovoy Cc: Michael S. Tsirkin, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Mon, Aug 23, 2021 at 6:45 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > > > On 8/23/2021 1:33 PM, Yongji Xie wrote: > > On Mon, Aug 23, 2021 at 5:38 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >> > >> On 8/23/2021 12:27 PM, Yongji Xie wrote: > >>> On Mon, Aug 23, 2021 at 5:04 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >>>> On 8/23/2021 11:35 AM, Yongji Xie wrote: > >>>>> On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >>>>>> On 8/23/2021 7:31 AM, Yongji Xie wrote: > >>>>>>> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >>>>>>>> On 8/9/2021 1:16 PM, Xie Yongji wrote: > >>>>>>>>> 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. > >>>>>>>> This is not clear to me. What is untrusted device ? is it a buggy device ? > >>>>>>>> > >>>>>>> A buggy device, the devices in an encrypted VM, or a userspace device > >>>>>>> created by VDUSE [1]. > >>>>>>> > >>>>>>> [1] https://lore.kernel.org/kvm/20210818120642.165-1-xieyongji@bytedance.com/ > >>>>>> if it's a userspace device, why don't you fix its control path code > >>>>>> instead of adding workarounds in the kernel driver ? > >>>>>> > >>>>> VDUSE kernel module would not touch (be aware of) the device specific > >>>>> configuration space. It should be more reasonable to fix it in the > >>>>> device driver. There is also some existing interface (.validate()) for > >>>>> doing that. > >>>> who is emulating the device configuration space ? > >>>> > >>> A userspace daemon will initialize the device configuration space and > >>> pass the contents to the VDUSE kernel module. The VDUSE kernel module > >>> will handle the access of the config space from the virtio device > >>> driver, but it doesn't need to know the contents (although we can know > >>> that). > >> So you add a workaround in the guest kernel drivers instead of checking > >> these quirks in the hypervisor ? > >> > > I didn't see any problem adding this validation in the device driver. > > > >> VDUSE kernel should enforce the security for the devices it > >> emulates/presents to the VM. > >> > > I agree that the VDUSE kernel should enforce the security for the > > emulated devices. But I still think the virtio device driver should > > handle this case since nobody can make sure the device can always set > > the correct value. Adding this validation would be helpful. > > It helpful if there is a justification for this. > > In this case, no such HW device exist and the only device that can cause > this trouble today is user space VDUSE device that must be validated by > the emulation VDUSE kernel driver. > > Otherwise, will can create 1000 commit like this in the virtio level > (for example for each feature for each virtio device). > Maybe not. Most of the configuration fields have already been validated at the virtio level. > > > >>>>> And regardless of userspace device, we still need to fix it for other cases. > >>>> which cases ? Do you know that there is a buggy HW we need to workaround ? > >>>> > >>> No, there isn't now. But this could be a potential attack surface if > >>> the host doesn't trust the device. > >> If the host doesn't trust a device, why it continues using it ? > >> > > IIUC this is the case for the encrypted VMs. > > what do you mean encrypted VM ? > The guest memory is encrypted, the host can not read or write any guest memory not explicitly shared with it. In this case, the guest doesn't trust the host. There are already some efforts [1] [2] to protect this kind of guest from untrusted devices. [1] https://lwn.net/ml/linux-kernel/20210421032117.5177-1-jasowang@redhat.com/ [2] https://www.spinics.net/lists/linux-virtualization/msg50182.html > And how this small patch causes a VM to be 100% encryption supported ? > I'm not sure if there will be some ways to leak data in an encrypted VM with the invalid block size. Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 10:45 ` Max Gurtovoy @ 2021-08-23 12:13 ` Michael S. Tsirkin 2021-08-23 12:13 ` Michael S. Tsirkin 1 sibling, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-08-23 12:13 UTC (permalink / raw) To: Max Gurtovoy Cc: Yongji Xie, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: > It helpful if there is a justification for this. > > In this case, no such HW device exist and the only device that can cause > this trouble today is user space VDUSE device that must be validated by the > emulation VDUSE kernel driver. > > Otherwise, will can create 1000 commit like this in the virtio level (for > example for each feature for each virtio device). Yea, it's a lot of work but I don't think it's avoidable. > > > > > > > > And regardless of userspace device, we still need to fix it for other cases. > > > > > which cases ? Do you know that there is a buggy HW we need to workaround ? > > > > > > > > > No, there isn't now. But this could be a potential attack surface if > > > > the host doesn't trust the device. > > > If the host doesn't trust a device, why it continues using it ? > > > > > IIUC this is the case for the encrypted VMs. > > what do you mean encrypted VM ? > > And how this small patch causes a VM to be 100% encryption supported ? > > > > Do you suggest we do these workarounds in all device drivers in the kernel ? > > > > > Isn't it the driver's job to validate some unreasonable configuration? > > The check should be in different layer. > > Virtio blk driver should not cover on some strange VDUSE stuff. Yes I'm not convinced VDUSE is a valid use-case. I think that for security and robustness it should validate data it gets from userspace right there after reading it. But I think this is useful for the virtio hardening thing. https://lwn.net/Articles/865216/ Yongji - I think the commit log should be much more explicit that this is hardening. Otherwise people get confused and think this needs a CVE or a backport for security. -- MST ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-08-23 12:13 ` Michael S. Tsirkin 0 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-08-23 12:13 UTC (permalink / raw) To: Max Gurtovoy Cc: linux-kernel, virtualization, linux-block, Yongji Xie, Stefan Hajnoczi On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: > It helpful if there is a justification for this. > > In this case, no such HW device exist and the only device that can cause > this trouble today is user space VDUSE device that must be validated by the > emulation VDUSE kernel driver. > > Otherwise, will can create 1000 commit like this in the virtio level (for > example for each feature for each virtio device). Yea, it's a lot of work but I don't think it's avoidable. > > > > > > > > And regardless of userspace device, we still need to fix it for other cases. > > > > > which cases ? Do you know that there is a buggy HW we need to workaround ? > > > > > > > > > No, there isn't now. But this could be a potential attack surface if > > > > the host doesn't trust the device. > > > If the host doesn't trust a device, why it continues using it ? > > > > > IIUC this is the case for the encrypted VMs. > > what do you mean encrypted VM ? > > And how this small patch causes a VM to be 100% encryption supported ? > > > > Do you suggest we do these workarounds in all device drivers in the kernel ? > > > > > Isn't it the driver's job to validate some unreasonable configuration? > > The check should be in different layer. > > Virtio blk driver should not cover on some strange VDUSE stuff. Yes I'm not convinced VDUSE is a valid use-case. I think that for security and robustness it should validate data it gets from userspace right there after reading it. But I think this is useful for the virtio hardening thing. https://lwn.net/Articles/865216/ Yongji - I think the commit log should be much more explicit that this is hardening. Otherwise people get confused and think this needs a CVE or a backport for security. -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 12:13 ` Michael S. Tsirkin (?) @ 2021-08-23 12:40 ` Yongji Xie 2021-08-23 16:02 ` Michael S. Tsirkin -1 siblings, 1 reply; 48+ messages in thread From: Yongji Xie @ 2021-08-23 12:40 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Max Gurtovoy, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Mon, Aug 23, 2021 at 8:13 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: > > It helpful if there is a justification for this. > > > > In this case, no such HW device exist and the only device that can cause > > this trouble today is user space VDUSE device that must be validated by the > > emulation VDUSE kernel driver. > > > > Otherwise, will can create 1000 commit like this in the virtio level (for > > example for each feature for each virtio device). > > Yea, it's a lot of work but I don't think it's avoidable. > > > > > > > > > > > And regardless of userspace device, we still need to fix it for other cases. > > > > > > which cases ? Do you know that there is a buggy HW we need to workaround ? > > > > > > > > > > > No, there isn't now. But this could be a potential attack surface if > > > > > the host doesn't trust the device. > > > > If the host doesn't trust a device, why it continues using it ? > > > > > > > IIUC this is the case for the encrypted VMs. > > > > what do you mean encrypted VM ? > > > > And how this small patch causes a VM to be 100% encryption supported ? > > > > > > Do you suggest we do these workarounds in all device drivers in the kernel ? > > > > > > > Isn't it the driver's job to validate some unreasonable configuration? > > > > The check should be in different layer. > > > > Virtio blk driver should not cover on some strange VDUSE stuff. > > Yes I'm not convinced VDUSE is a valid use-case. I think that for > security and robustness it should validate data it gets from userspace > right there after reading it. > But I think this is useful for the virtio hardening thing. > https://lwn.net/Articles/865216/ > > Yongji - I think the commit log should be much more explicit that > this is hardening. Otherwise people get confused and think this > needs a CVE or a backport for security. > OK, do I need to send a v6? This patch seems to be already merged into Linus's tree. Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 12:40 ` Yongji Xie @ 2021-08-23 16:02 ` Michael S. Tsirkin 0 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-08-23 16:02 UTC (permalink / raw) To: Yongji Xie Cc: Max Gurtovoy, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Mon, Aug 23, 2021 at 08:40:30PM +0800, Yongji Xie wrote: > On Mon, Aug 23, 2021 at 8:13 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: > > > It helpful if there is a justification for this. > > > > > > In this case, no such HW device exist and the only device that can cause > > > this trouble today is user space VDUSE device that must be validated by the > > > emulation VDUSE kernel driver. > > > > > > Otherwise, will can create 1000 commit like this in the virtio level (for > > > example for each feature for each virtio device). > > > > Yea, it's a lot of work but I don't think it's avoidable. > > > > > > > > > > > > > > And regardless of userspace device, we still need to fix it for other cases. > > > > > > > which cases ? Do you know that there is a buggy HW we need to workaround ? > > > > > > > > > > > > > No, there isn't now. But this could be a potential attack surface if > > > > > > the host doesn't trust the device. > > > > > If the host doesn't trust a device, why it continues using it ? > > > > > > > > > IIUC this is the case for the encrypted VMs. > > > > > > what do you mean encrypted VM ? > > > > > > And how this small patch causes a VM to be 100% encryption supported ? > > > > > > > > Do you suggest we do these workarounds in all device drivers in the kernel ? > > > > > > > > > Isn't it the driver's job to validate some unreasonable configuration? > > > > > > The check should be in different layer. > > > > > > Virtio blk driver should not cover on some strange VDUSE stuff. > > > > Yes I'm not convinced VDUSE is a valid use-case. I think that for > > security and robustness it should validate data it gets from userspace > > right there after reading it. > > But I think this is useful for the virtio hardening thing. > > https://lwn.net/Articles/865216/ > > > > Yongji - I think the commit log should be much more explicit that > > this is hardening. Otherwise people get confused and think this > > needs a CVE or a backport for security. > > > > OK, do I need to send a v6? This patch seems to be already merged into > Linus's tree. > > Thanks, > Yongji No, it's a comment for the future - I assume you will keep adding this kind of validation in other places. -- MST ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-08-23 16:02 ` Michael S. Tsirkin 0 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-08-23 16:02 UTC (permalink / raw) To: Yongji Xie Cc: Max Gurtovoy, linux-kernel, virtualization, linux-block, Stefan Hajnoczi On Mon, Aug 23, 2021 at 08:40:30PM +0800, Yongji Xie wrote: > On Mon, Aug 23, 2021 at 8:13 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: > > > It helpful if there is a justification for this. > > > > > > In this case, no such HW device exist and the only device that can cause > > > this trouble today is user space VDUSE device that must be validated by the > > > emulation VDUSE kernel driver. > > > > > > Otherwise, will can create 1000 commit like this in the virtio level (for > > > example for each feature for each virtio device). > > > > Yea, it's a lot of work but I don't think it's avoidable. > > > > > > > > > > > > > > And regardless of userspace device, we still need to fix it for other cases. > > > > > > > which cases ? Do you know that there is a buggy HW we need to workaround ? > > > > > > > > > > > > > No, there isn't now. But this could be a potential attack surface if > > > > > > the host doesn't trust the device. > > > > > If the host doesn't trust a device, why it continues using it ? > > > > > > > > > IIUC this is the case for the encrypted VMs. > > > > > > what do you mean encrypted VM ? > > > > > > And how this small patch causes a VM to be 100% encryption supported ? > > > > > > > > Do you suggest we do these workarounds in all device drivers in the kernel ? > > > > > > > > > Isn't it the driver's job to validate some unreasonable configuration? > > > > > > The check should be in different layer. > > > > > > Virtio blk driver should not cover on some strange VDUSE stuff. > > > > Yes I'm not convinced VDUSE is a valid use-case. I think that for > > security and robustness it should validate data it gets from userspace > > right there after reading it. > > But I think this is useful for the virtio hardening thing. > > https://lwn.net/Articles/865216/ > > > > Yongji - I think the commit log should be much more explicit that > > this is hardening. Otherwise people get confused and think this > > needs a CVE or a backport for security. > > > > OK, do I need to send a v6? This patch seems to be already merged into > Linus's tree. > > Thanks, > Yongji No, it's a comment for the future - I assume you will keep adding this kind of validation in other places. -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 12:13 ` Michael S. Tsirkin (?) (?) @ 2021-08-23 22:31 ` Max Gurtovoy 2021-08-24 2:47 ` Jason Wang -1 siblings, 1 reply; 48+ messages in thread From: Max Gurtovoy @ 2021-08-23 22:31 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Yongji Xie, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On 8/23/2021 3:13 PM, Michael S. Tsirkin wrote: > On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: >> It helpful if there is a justification for this. >> >> In this case, no such HW device exist and the only device that can cause >> this trouble today is user space VDUSE device that must be validated by the >> emulation VDUSE kernel driver. >> >> Otherwise, will can create 1000 commit like this in the virtio level (for >> example for each feature for each virtio device). > Yea, it's a lot of work but I don't think it's avoidable. > >>>>>>> And regardless of userspace device, we still need to fix it for other cases. >>>>>> which cases ? Do you know that there is a buggy HW we need to workaround ? >>>>>> >>>>> No, there isn't now. But this could be a potential attack surface if >>>>> the host doesn't trust the device. >>>> If the host doesn't trust a device, why it continues using it ? >>>> >>> IIUC this is the case for the encrypted VMs. >> what do you mean encrypted VM ? >> >> And how this small patch causes a VM to be 100% encryption supported ? >> >>>> Do you suggest we do these workarounds in all device drivers in the kernel ? >>>> >>> Isn't it the driver's job to validate some unreasonable configuration? >> The check should be in different layer. >> >> Virtio blk driver should not cover on some strange VDUSE stuff. > Yes I'm not convinced VDUSE is a valid use-case. I think that for > security and robustness it should validate data it gets from userspace > right there after reading it. > But I think this is useful for the virtio hardening thing. > https://lwn.net/Articles/865216/ I don't see how this change is assisting confidential computing. Confidential computingtalks about encrypting guest memory from the host, and not adding some quirks to devices. > > Yongji - I think the commit log should be much more explicit that > this is hardening. Otherwise people get confused and think this > needs a CVE or a backport for security. > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-23 22:31 ` Max Gurtovoy @ 2021-08-24 2:47 ` Jason Wang 0 siblings, 0 replies; 48+ messages in thread From: Jason Wang @ 2021-08-24 2:47 UTC (permalink / raw) To: Max Gurtovoy Cc: Michael S. Tsirkin, Yongji Xie, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > > > On 8/23/2021 3:13 PM, Michael S. Tsirkin wrote: > > On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: > >> It helpful if there is a justification for this. > >> > >> In this case, no such HW device exist and the only device that can cause > >> this trouble today is user space VDUSE device that must be validated by the > >> emulation VDUSE kernel driver. > >> > >> Otherwise, will can create 1000 commit like this in the virtio level (for > >> example for each feature for each virtio device). > > Yea, it's a lot of work but I don't think it's avoidable. > > > >>>>>>> And regardless of userspace device, we still need to fix it for other cases. > >>>>>> which cases ? Do you know that there is a buggy HW we need to workaround ? > >>>>>> > >>>>> No, there isn't now. But this could be a potential attack surface if > >>>>> the host doesn't trust the device. > >>>> If the host doesn't trust a device, why it continues using it ? > >>>> > >>> IIUC this is the case for the encrypted VMs. > >> what do you mean encrypted VM ? > >> > >> And how this small patch causes a VM to be 100% encryption supported ? > >> > >>>> Do you suggest we do these workarounds in all device drivers in the kernel ? > >>>> > >>> Isn't it the driver's job to validate some unreasonable configuration? > >> The check should be in different layer. > >> > >> Virtio blk driver should not cover on some strange VDUSE stuff. > > Yes I'm not convinced VDUSE is a valid use-case. I think that for > > security and robustness it should validate data it gets from userspace > > right there after reading it. > > But I think this is useful for the virtio hardening thing. > > https://lwn.net/Articles/865216/ > > I don't see how this change is assisting confidential computing. > > Confidential computingtalks about encrypting guest memory from the host, > and not adding some quirks to devices. In the case of confidential computing, the hypervisor and hard device is not in the trust zone. It means the guest doesn't trust the cloud vendor. That's why we need to validate any input from them. Thanks > > > > > Yongji - I think the commit log should be much more explicit that > > this is hardening. Otherwise people get confused and think this > > needs a CVE or a backport for security. > > > ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-08-24 2:47 ` Jason Wang 0 siblings, 0 replies; 48+ messages in thread From: Jason Wang @ 2021-08-24 2:47 UTC (permalink / raw) To: Max Gurtovoy Cc: Michael S. Tsirkin, linux-kernel, virtualization, linux-block, Yongji Xie, Stefan Hajnoczi On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > > > On 8/23/2021 3:13 PM, Michael S. Tsirkin wrote: > > On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: > >> It helpful if there is a justification for this. > >> > >> In this case, no such HW device exist and the only device that can cause > >> this trouble today is user space VDUSE device that must be validated by the > >> emulation VDUSE kernel driver. > >> > >> Otherwise, will can create 1000 commit like this in the virtio level (for > >> example for each feature for each virtio device). > > Yea, it's a lot of work but I don't think it's avoidable. > > > >>>>>>> And regardless of userspace device, we still need to fix it for other cases. > >>>>>> which cases ? Do you know that there is a buggy HW we need to workaround ? > >>>>>> > >>>>> No, there isn't now. But this could be a potential attack surface if > >>>>> the host doesn't trust the device. > >>>> If the host doesn't trust a device, why it continues using it ? > >>>> > >>> IIUC this is the case for the encrypted VMs. > >> what do you mean encrypted VM ? > >> > >> And how this small patch causes a VM to be 100% encryption supported ? > >> > >>>> Do you suggest we do these workarounds in all device drivers in the kernel ? > >>>> > >>> Isn't it the driver's job to validate some unreasonable configuration? > >> The check should be in different layer. > >> > >> Virtio blk driver should not cover on some strange VDUSE stuff. > > Yes I'm not convinced VDUSE is a valid use-case. I think that for > > security and robustness it should validate data it gets from userspace > > right there after reading it. > > But I think this is useful for the virtio hardening thing. > > https://lwn.net/Articles/865216/ > > I don't see how this change is assisting confidential computing. > > Confidential computingtalks about encrypting guest memory from the host, > and not adding some quirks to devices. In the case of confidential computing, the hypervisor and hard device is not in the trust zone. It means the guest doesn't trust the cloud vendor. That's why we need to validate any input from them. Thanks > > > > > Yongji - I think the commit log should be much more explicit that > > this is hardening. Otherwise people get confused and think this > > needs a CVE or a backport for security. > > > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-24 2:47 ` Jason Wang (?) @ 2021-08-24 10:11 ` Max Gurtovoy 2021-08-24 12:52 ` Yongji Xie -1 siblings, 1 reply; 48+ messages in thread From: Max Gurtovoy @ 2021-08-24 10:11 UTC (permalink / raw) To: Jason Wang Cc: Michael S. Tsirkin, Yongji Xie, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On 8/24/2021 5:47 AM, Jason Wang wrote: > On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >> >> On 8/23/2021 3:13 PM, Michael S. Tsirkin wrote: >>> On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: >>>> It helpful if there is a justification for this. >>>> >>>> In this case, no such HW device exist and the only device that can cause >>>> this trouble today is user space VDUSE device that must be validated by the >>>> emulation VDUSE kernel driver. >>>> >>>> Otherwise, will can create 1000 commit like this in the virtio level (for >>>> example for each feature for each virtio device). >>> Yea, it's a lot of work but I don't think it's avoidable. >>> >>>>>>>>> And regardless of userspace device, we still need to fix it for other cases. >>>>>>>> which cases ? Do you know that there is a buggy HW we need to workaround ? >>>>>>>> >>>>>>> No, there isn't now. But this could be a potential attack surface if >>>>>>> the host doesn't trust the device. >>>>>> If the host doesn't trust a device, why it continues using it ? >>>>>> >>>>> IIUC this is the case for the encrypted VMs. >>>> what do you mean encrypted VM ? >>>> >>>> And how this small patch causes a VM to be 100% encryption supported ? >>>> >>>>>> Do you suggest we do these workarounds in all device drivers in the kernel ? >>>>>> >>>>> Isn't it the driver's job to validate some unreasonable configuration? >>>> The check should be in different layer. >>>> >>>> Virtio blk driver should not cover on some strange VDUSE stuff. >>> Yes I'm not convinced VDUSE is a valid use-case. I think that for >>> security and robustness it should validate data it gets from userspace >>> right there after reading it. >>> But I think this is useful for the virtio hardening thing. >>> https://lwn.net/Articles/865216/ >> I don't see how this change is assisting confidential computing. >> >> Confidential computingtalks about encrypting guest memory from the host, >> and not adding some quirks to devices. > In the case of confidential computing, the hypervisor and hard device > is not in the trust zone. It means the guest doesn't trust the cloud > vendor. Confidential computing protects data during processing ("in-use" data). Nothing to do with virtio feature negotiation. > > That's why we need to validate any input from them. > > Thanks > >>> Yongji - I think the commit log should be much more explicit that >>> this is hardening. Otherwise people get confused and think this >>> needs a CVE or a backport for security. >>> ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-24 10:11 ` Max Gurtovoy @ 2021-08-24 12:52 ` Yongji Xie 2021-08-24 13:30 ` Max Gurtovoy 0 siblings, 1 reply; 48+ messages in thread From: Yongji Xie @ 2021-08-24 12:52 UTC (permalink / raw) To: Max Gurtovoy Cc: Jason Wang, Michael S. Tsirkin, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Tue, Aug 24, 2021 at 6:11 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > > > On 8/24/2021 5:47 AM, Jason Wang wrote: > > On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >> > >> On 8/23/2021 3:13 PM, Michael S. Tsirkin wrote: > >>> On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: > >>>> It helpful if there is a justification for this. > >>>> > >>>> In this case, no such HW device exist and the only device that can cause > >>>> this trouble today is user space VDUSE device that must be validated by the > >>>> emulation VDUSE kernel driver. > >>>> > >>>> Otherwise, will can create 1000 commit like this in the virtio level (for > >>>> example for each feature for each virtio device). > >>> Yea, it's a lot of work but I don't think it's avoidable. > >>> > >>>>>>>>> And regardless of userspace device, we still need to fix it for other cases. > >>>>>>>> which cases ? Do you know that there is a buggy HW we need to workaround ? > >>>>>>>> > >>>>>>> No, there isn't now. But this could be a potential attack surface if > >>>>>>> the host doesn't trust the device. > >>>>>> If the host doesn't trust a device, why it continues using it ? > >>>>>> > >>>>> IIUC this is the case for the encrypted VMs. > >>>> what do you mean encrypted VM ? > >>>> > >>>> And how this small patch causes a VM to be 100% encryption supported ? > >>>> > >>>>>> Do you suggest we do these workarounds in all device drivers in the kernel ? > >>>>>> > >>>>> Isn't it the driver's job to validate some unreasonable configuration? > >>>> The check should be in different layer. > >>>> > >>>> Virtio blk driver should not cover on some strange VDUSE stuff. > >>> Yes I'm not convinced VDUSE is a valid use-case. I think that for > >>> security and robustness it should validate data it gets from userspace > >>> right there after reading it. > >>> But I think this is useful for the virtio hardening thing. > >>> https://lwn.net/Articles/865216/ > >> I don't see how this change is assisting confidential computing. > >> > >> Confidential computingtalks about encrypting guest memory from the host, > >> and not adding some quirks to devices. > > In the case of confidential computing, the hypervisor and hard device > > is not in the trust zone. It means the guest doesn't trust the cloud > > vendor. > > Confidential computing protects data during processing ("in-use" data). > > Nothing to do with virtio feature negotiation. > But if a misbehaving device can corrupt the guest memory, I think it should be avoided. Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-24 12:52 ` Yongji Xie @ 2021-08-24 13:30 ` Max Gurtovoy 2021-08-24 13:38 ` Yongji Xie 0 siblings, 1 reply; 48+ messages in thread From: Max Gurtovoy @ 2021-08-24 13:30 UTC (permalink / raw) To: Yongji Xie Cc: Jason Wang, Michael S. Tsirkin, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On 8/24/2021 3:52 PM, Yongji Xie wrote: > On Tue, Aug 24, 2021 at 6:11 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >> >> On 8/24/2021 5:47 AM, Jason Wang wrote: >>> On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >>>> On 8/23/2021 3:13 PM, Michael S. Tsirkin wrote: >>>>> On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: >>>>>> It helpful if there is a justification for this. >>>>>> >>>>>> In this case, no such HW device exist and the only device that can cause >>>>>> this trouble today is user space VDUSE device that must be validated by the >>>>>> emulation VDUSE kernel driver. >>>>>> >>>>>> Otherwise, will can create 1000 commit like this in the virtio level (for >>>>>> example for each feature for each virtio device). >>>>> Yea, it's a lot of work but I don't think it's avoidable. >>>>> >>>>>>>>>>> And regardless of userspace device, we still need to fix it for other cases. >>>>>>>>>> which cases ? Do you know that there is a buggy HW we need to workaround ? >>>>>>>>>> >>>>>>>>> No, there isn't now. But this could be a potential attack surface if >>>>>>>>> the host doesn't trust the device. >>>>>>>> If the host doesn't trust a device, why it continues using it ? >>>>>>>> >>>>>>> IIUC this is the case for the encrypted VMs. >>>>>> what do you mean encrypted VM ? >>>>>> >>>>>> And how this small patch causes a VM to be 100% encryption supported ? >>>>>> >>>>>>>> Do you suggest we do these workarounds in all device drivers in the kernel ? >>>>>>>> >>>>>>> Isn't it the driver's job to validate some unreasonable configuration? >>>>>> The check should be in different layer. >>>>>> >>>>>> Virtio blk driver should not cover on some strange VDUSE stuff. >>>>> Yes I'm not convinced VDUSE is a valid use-case. I think that for >>>>> security and robustness it should validate data it gets from userspace >>>>> right there after reading it. >>>>> But I think this is useful for the virtio hardening thing. >>>>> https://lwn.net/Articles/865216/ >>>> I don't see how this change is assisting confidential computing. >>>> >>>> Confidential computingtalks about encrypting guest memory from the host, >>>> and not adding some quirks to devices. >>> In the case of confidential computing, the hypervisor and hard device >>> is not in the trust zone. It means the guest doesn't trust the cloud >>> vendor. >> Confidential computing protects data during processing ("in-use" data). >> >> Nothing to do with virtio feature negotiation. >> > But if a misbehaving device can corrupt the guest memory, I think it > should be avoided. So don't say it's related to confidential computing, and fix it in the VDUSE kernel driver in the hypervisor. If this is existing device and we want to add a quirk to it, so be it. But it's not the case. > Thanks, > Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-24 13:30 ` Max Gurtovoy @ 2021-08-24 13:38 ` Yongji Xie 2021-08-24 13:48 ` Max Gurtovoy 0 siblings, 1 reply; 48+ messages in thread From: Yongji Xie @ 2021-08-24 13:38 UTC (permalink / raw) To: Max Gurtovoy Cc: Jason Wang, Michael S. Tsirkin, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Tue, Aug 24, 2021 at 9:30 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > > > On 8/24/2021 3:52 PM, Yongji Xie wrote: > > On Tue, Aug 24, 2021 at 6:11 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >> > >> On 8/24/2021 5:47 AM, Jason Wang wrote: > >>> On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >>>> On 8/23/2021 3:13 PM, Michael S. Tsirkin wrote: > >>>>> On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: > >>>>>> It helpful if there is a justification for this. > >>>>>> > >>>>>> In this case, no such HW device exist and the only device that can cause > >>>>>> this trouble today is user space VDUSE device that must be validated by the > >>>>>> emulation VDUSE kernel driver. > >>>>>> > >>>>>> Otherwise, will can create 1000 commit like this in the virtio level (for > >>>>>> example for each feature for each virtio device). > >>>>> Yea, it's a lot of work but I don't think it's avoidable. > >>>>> > >>>>>>>>>>> And regardless of userspace device, we still need to fix it for other cases. > >>>>>>>>>> which cases ? Do you know that there is a buggy HW we need to workaround ? > >>>>>>>>>> > >>>>>>>>> No, there isn't now. But this could be a potential attack surface if > >>>>>>>>> the host doesn't trust the device. > >>>>>>>> If the host doesn't trust a device, why it continues using it ? > >>>>>>>> > >>>>>>> IIUC this is the case for the encrypted VMs. > >>>>>> what do you mean encrypted VM ? > >>>>>> > >>>>>> And how this small patch causes a VM to be 100% encryption supported ? > >>>>>> > >>>>>>>> Do you suggest we do these workarounds in all device drivers in the kernel ? > >>>>>>>> > >>>>>>> Isn't it the driver's job to validate some unreasonable configuration? > >>>>>> The check should be in different layer. > >>>>>> > >>>>>> Virtio blk driver should not cover on some strange VDUSE stuff. > >>>>> Yes I'm not convinced VDUSE is a valid use-case. I think that for > >>>>> security and robustness it should validate data it gets from userspace > >>>>> right there after reading it. > >>>>> But I think this is useful for the virtio hardening thing. > >>>>> https://lwn.net/Articles/865216/ > >>>> I don't see how this change is assisting confidential computing. > >>>> > >>>> Confidential computingtalks about encrypting guest memory from the host, > >>>> and not adding some quirks to devices. > >>> In the case of confidential computing, the hypervisor and hard device > >>> is not in the trust zone. It means the guest doesn't trust the cloud > >>> vendor. > >> Confidential computing protects data during processing ("in-use" data). > >> > >> Nothing to do with virtio feature negotiation. > >> > > But if a misbehaving device can corrupt the guest memory, I think it > > should be avoided. > > So don't say it's related to confidential computing, and fix it in the > VDUSE kernel driver in the hypervisor. > What I mean is in confidential computing cases. An untrusted device might corrupt the protected guest memory, it should be avoided. Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-24 13:38 ` Yongji Xie @ 2021-08-24 13:48 ` Max Gurtovoy 0 siblings, 0 replies; 48+ messages in thread From: Max Gurtovoy @ 2021-08-24 13:48 UTC (permalink / raw) To: Yongji Xie Cc: Jason Wang, Michael S. Tsirkin, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On 8/24/2021 4:38 PM, Yongji Xie wrote: > On Tue, Aug 24, 2021 at 9:30 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >> >> On 8/24/2021 3:52 PM, Yongji Xie wrote: >>> On Tue, Aug 24, 2021 at 6:11 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >>>> On 8/24/2021 5:47 AM, Jason Wang wrote: >>>>> On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >>>>>> On 8/23/2021 3:13 PM, Michael S. Tsirkin wrote: >>>>>>> On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote: >>>>>>>> It helpful if there is a justification for this. >>>>>>>> >>>>>>>> In this case, no such HW device exist and the only device that can cause >>>>>>>> this trouble today is user space VDUSE device that must be validated by the >>>>>>>> emulation VDUSE kernel driver. >>>>>>>> >>>>>>>> Otherwise, will can create 1000 commit like this in the virtio level (for >>>>>>>> example for each feature for each virtio device). >>>>>>> Yea, it's a lot of work but I don't think it's avoidable. >>>>>>> >>>>>>>>>>>>> And regardless of userspace device, we still need to fix it for other cases. >>>>>>>>>>>> which cases ? Do you know that there is a buggy HW we need to workaround ? >>>>>>>>>>>> >>>>>>>>>>> No, there isn't now. But this could be a potential attack surface if >>>>>>>>>>> the host doesn't trust the device. >>>>>>>>>> If the host doesn't trust a device, why it continues using it ? >>>>>>>>>> >>>>>>>>> IIUC this is the case for the encrypted VMs. >>>>>>>> what do you mean encrypted VM ? >>>>>>>> >>>>>>>> And how this small patch causes a VM to be 100% encryption supported ? >>>>>>>> >>>>>>>>>> Do you suggest we do these workarounds in all device drivers in the kernel ? >>>>>>>>>> >>>>>>>>> Isn't it the driver's job to validate some unreasonable configuration? >>>>>>>> The check should be in different layer. >>>>>>>> >>>>>>>> Virtio blk driver should not cover on some strange VDUSE stuff. >>>>>>> Yes I'm not convinced VDUSE is a valid use-case. I think that for >>>>>>> security and robustness it should validate data it gets from userspace >>>>>>> right there after reading it. >>>>>>> But I think this is useful for the virtio hardening thing. >>>>>>> https://lwn.net/Articles/865216/ >>>>>> I don't see how this change is assisting confidential computing. >>>>>> >>>>>> Confidential computingtalks about encrypting guest memory from the host, >>>>>> and not adding some quirks to devices. >>>>> In the case of confidential computing, the hypervisor and hard device >>>>> is not in the trust zone. It means the guest doesn't trust the cloud >>>>> vendor. >>>> Confidential computing protects data during processing ("in-use" data). >>>> >>>> Nothing to do with virtio feature negotiation. >>>> >>> But if a misbehaving device can corrupt the guest memory, I think it >>> should be avoided. >> So don't say it's related to confidential computing, and fix it in the >> VDUSE kernel driver in the hypervisor. >> > What I mean is in confidential computing cases. An untrusted device > might corrupt the protected guest memory, it should be avoided. This patch has nothing to do with confidential computing by definition (virtio feature negotiation are not "in-use" data). It's device configuration space. MST, I prefer adding quirks for vDPA devices in VDUSE driver and not adding workarounds to virtio driver. I guess this patch can stay but future patches like this shouldn't be merged without a very good reason. > > Thanks, > Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-08-09 10:16 [PATCH v5] virtio-blk: Add validation for block size in config space Xie Yongji @ 2021-10-04 15:27 ` Michael S. Tsirkin 2021-08-22 23:17 ` Max Gurtovoy 2021-10-04 15:27 ` Michael S. Tsirkin 2 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-10-04 15:27 UTC (permalink / raw) To: Xie Yongji; +Cc: jasowang, stefanha, virtualization, linux-block, linux-kernel On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote: > 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@bytedance.com> So I had to revert this due basically bugs in QEMU. My suggestion at this point is to try and update blk_queue_logical_block_size to BUG_ON when the size is out of a reasonable range. This has the advantage of fixing more hardware, not just virtio. > --- > 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); > + > + 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, > -- > 2.11.0 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-10-04 15:27 ` Michael S. Tsirkin 0 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-10-04 15:27 UTC (permalink / raw) To: Xie Yongji; +Cc: linux-block, linux-kernel, stefanha, virtualization On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote: > 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@bytedance.com> So I had to revert this due basically bugs in QEMU. My suggestion at this point is to try and update blk_queue_logical_block_size to BUG_ON when the size is out of a reasonable range. This has the advantage of fixing more hardware, not just virtio. > --- > 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); > + > + 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, > -- > 2.11.0 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-10-04 15:27 ` Michael S. Tsirkin @ 2021-10-04 15:39 ` Michael S. Tsirkin -1 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-10-04 15:39 UTC (permalink / raw) To: Xie Yongji; +Cc: jasowang, stefanha, virtualization, linux-block, linux-kernel On Mon, Oct 04, 2021 at 11:27:29AM -0400, Michael S. Tsirkin wrote: > On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote: > > 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@bytedance.com> > > So I had to revert this due basically bugs in QEMU. > > My suggestion at this point is to try and update > blk_queue_logical_block_size to BUG_ON when the size > is out of a reasonable range. > > This has the advantage of fixing more hardware, not just virtio. > > > > > --- > > 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); > > + > > + return 0; > > +} > > + > > static int virtblk_probe(struct virtio_device *vdev) > > { > > struct virtio_blk *vblk; I started wondering about this. So let's assume is PAGE_SIZE < blk_size (after all it's up to guest at many platforms). Will using the device even work given blk size is less than what is can support? And what exactly happens today if blk_size is out of this range? > > @@ -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, > > -- > > 2.11.0 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-10-04 15:39 ` Michael S. Tsirkin 0 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-10-04 15:39 UTC (permalink / raw) To: Xie Yongji; +Cc: linux-block, linux-kernel, stefanha, virtualization On Mon, Oct 04, 2021 at 11:27:29AM -0400, Michael S. Tsirkin wrote: > On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote: > > 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@bytedance.com> > > So I had to revert this due basically bugs in QEMU. > > My suggestion at this point is to try and update > blk_queue_logical_block_size to BUG_ON when the size > is out of a reasonable range. > > This has the advantage of fixing more hardware, not just virtio. > > > > > --- > > 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); > > + > > + return 0; > > +} > > + > > static int virtblk_probe(struct virtio_device *vdev) > > { > > struct virtio_blk *vblk; I started wondering about this. So let's assume is PAGE_SIZE < blk_size (after all it's up to guest at many platforms). Will using the device even work given blk size is less than what is can support? And what exactly happens today if blk_size is out of this range? > > @@ -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, > > -- > > 2.11.0 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-10-04 15:39 ` Michael S. Tsirkin (?) @ 2021-10-05 15:52 ` Yongji Xie -1 siblings, 0 replies; 48+ messages in thread From: Yongji Xie @ 2021-10-05 15:52 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Mon, Oct 4, 2021 at 11:39 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Oct 04, 2021 at 11:27:29AM -0400, Michael S. Tsirkin wrote: > > On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote: > > > 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@bytedance.com> > > > > So I had to revert this due basically bugs in QEMU. > > > > My suggestion at this point is to try and update > > blk_queue_logical_block_size to BUG_ON when the size > > is out of a reasonable range. > > > > This has the advantage of fixing more hardware, not just virtio. > > > > > > > > > --- > > > 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); > > > + > > > + return 0; > > > +} > > > + > > > static int virtblk_probe(struct virtio_device *vdev) > > > { > > > struct virtio_blk *vblk; > > I started wondering about this. So let's assume is > PAGE_SIZE < blk_size (after all it's up to guest at many platforms). > > Will using the device even work given blk size is less than what > is can support? > > And what exactly happens today if blk_size is out of this range? > Now the block layer can't support the block size larger than the page size. Otherwise, it would cause a random crash, e.g. in block_read_full_page(). Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-10-04 15:27 ` Michael S. Tsirkin @ 2021-10-05 10:42 ` Michael S. Tsirkin -1 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-10-05 10:42 UTC (permalink / raw) To: Xie Yongji Cc: jasowang, stefanha, virtualization, linux-block, linux-kernel, Kevin Wolf, Christoph Hellwig, Jens Axboe On Mon, Oct 04, 2021 at 11:27:29AM -0400, Michael S. Tsirkin wrote: > On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote: > > 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@bytedance.com> > > So I had to revert this due basically bugs in QEMU. > > My suggestion at this point is to try and update > blk_queue_logical_block_size to BUG_ON when the size > is out of a reasonable range. > > This has the advantage of fixing more hardware, not just virtio. > Stefan also pointed out this duplicates the logic from if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) return -EINVAL; and a bunch of other places. Would it be acceptable for blk layer to validate the input instead of having each driver do it's own thing? Maybe inside blk_queue_logical_block_size? > > > --- > > 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); > > + > > + 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, > > -- > > 2.11.0 ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-10-05 10:42 ` Michael S. Tsirkin 0 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-10-05 10:42 UTC (permalink / raw) To: Xie Yongji Cc: Jens Axboe, linux-kernel, virtualization, linux-block, stefanha, Christoph Hellwig On Mon, Oct 04, 2021 at 11:27:29AM -0400, Michael S. Tsirkin wrote: > On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote: > > 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@bytedance.com> > > So I had to revert this due basically bugs in QEMU. > > My suggestion at this point is to try and update > blk_queue_logical_block_size to BUG_ON when the size > is out of a reasonable range. > > This has the advantage of fixing more hardware, not just virtio. > Stefan also pointed out this duplicates the logic from if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) return -EINVAL; and a bunch of other places. Would it be acceptable for blk layer to validate the input instead of having each driver do it's own thing? Maybe inside blk_queue_logical_block_size? > > > --- > > 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); > > + > > + 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, > > -- > > 2.11.0 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-10-05 10:42 ` Michael S. Tsirkin (?) @ 2021-10-05 15:45 ` Yongji Xie -1 siblings, 0 replies; 48+ messages in thread From: Yongji Xie @ 2021-10-05 15:45 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel, Kevin Wolf, Christoph Hellwig, Jens Axboe On Tue, Oct 5, 2021 at 6:42 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Oct 04, 2021 at 11:27:29AM -0400, Michael S. Tsirkin wrote: > > On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote: > > > 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@bytedance.com> > > > > So I had to revert this due basically bugs in QEMU. > > > > My suggestion at this point is to try and update > > blk_queue_logical_block_size to BUG_ON when the size > > is out of a reasonable range. > > > > This has the advantage of fixing more hardware, not just virtio. > > > > Stefan also pointed out this duplicates the logic from > > if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) > return -EINVAL; > > > and a bunch of other places. > > > Would it be acceptable for blk layer to validate the input > instead of having each driver do it's own thing? > Maybe inside blk_queue_logical_block_size? > Now the nbd and loop driver will validate the blksize and return error if it's invalid. But the nvme and null_blk driver just corrects the blksize to a reasonable range without returning any error. I'm not sure which way the block layer should follow. Or just simplify the logic in nbd and loop driver? Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-10-05 10:42 ` Michael S. Tsirkin @ 2021-10-05 18:26 ` Martin K. Petersen -1 siblings, 0 replies; 48+ messages in thread From: Martin K. Petersen @ 2021-10-05 18:26 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Xie Yongji, jasowang, stefanha, virtualization, linux-block, linux-kernel, Kevin Wolf, Christoph Hellwig, Jens Axboe Michael, > Would it be acceptable for blk layer to validate the input instead of > having each driver do it's own thing? Maybe inside > blk_queue_logical_block_size? I think that would be fine. I believe we had some patches floating around a few years ago attempting to make that change. -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-10-05 18:26 ` Martin K. Petersen 0 siblings, 0 replies; 48+ messages in thread From: Martin K. Petersen @ 2021-10-05 18:26 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jens Axboe, linux-kernel, virtualization, linux-block, Xie Yongji, stefanha, Christoph Hellwig Michael, > Would it be acceptable for blk layer to validate the input instead of > having each driver do it's own thing? Maybe inside > blk_queue_logical_block_size? I think that would be fine. I believe we had some patches floating around a few years ago attempting to make that change. -- Martin K. Petersen Oracle Linux Engineering _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-10-05 10:42 ` Michael S. Tsirkin @ 2021-10-11 11:40 ` Christoph Hellwig -1 siblings, 0 replies; 48+ messages in thread From: Christoph Hellwig @ 2021-10-11 11:40 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Xie Yongji, jasowang, stefanha, virtualization, linux-block, linux-kernel, Kevin Wolf, Christoph Hellwig, Jens Axboe On Tue, Oct 05, 2021 at 06:42:43AM -0400, Michael S. Tsirkin wrote: > Stefan also pointed out this duplicates the logic from > > if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) > return -EINVAL; > > > and a bunch of other places. > > > Would it be acceptable for blk layer to validate the input > instead of having each driver do it's own thing? > Maybe inside blk_queue_logical_block_size? I'm pretty sure we want down that before. Let's just add a helper just for that check for now as part of this series. Actually validating in in blk_queue_logical_block_size seems like a good idea, but returning errors from that has a long tail. ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-10-11 11:40 ` Christoph Hellwig 0 siblings, 0 replies; 48+ messages in thread From: Christoph Hellwig @ 2021-10-11 11:40 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jens Axboe, linux-kernel, virtualization, linux-block, Xie Yongji, stefanha, Christoph Hellwig On Tue, Oct 05, 2021 at 06:42:43AM -0400, Michael S. Tsirkin wrote: > Stefan also pointed out this duplicates the logic from > > if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) > return -EINVAL; > > > and a bunch of other places. > > > Would it be acceptable for blk layer to validate the input > instead of having each driver do it's own thing? > Maybe inside blk_queue_logical_block_size? I'm pretty sure we want down that before. Let's just add a helper just for that check for now as part of this series. Actually validating in in blk_queue_logical_block_size seems like a good idea, but returning errors from that has a long tail. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-10-11 11:40 ` Christoph Hellwig @ 2021-10-13 12:21 ` Michael S. Tsirkin -1 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-10-13 12:21 UTC (permalink / raw) To: Christoph Hellwig Cc: Xie Yongji, jasowang, stefanha, virtualization, linux-block, linux-kernel, Kevin Wolf, Jens Axboe On Mon, Oct 11, 2021 at 01:40:41PM +0200, Christoph Hellwig wrote: > On Tue, Oct 05, 2021 at 06:42:43AM -0400, Michael S. Tsirkin wrote: > > Stefan also pointed out this duplicates the logic from > > > > if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) > > return -EINVAL; > > > > > > and a bunch of other places. > > > > > > Would it be acceptable for blk layer to validate the input > > instead of having each driver do it's own thing? > > Maybe inside blk_queue_logical_block_size? > > I'm pretty sure we want down that before. Let's just add a helper > just for that check for now as part of this series. Actually validating > in in blk_queue_logical_block_size seems like a good idea, but returning > errors from that has a long tail. Xie Yongji, I think I will revert this patch for now - can you please work out adding that helper and using it in virtio? Thanks, -- MST ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-10-13 12:21 ` Michael S. Tsirkin 0 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-10-13 12:21 UTC (permalink / raw) To: Christoph Hellwig Cc: Jens Axboe, linux-kernel, virtualization, linux-block, Xie Yongji, stefanha On Mon, Oct 11, 2021 at 01:40:41PM +0200, Christoph Hellwig wrote: > On Tue, Oct 05, 2021 at 06:42:43AM -0400, Michael S. Tsirkin wrote: > > Stefan also pointed out this duplicates the logic from > > > > if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) > > return -EINVAL; > > > > > > and a bunch of other places. > > > > > > Would it be acceptable for blk layer to validate the input > > instead of having each driver do it's own thing? > > Maybe inside blk_queue_logical_block_size? > > I'm pretty sure we want down that before. Let's just add a helper > just for that check for now as part of this series. Actually validating > in in blk_queue_logical_block_size seems like a good idea, but returning > errors from that has a long tail. Xie Yongji, I think I will revert this patch for now - can you please work out adding that helper and using it in virtio? Thanks, -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-10-13 12:21 ` Michael S. Tsirkin (?) @ 2021-10-13 12:34 ` Yongji Xie 2021-10-13 12:51 ` Michael S. Tsirkin -1 siblings, 1 reply; 48+ messages in thread From: Yongji Xie @ 2021-10-13 12:34 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Christoph Hellwig, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel, Kevin Wolf, Jens Axboe On Wed, Oct 13, 2021 at 8:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Oct 11, 2021 at 01:40:41PM +0200, Christoph Hellwig wrote: > > On Tue, Oct 05, 2021 at 06:42:43AM -0400, Michael S. Tsirkin wrote: > > > Stefan also pointed out this duplicates the logic from > > > > > > if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) > > > return -EINVAL; > > > > > > > > > and a bunch of other places. > > > > > > > > > Would it be acceptable for blk layer to validate the input > > > instead of having each driver do it's own thing? > > > Maybe inside blk_queue_logical_block_size? > > > > I'm pretty sure we want down that before. Let's just add a helper > > just for that check for now as part of this series. Actually validating > > in in blk_queue_logical_block_size seems like a good idea, but returning > > errors from that has a long tail. > > Xie Yongji, I think I will revert this patch for now - can you > please work out adding that helper and using it in virtio? > Fine, I will do it. Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-10-13 12:34 ` Yongji Xie @ 2021-10-13 12:51 ` Michael S. Tsirkin 0 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-10-13 12:51 UTC (permalink / raw) To: Yongji Xie Cc: Christoph Hellwig, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel, Kevin Wolf, Jens Axboe On Wed, Oct 13, 2021 at 08:34:22PM +0800, Yongji Xie wrote: > On Wed, Oct 13, 2021 at 8:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Oct 11, 2021 at 01:40:41PM +0200, Christoph Hellwig wrote: > > > On Tue, Oct 05, 2021 at 06:42:43AM -0400, Michael S. Tsirkin wrote: > > > > Stefan also pointed out this duplicates the logic from > > > > > > > > if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) > > > > return -EINVAL; > > > > > > > > > > > > and a bunch of other places. > > > > > > > > > > > > Would it be acceptable for blk layer to validate the input > > > > instead of having each driver do it's own thing? > > > > Maybe inside blk_queue_logical_block_size? > > > > > > I'm pretty sure we want down that before. Let's just add a helper > > > just for that check for now as part of this series. Actually validating > > > in in blk_queue_logical_block_size seems like a good idea, but returning > > > errors from that has a long tail. > > > > Xie Yongji, I think I will revert this patch for now - can you > > please work out adding that helper and using it in virtio? > > > > Fine, I will do it. > > Thanks, > Yongji Great, thanks! And while at it, pls research a bit more and mention in the commit log what is the result of an illegal blk size? Is it memory corruption? A catastrophic failure? If it's one of these cases, then it's ok to just fail probe. -- MST ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space @ 2021-10-13 12:51 ` Michael S. Tsirkin 0 siblings, 0 replies; 48+ messages in thread From: Michael S. Tsirkin @ 2021-10-13 12:51 UTC (permalink / raw) To: Yongji Xie Cc: Jens Axboe, linux-kernel, virtualization, linux-block, Stefan Hajnoczi, Christoph Hellwig On Wed, Oct 13, 2021 at 08:34:22PM +0800, Yongji Xie wrote: > On Wed, Oct 13, 2021 at 8:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Oct 11, 2021 at 01:40:41PM +0200, Christoph Hellwig wrote: > > > On Tue, Oct 05, 2021 at 06:42:43AM -0400, Michael S. Tsirkin wrote: > > > > Stefan also pointed out this duplicates the logic from > > > > > > > > if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) > > > > return -EINVAL; > > > > > > > > > > > > and a bunch of other places. > > > > > > > > > > > > Would it be acceptable for blk layer to validate the input > > > > instead of having each driver do it's own thing? > > > > Maybe inside blk_queue_logical_block_size? > > > > > > I'm pretty sure we want down that before. Let's just add a helper > > > just for that check for now as part of this series. Actually validating > > > in in blk_queue_logical_block_size seems like a good idea, but returning > > > errors from that has a long tail. > > > > Xie Yongji, I think I will revert this patch for now - can you > > please work out adding that helper and using it in virtio? > > > > Fine, I will do it. > > Thanks, > Yongji Great, thanks! And while at it, pls research a bit more and mention in the commit log what is the result of an illegal blk size? Is it memory corruption? A catastrophic failure? If it's one of these cases, then it's ok to just fail probe. -- MST _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-10-13 12:51 ` Michael S. Tsirkin (?) @ 2021-10-13 12:59 ` Yongji Xie -1 siblings, 0 replies; 48+ messages in thread From: Yongji Xie @ 2021-10-13 12:59 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Christoph Hellwig, Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel, Kevin Wolf, Jens Axboe On Wed, Oct 13, 2021 at 8:51 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Oct 13, 2021 at 08:34:22PM +0800, Yongji Xie wrote: > > On Wed, Oct 13, 2021 at 8:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Mon, Oct 11, 2021 at 01:40:41PM +0200, Christoph Hellwig wrote: > > > > On Tue, Oct 05, 2021 at 06:42:43AM -0400, Michael S. Tsirkin wrote: > > > > > Stefan also pointed out this duplicates the logic from > > > > > > > > > > if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize)) > > > > > return -EINVAL; > > > > > > > > > > > > > > > and a bunch of other places. > > > > > > > > > > > > > > > Would it be acceptable for blk layer to validate the input > > > > > instead of having each driver do it's own thing? > > > > > Maybe inside blk_queue_logical_block_size? > > > > > > > > I'm pretty sure we want down that before. Let's just add a helper > > > > just for that check for now as part of this series. Actually validating > > > > in in blk_queue_logical_block_size seems like a good idea, but returning > > > > errors from that has a long tail. > > > > > > Xie Yongji, I think I will revert this patch for now - can you > > > please work out adding that helper and using it in virtio? > > > > > > > Fine, I will do it. > > > > Thanks, > > Yongji > > Great, thanks! And while at it, pls research a bit more and mention > in the commit log what is the result of an illegal blk size? > Is it memory corruption? A catastrophic failure? > If it's one of these cases, then it's ok to just fail probe. > Sure, and I think it will be one of these cases. Will add some stack dump in the commit log. Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] virtio-blk: Add validation for block size in config space 2021-10-04 15:27 ` Michael S. Tsirkin ` (2 preceding siblings ...) (?) @ 2021-10-05 15:24 ` Yongji Xie -1 siblings, 0 replies; 48+ messages in thread From: Yongji Xie @ 2021-10-05 15:24 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Jason Wang, Stefan Hajnoczi, virtualization, linux-block, linux-kernel On Mon, Oct 4, 2021 at 11:27 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote: > > 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@bytedance.com> > > So I had to revert this due basically bugs in QEMU. > > My suggestion at this point is to try and update > blk_queue_logical_block_size to BUG_ON when the size > is out of a reasonable range. > > This has the advantage of fixing more hardware, not just virtio. > I wonder if it's better to just add a new patch to remove the virtblk_validate() part. And the check of block size in virtblk_probe() can be safely removed after the block layer is changed to validate the block size. Thanks, Yongji ^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2021-10-13 12:59 UTC | newest] Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-09 10:16 [PATCH v5] virtio-blk: Add validation for block size in config space Xie Yongji 2021-08-10 3:05 ` Jason Wang 2021-08-10 3:05 ` Jason Wang 2021-08-10 4:59 ` Yongji Xie 2021-08-10 6:59 ` Jason Wang 2021-08-10 6:59 ` Jason Wang 2021-08-22 23:17 ` Max Gurtovoy 2021-08-23 4:31 ` Yongji Xie 2021-08-23 8:07 ` Max Gurtovoy 2021-08-23 8:35 ` Yongji Xie 2021-08-23 9:04 ` Max Gurtovoy 2021-08-23 9:27 ` Yongji Xie 2021-08-23 9:38 ` Max Gurtovoy 2021-08-23 10:33 ` Yongji Xie 2021-08-23 10:45 ` Max Gurtovoy 2021-08-23 11:41 ` Yongji Xie 2021-08-23 12:13 ` Michael S. Tsirkin 2021-08-23 12:13 ` Michael S. Tsirkin 2021-08-23 12:40 ` Yongji Xie 2021-08-23 16:02 ` Michael S. Tsirkin 2021-08-23 16:02 ` Michael S. Tsirkin 2021-08-23 22:31 ` Max Gurtovoy 2021-08-24 2:47 ` Jason Wang 2021-08-24 2:47 ` Jason Wang 2021-08-24 10:11 ` Max Gurtovoy 2021-08-24 12:52 ` Yongji Xie 2021-08-24 13:30 ` Max Gurtovoy 2021-08-24 13:38 ` Yongji Xie 2021-08-24 13:48 ` Max Gurtovoy 2021-10-04 15:27 ` Michael S. Tsirkin 2021-10-04 15:27 ` Michael S. Tsirkin 2021-10-04 15:39 ` Michael S. Tsirkin 2021-10-04 15:39 ` Michael S. Tsirkin 2021-10-05 15:52 ` Yongji Xie 2021-10-05 10:42 ` Michael S. Tsirkin 2021-10-05 10:42 ` Michael S. Tsirkin 2021-10-05 15:45 ` Yongji Xie 2021-10-05 18:26 ` Martin K. Petersen 2021-10-05 18:26 ` Martin K. Petersen 2021-10-11 11:40 ` Christoph Hellwig 2021-10-11 11:40 ` Christoph Hellwig 2021-10-13 12:21 ` Michael S. Tsirkin 2021-10-13 12:21 ` Michael S. Tsirkin 2021-10-13 12:34 ` Yongji Xie 2021-10-13 12:51 ` Michael S. Tsirkin 2021-10-13 12:51 ` Michael S. Tsirkin 2021-10-13 12:59 ` Yongji Xie 2021-10-05 15:24 ` Yongji Xie
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.