linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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; 36+ 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] 36+ 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-10  4:59   ` Yongji Xie
  2021-08-22 23:17 ` Max Gurtovoy
  2021-10-04 15:27 ` Michael S. Tsirkin
  2 siblings, 1 reply; 36+ 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] 36+ 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
  0 siblings, 1 reply; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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
  2021-08-23 12:40                     ` Yongji Xie
  2021-08-23 22:31                     ` Max Gurtovoy
  1 sibling, 2 replies; 36+ 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] 36+ 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
  2021-08-23 22:31                     ` Max Gurtovoy
  1 sibling, 1 reply; 36+ 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] 36+ 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; 36+ 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] 36+ 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 22:31                     ` Max Gurtovoy
  2021-08-24  2:47                       ` Jason Wang
  1 sibling, 1 reply; 36+ 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] 36+ 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
  2021-08-24 10:11                         ` Max Gurtovoy
  0 siblings, 1 reply; 36+ 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] 36+ 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
  0 siblings, 1 reply; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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; 36+ 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] 36+ 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
  2021-10-04 15:39   ` Michael S. Tsirkin
                     ` (2 more replies)
  2 siblings, 3 replies; 36+ 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] 36+ 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
  2021-10-05 15:52     ` Yongji Xie
  2021-10-05 10:42   ` Michael S. Tsirkin
  2021-10-05 15:24   ` Yongji Xie
  2 siblings, 1 reply; 36+ 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] 36+ 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
@ 2021-10-05 10:42   ` Michael S. Tsirkin
  2021-10-05 15:45     ` Yongji Xie
                       ` (2 more replies)
  2021-10-05 15:24   ` Yongji Xie
  2 siblings, 3 replies; 36+ 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] 36+ 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
  2021-10-05 10:42   ` Michael S. Tsirkin
@ 2021-10-05 15:24   ` Yongji Xie
  2 siblings, 0 replies; 36+ 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] 36+ 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
  2021-10-05 18:26     ` Martin K. Petersen
  2021-10-11 11:40     ` Christoph Hellwig
  2 siblings, 0 replies; 36+ 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] 36+ 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
  0 siblings, 0 replies; 36+ 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] 36+ 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
@ 2021-10-05 18:26     ` Martin K. Petersen
  2021-10-11 11:40     ` Christoph Hellwig
  2 siblings, 0 replies; 36+ 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] 36+ 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
  2021-10-05 18:26     ` Martin K. Petersen
@ 2021-10-11 11:40     ` Christoph Hellwig
  2021-10-13 12:21       ` Michael S. Tsirkin
  2 siblings, 1 reply; 36+ 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] 36+ 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
  2021-10-13 12:34         ` Yongji Xie
  0 siblings, 1 reply; 36+ 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] 36+ 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
  0 siblings, 1 reply; 36+ 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] 36+ 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
  2021-10-13 12:59             ` Yongji Xie
  0 siblings, 1 reply; 36+ 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] 36+ 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
  0 siblings, 0 replies; 36+ 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] 36+ messages in thread

end of thread, other threads:[~2021-10-13 12:59 UTC | newest]

Thread overview: 36+ 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  4:59   ` Yongji Xie
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:40                     ` Yongji Xie
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 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:39   ` Michael S. Tsirkin
2021-10-05 15:52     ` Yongji Xie
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-11 11:40     ` Christoph Hellwig
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:59             ` Yongji Xie
2021-10-05 15:24   ` Yongji Xie

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).