Re: [PATCH v5] virtio-blk: Add validation for block size in config space

Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Jason Wang]

在 2021/8/9 下午6:16, Xie Yongji 写道:
> An untrusted device might presents an invalid block size
> in configuration space. This tries to add validation for it
> in the validate callback and clear the VIRTIO_BLK_F_BLK_SIZE
> feature bit if the value is out of the supported range.
>
> And we also double check the value in virtblk_probe() in
> case that it's changed after the validation.
>
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>   drivers/block/virtio_blk.c | 39 +++++++++++++++++++++++++++++++++------
>   1 file changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 4b49df2dfd23..afb37aac09e8 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -692,6 +692,28 @@ static const struct blk_mq_ops virtio_mq_ops = {
>   static unsigned int virtblk_queue_depth;
>   module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
>   
> +static int virtblk_validate(struct virtio_device *vdev)
> +{
> +	u32 blk_size;
> +
> +	if (!vdev->config->get) {
> +		dev_err(&vdev->dev, "%s failure: config access disabled\n",
> +			__func__);
> +		return -EINVAL;
> +	}
> +
> +	if (!virtio_has_feature(vdev, VIRTIO_BLK_F_BLK_SIZE))
> +		return 0;
> +
> +	blk_size = virtio_cread32(vdev,
> +			offsetof(struct virtio_blk_config, blk_size));
> +
> +	if (blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)
> +		__virtio_clear_bit(vdev, VIRTIO_BLK_F_BLK_SIZE);


I wonder if it's better to just fail here as what we did for probe().

Thanks


> +
> +	return 0;
> +}
> +
>   static int virtblk_probe(struct virtio_device *vdev)
>   {
>   	struct virtio_blk *vblk;
> @@ -703,12 +725,6 @@ static int virtblk_probe(struct virtio_device *vdev)
>   	u8 physical_block_exp, alignment_offset;
>   	unsigned int queue_depth;
>   
> -	if (!vdev->config->get) {
> -		dev_err(&vdev->dev, "%s failure: config access disabled\n",
> -			__func__);
> -		return -EINVAL;
> -	}
> -
>   	err = ida_simple_get(&vd_index_ida, 0, minor_to_index(1 << MINORBITS),
>   			     GFP_KERNEL);
>   	if (err < 0)
> @@ -823,6 +839,14 @@ static int virtblk_probe(struct virtio_device *vdev)
>   	else
>   		blk_size = queue_logical_block_size(q);
>   
> +	if (unlikely(blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)) {
> +		dev_err(&vdev->dev,
> +			"block size is changed unexpectedly, now is %u\n",
> +			blk_size);
> +		err = -EINVAL;
> +		goto err_cleanup_disk;
> +	}
> +
>   	/* Use topology information if available */
>   	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,
>   				   struct virtio_blk_config, physical_block_exp,
> @@ -881,6 +905,8 @@ static int virtblk_probe(struct virtio_device *vdev)
>   	device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups);
>   	return 0;
>   
> +err_cleanup_disk:
> +	blk_cleanup_disk(vblk->disk);
>   out_free_tags:
>   	blk_mq_free_tag_set(&vblk->tag_set);
>   out_free_vq:
> @@ -983,6 +1009,7 @@ static struct virtio_driver virtio_blk = {
>   	.driver.name			= KBUILD_MODNAME,
>   	.driver.owner			= THIS_MODULE,
>   	.id_table			= id_table,
> +	.validate			= virtblk_validate,
>   	.probe				= virtblk_probe,
>   	.remove				= virtblk_remove,
>   	.config_changed			= virtblk_config_changed,

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Jason Wang]

在 2021/8/10 下午12:59, Yongji Xie 写道:
> On Tue, Aug 10, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> 在 2021/8/9 下午6:16, Xie Yongji 写道:
>>> An untrusted device might presents an invalid block size
>>> in configuration space. This tries to add validation for it
>>> in the validate callback and clear the VIRTIO_BLK_F_BLK_SIZE
>>> feature bit if the value is out of the supported range.
>>>
>>> And we also double check the value in virtblk_probe() in
>>> case that it's changed after the validation.
>>>
>>> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
>>> ---
>>>    drivers/block/virtio_blk.c | 39 +++++++++++++++++++++++++++++++++------
>>>    1 file changed, 33 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>>> index 4b49df2dfd23..afb37aac09e8 100644
>>> --- a/drivers/block/virtio_blk.c
>>> +++ b/drivers/block/virtio_blk.c
>>> @@ -692,6 +692,28 @@ static const struct blk_mq_ops virtio_mq_ops = {
>>>    static unsigned int virtblk_queue_depth;
>>>    module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
>>>
>>> +static int virtblk_validate(struct virtio_device *vdev)
>>> +{
>>> +     u32 blk_size;
>>> +
>>> +     if (!vdev->config->get) {
>>> +             dev_err(&vdev->dev, "%s failure: config access disabled\n",
>>> +                     __func__);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     if (!virtio_has_feature(vdev, VIRTIO_BLK_F_BLK_SIZE))
>>> +             return 0;
>>> +
>>> +     blk_size = virtio_cread32(vdev,
>>> +                     offsetof(struct virtio_blk_config, blk_size));
>>> +
>>> +     if (blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)
>>> +             __virtio_clear_bit(vdev, VIRTIO_BLK_F_BLK_SIZE);
>>
>> I wonder if it's better to just fail here as what we did for probe().
>>
> Looks like we don't need to do that since we already clear the
> VIRTIO_BLK_F_BLK_SIZE to tell the device that we don't use the block
> size in configuration space. Just like what we did in
> virtnet_validate().
>
> Thanks,
> Yongji


Ok, so

Acked-by: Jason Wang <jasowang@redhat.com>



>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Michael S. Tsirkin]

On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote:
> It helpful if there is a justification for this.
> 
> In this case, no such HW device exist and the only device that can cause
> this trouble today is user space VDUSE device that must be validated by the
> emulation VDUSE kernel driver.
> 
> Otherwise, will can create 1000 commit like this in the virtio level (for
> example for each feature for each virtio device).

Yea, it's a lot of work but I don't think it's avoidable.

> > 
> > > > > > And regardless of userspace device, we still need to fix it for other cases.
> > > > > which cases ? Do you know that there is a buggy HW we need to workaround ?
> > > > > 
> > > > No, there isn't now. But this could be a potential attack surface if
> > > > the host doesn't trust the device.
> > > If the host doesn't trust a device, why it continues using it ?
> > > 
> > IIUC this is the case for the encrypted VMs.
> 
> what do you mean encrypted VM ?
> 
> And how this small patch causes a VM to be 100% encryption supported ?
> 
> > > Do you suggest we do these workarounds in all device drivers in the kernel ?
> > > 
> > Isn't it the driver's job to validate some unreasonable configuration?
> 
> The check should be in different layer.
> 
> Virtio blk driver should not cover on some strange VDUSE stuff.

Yes I'm not convinced VDUSE is a valid use-case. I think that for
security and robustness it should validate data it gets from userspace
right there after reading it.
But I think this is useful for the virtio hardening thing.
https://lwn.net/Articles/865216/

Yongji - I think the commit log should be much more explicit that
this is hardening. Otherwise people get confused and think this
needs a CVE or a backport for security.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Michael S. Tsirkin]

On Mon, Aug 23, 2021 at 08:40:30PM +0800, Yongji Xie wrote:
> On Mon, Aug 23, 2021 at 8:13 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote:
> > > It helpful if there is a justification for this.
> > >
> > > In this case, no such HW device exist and the only device that can cause
> > > this trouble today is user space VDUSE device that must be validated by the
> > > emulation VDUSE kernel driver.
> > >
> > > Otherwise, will can create 1000 commit like this in the virtio level (for
> > > example for each feature for each virtio device).
> >
> > Yea, it's a lot of work but I don't think it's avoidable.
> >
> > > >
> > > > > > > > And regardless of userspace device, we still need to fix it for other cases.
> > > > > > > which cases ? Do you know that there is a buggy HW we need to workaround ?
> > > > > > >
> > > > > > No, there isn't now. But this could be a potential attack surface if
> > > > > > the host doesn't trust the device.
> > > > > If the host doesn't trust a device, why it continues using it ?
> > > > >
> > > > IIUC this is the case for the encrypted VMs.
> > >
> > > what do you mean encrypted VM ?
> > >
> > > And how this small patch causes a VM to be 100% encryption supported ?
> > >
> > > > > Do you suggest we do these workarounds in all device drivers in the kernel ?
> > > > >
> > > > Isn't it the driver's job to validate some unreasonable configuration?
> > >
> > > The check should be in different layer.
> > >
> > > Virtio blk driver should not cover on some strange VDUSE stuff.
> >
> > Yes I'm not convinced VDUSE is a valid use-case. I think that for
> > security and robustness it should validate data it gets from userspace
> > right there after reading it.
> > But I think this is useful for the virtio hardening thing.
> > https://lwn.net/Articles/865216/
> >
> > Yongji - I think the commit log should be much more explicit that
> > this is hardening. Otherwise people get confused and think this
> > needs a CVE or a backport for security.
> >
> 
> OK, do I need to send a v6? This patch seems to be already merged into
> Linus's tree.
> 
> Thanks,
> Yongji

No, it's a comment for the future - I assume you will keep adding this
kind of validation in other places.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Jason Wang]

On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
>
>
> On 8/23/2021 3:13 PM, Michael S. Tsirkin wrote:
> > On Mon, Aug 23, 2021 at 01:45:31PM +0300, Max Gurtovoy wrote:
> >> It helpful if there is a justification for this.
> >>
> >> In this case, no such HW device exist and the only device that can cause
> >> this trouble today is user space VDUSE device that must be validated by the
> >> emulation VDUSE kernel driver.
> >>
> >> Otherwise, will can create 1000 commit like this in the virtio level (for
> >> example for each feature for each virtio device).
> > Yea, it's a lot of work but I don't think it's avoidable.
> >
> >>>>>>> And regardless of userspace device, we still need to fix it for other cases.
> >>>>>> which cases ? Do you know that there is a buggy HW we need to workaround ?
> >>>>>>
> >>>>> No, there isn't now. But this could be a potential attack surface if
> >>>>> the host doesn't trust the device.
> >>>> If the host doesn't trust a device, why it continues using it ?
> >>>>
> >>> IIUC this is the case for the encrypted VMs.
> >> what do you mean encrypted VM ?
> >>
> >> And how this small patch causes a VM to be 100% encryption supported ?
> >>
> >>>> Do you suggest we do these workarounds in all device drivers in the kernel ?
> >>>>
> >>> Isn't it the driver's job to validate some unreasonable configuration?
> >> The check should be in different layer.
> >>
> >> Virtio blk driver should not cover on some strange VDUSE stuff.
> > Yes I'm not convinced VDUSE is a valid use-case. I think that for
> > security and robustness it should validate data it gets from userspace
> > right there after reading it.
> > But I think this is useful for the virtio hardening thing.
> > https://lwn.net/Articles/865216/
>
> I don't see how this change is assisting confidential computing.
>
> Confidential computingtalks about encrypting guest memory from the host,
> and not adding some quirks to devices.

In the case of confidential computing, the hypervisor and hard device
is not in the trust zone. It means the guest doesn't trust the cloud
vendor.

That's why we need to validate any input from them.

Thanks

>
> >
> > Yongji - I think the commit log should be much more explicit that
> > this is hardening. Otherwise people get confused and think this
> > needs a CVE or a backport for security.
> >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Michael S. Tsirkin]

On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote:
> An untrusted device might presents an invalid block size
> in configuration space. This tries to add validation for it
> in the validate callback and clear the VIRTIO_BLK_F_BLK_SIZE
> feature bit if the value is out of the supported range.
> 
> And we also double check the value in virtblk_probe() in
> case that it's changed after the validation.
> 
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>

So I had to revert this due basically bugs in QEMU.

My suggestion at this point is to try and update
blk_queue_logical_block_size to BUG_ON when the size
is out of a reasonable range.

This has the advantage of fixing more hardware, not just virtio.



> ---
>  drivers/block/virtio_blk.c | 39 +++++++++++++++++++++++++++++++++------
>  1 file changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 4b49df2dfd23..afb37aac09e8 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -692,6 +692,28 @@ static const struct blk_mq_ops virtio_mq_ops = {
>  static unsigned int virtblk_queue_depth;
>  module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
>  
> +static int virtblk_validate(struct virtio_device *vdev)
> +{
> +	u32 blk_size;
> +
> +	if (!vdev->config->get) {
> +		dev_err(&vdev->dev, "%s failure: config access disabled\n",
> +			__func__);
> +		return -EINVAL;
> +	}
> +
> +	if (!virtio_has_feature(vdev, VIRTIO_BLK_F_BLK_SIZE))
> +		return 0;
> +
> +	blk_size = virtio_cread32(vdev,
> +			offsetof(struct virtio_blk_config, blk_size));
> +
> +	if (blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)
> +		__virtio_clear_bit(vdev, VIRTIO_BLK_F_BLK_SIZE);
> +
> +	return 0;
> +}
> +
>  static int virtblk_probe(struct virtio_device *vdev)
>  {
>  	struct virtio_blk *vblk;
> @@ -703,12 +725,6 @@ static int virtblk_probe(struct virtio_device *vdev)
>  	u8 physical_block_exp, alignment_offset;
>  	unsigned int queue_depth;
>  
> -	if (!vdev->config->get) {
> -		dev_err(&vdev->dev, "%s failure: config access disabled\n",
> -			__func__);
> -		return -EINVAL;
> -	}
> -
>  	err = ida_simple_get(&vd_index_ida, 0, minor_to_index(1 << MINORBITS),
>  			     GFP_KERNEL);
>  	if (err < 0)
> @@ -823,6 +839,14 @@ static int virtblk_probe(struct virtio_device *vdev)
>  	else
>  		blk_size = queue_logical_block_size(q);
>  
> +	if (unlikely(blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)) {
> +		dev_err(&vdev->dev,
> +			"block size is changed unexpectedly, now is %u\n",
> +			blk_size);
> +		err = -EINVAL;
> +		goto err_cleanup_disk;
> +	}
> +
>  	/* Use topology information if available */
>  	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,
>  				   struct virtio_blk_config, physical_block_exp,
> @@ -881,6 +905,8 @@ static int virtblk_probe(struct virtio_device *vdev)
>  	device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups);
>  	return 0;
>  
> +err_cleanup_disk:
> +	blk_cleanup_disk(vblk->disk);
>  out_free_tags:
>  	blk_mq_free_tag_set(&vblk->tag_set);
>  out_free_vq:
> @@ -983,6 +1009,7 @@ static struct virtio_driver virtio_blk = {
>  	.driver.name			= KBUILD_MODNAME,
>  	.driver.owner			= THIS_MODULE,
>  	.id_table			= id_table,
> +	.validate			= virtblk_validate,
>  	.probe				= virtblk_probe,
>  	.remove				= virtblk_remove,
>  	.config_changed			= virtblk_config_changed,
> -- 
> 2.11.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Michael S. Tsirkin]

On Mon, Oct 04, 2021 at 11:27:29AM -0400, Michael S. Tsirkin wrote:
> On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote:
> > An untrusted device might presents an invalid block size
> > in configuration space. This tries to add validation for it
> > in the validate callback and clear the VIRTIO_BLK_F_BLK_SIZE
> > feature bit if the value is out of the supported range.
> > 
> > And we also double check the value in virtblk_probe() in
> > case that it's changed after the validation.
> > 
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> 
> So I had to revert this due basically bugs in QEMU.
> 
> My suggestion at this point is to try and update
> blk_queue_logical_block_size to BUG_ON when the size
> is out of a reasonable range.
> 
> This has the advantage of fixing more hardware, not just virtio.
> 
> 
> 
> > ---
> >  drivers/block/virtio_blk.c | 39 +++++++++++++++++++++++++++++++++------
> >  1 file changed, 33 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 4b49df2dfd23..afb37aac09e8 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -692,6 +692,28 @@ static const struct blk_mq_ops virtio_mq_ops = {
> >  static unsigned int virtblk_queue_depth;
> >  module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
> >  
> > +static int virtblk_validate(struct virtio_device *vdev)
> > +{
> > +	u32 blk_size;
> > +
> > +	if (!vdev->config->get) {
> > +		dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > +			__func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!virtio_has_feature(vdev, VIRTIO_BLK_F_BLK_SIZE))
> > +		return 0;
> > +
> > +	blk_size = virtio_cread32(vdev,
> > +			offsetof(struct virtio_blk_config, blk_size));
> > +
> > +	if (blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)
> > +		__virtio_clear_bit(vdev, VIRTIO_BLK_F_BLK_SIZE);
> > +
> > +	return 0;
> > +}
> > +
> >  static int virtblk_probe(struct virtio_device *vdev)
> >  {
> >  	struct virtio_blk *vblk;

I started wondering about this. So let's assume is
PAGE_SIZE < blk_size (after all it's up to guest at many platforms).

Will using the device even work given blk size is less than what
is can support?

And what exactly happens today if blk_size is out of this range?





> > @@ -703,12 +725,6 @@ static int virtblk_probe(struct virtio_device *vdev)
> >  	u8 physical_block_exp, alignment_offset;
> >  	unsigned int queue_depth;
> >  
> > -	if (!vdev->config->get) {
> > -		dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > -			__func__);
> > -		return -EINVAL;
> > -	}
> > -
> >  	err = ida_simple_get(&vd_index_ida, 0, minor_to_index(1 << MINORBITS),
> >  			     GFP_KERNEL);
> >  	if (err < 0)
> > @@ -823,6 +839,14 @@ static int virtblk_probe(struct virtio_device *vdev)
> >  	else
> >  		blk_size = queue_logical_block_size(q);
> >  
> > +	if (unlikely(blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)) {
> > +		dev_err(&vdev->dev,
> > +			"block size is changed unexpectedly, now is %u\n",
> > +			blk_size);
> > +		err = -EINVAL;
> > +		goto err_cleanup_disk;
> > +	}
> > +
> >  	/* Use topology information if available */
> >  	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,
> >  				   struct virtio_blk_config, physical_block_exp,
> > @@ -881,6 +905,8 @@ static int virtblk_probe(struct virtio_device *vdev)
> >  	device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups);
> >  	return 0;
> >  
> > +err_cleanup_disk:
> > +	blk_cleanup_disk(vblk->disk);
> >  out_free_tags:
> >  	blk_mq_free_tag_set(&vblk->tag_set);
> >  out_free_vq:
> > @@ -983,6 +1009,7 @@ static struct virtio_driver virtio_blk = {
> >  	.driver.name			= KBUILD_MODNAME,
> >  	.driver.owner			= THIS_MODULE,
> >  	.id_table			= id_table,
> > +	.validate			= virtblk_validate,
> >  	.probe				= virtblk_probe,
> >  	.remove				= virtblk_remove,
> >  	.config_changed			= virtblk_config_changed,
> > -- 
> > 2.11.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Michael S. Tsirkin]

On Mon, Oct 04, 2021 at 11:27:29AM -0400, Michael S. Tsirkin wrote:
> On Mon, Aug 09, 2021 at 06:16:09PM +0800, Xie Yongji wrote:
> > An untrusted device might presents an invalid block size
> > in configuration space. This tries to add validation for it
> > in the validate callback and clear the VIRTIO_BLK_F_BLK_SIZE
> > feature bit if the value is out of the supported range.
> > 
> > And we also double check the value in virtblk_probe() in
> > case that it's changed after the validation.
> > 
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> 
> So I had to revert this due basically bugs in QEMU.
> 
> My suggestion at this point is to try and update
> blk_queue_logical_block_size to BUG_ON when the size
> is out of a reasonable range.
> 
> This has the advantage of fixing more hardware, not just virtio.
> 

Stefan also pointed out this duplicates the logic from 

        if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize))
                return -EINVAL;


and a bunch of other places.


Would it be acceptable for blk layer to validate the input
instead of having each driver do it's own thing?
Maybe inside blk_queue_logical_block_size?



> 
> > ---
> >  drivers/block/virtio_blk.c | 39 +++++++++++++++++++++++++++++++++------
> >  1 file changed, 33 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 4b49df2dfd23..afb37aac09e8 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -692,6 +692,28 @@ static const struct blk_mq_ops virtio_mq_ops = {
> >  static unsigned int virtblk_queue_depth;
> >  module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
> >  
> > +static int virtblk_validate(struct virtio_device *vdev)
> > +{
> > +	u32 blk_size;
> > +
> > +	if (!vdev->config->get) {
> > +		dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > +			__func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!virtio_has_feature(vdev, VIRTIO_BLK_F_BLK_SIZE))
> > +		return 0;
> > +
> > +	blk_size = virtio_cread32(vdev,
> > +			offsetof(struct virtio_blk_config, blk_size));
> > +
> > +	if (blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)
> > +		__virtio_clear_bit(vdev, VIRTIO_BLK_F_BLK_SIZE);
> > +
> > +	return 0;
> > +}
> > +
> >  static int virtblk_probe(struct virtio_device *vdev)
> >  {
> >  	struct virtio_blk *vblk;
> > @@ -703,12 +725,6 @@ static int virtblk_probe(struct virtio_device *vdev)
> >  	u8 physical_block_exp, alignment_offset;
> >  	unsigned int queue_depth;
> >  
> > -	if (!vdev->config->get) {
> > -		dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > -			__func__);
> > -		return -EINVAL;
> > -	}
> > -
> >  	err = ida_simple_get(&vd_index_ida, 0, minor_to_index(1 << MINORBITS),
> >  			     GFP_KERNEL);
> >  	if (err < 0)
> > @@ -823,6 +839,14 @@ static int virtblk_probe(struct virtio_device *vdev)
> >  	else
> >  		blk_size = queue_logical_block_size(q);
> >  
> > +	if (unlikely(blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)) {
> > +		dev_err(&vdev->dev,
> > +			"block size is changed unexpectedly, now is %u\n",
> > +			blk_size);
> > +		err = -EINVAL;
> > +		goto err_cleanup_disk;
> > +	}
> > +
> >  	/* Use topology information if available */
> >  	err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,
> >  				   struct virtio_blk_config, physical_block_exp,
> > @@ -881,6 +905,8 @@ static int virtblk_probe(struct virtio_device *vdev)
> >  	device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups);
> >  	return 0;
> >  
> > +err_cleanup_disk:
> > +	blk_cleanup_disk(vblk->disk);
> >  out_free_tags:
> >  	blk_mq_free_tag_set(&vblk->tag_set);
> >  out_free_vq:
> > @@ -983,6 +1009,7 @@ static struct virtio_driver virtio_blk = {
> >  	.driver.name			= KBUILD_MODNAME,
> >  	.driver.owner			= THIS_MODULE,
> >  	.id_table			= id_table,
> > +	.validate			= virtblk_validate,
> >  	.probe				= virtblk_probe,
> >  	.remove				= virtblk_remove,
> >  	.config_changed			= virtblk_config_changed,
> > -- 
> > 2.11.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Martin K. Petersen]

Michael,

> Would it be acceptable for blk layer to validate the input instead of
> having each driver do it's own thing?  Maybe inside
> blk_queue_logical_block_size?

I think that would be fine. I believe we had some patches floating
around a few years ago attempting to make that change.

-- 
Martin K. Petersen	Oracle Linux Engineering
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Christoph Hellwig]

On Tue, Oct 05, 2021 at 06:42:43AM -0400, Michael S. Tsirkin wrote:
> Stefan also pointed out this duplicates the logic from 
> 
>         if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize))
>                 return -EINVAL;
> 
> 
> and a bunch of other places.
> 
> 
> Would it be acceptable for blk layer to validate the input
> instead of having each driver do it's own thing?
> Maybe inside blk_queue_logical_block_size?

I'm pretty sure we want down that before.  Let's just add a helper
just for that check for now as part of this series.  Actually validating
in in blk_queue_logical_block_size seems like a good idea, but returning
errors from that has a long tail.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Michael S. Tsirkin]

On Mon, Oct 11, 2021 at 01:40:41PM +0200, Christoph Hellwig wrote:
> On Tue, Oct 05, 2021 at 06:42:43AM -0400, Michael S. Tsirkin wrote:
> > Stefan also pointed out this duplicates the logic from 
> > 
> >         if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize))
> >                 return -EINVAL;
> > 
> > 
> > and a bunch of other places.
> > 
> > 
> > Would it be acceptable for blk layer to validate the input
> > instead of having each driver do it's own thing?
> > Maybe inside blk_queue_logical_block_size?
> 
> I'm pretty sure we want down that before.  Let's just add a helper
> just for that check for now as part of this series.  Actually validating
> in in blk_queue_logical_block_size seems like a good idea, but returning
> errors from that has a long tail.

Xie Yongji, I think I will revert this patch for now - can you
please work out adding that helper and using it in virtio?

Thanks,

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5] virtio-blk: Add validation for block size in config space

[Michael S. Tsirkin]

On Wed, Oct 13, 2021 at 08:34:22PM +0800, Yongji Xie wrote:
> On Wed, Oct 13, 2021 at 8:21 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Oct 11, 2021 at 01:40:41PM +0200, Christoph Hellwig wrote:
> > > On Tue, Oct 05, 2021 at 06:42:43AM -0400, Michael S. Tsirkin wrote:
> > > > Stefan also pointed out this duplicates the logic from
> > > >
> > > >         if (blksize < 512 || blksize > PAGE_SIZE || !is_power_of_2(blksize))
> > > >                 return -EINVAL;
> > > >
> > > >
> > > > and a bunch of other places.
> > > >
> > > >
> > > > Would it be acceptable for blk layer to validate the input
> > > > instead of having each driver do it's own thing?
> > > > Maybe inside blk_queue_logical_block_size?
> > >
> > > I'm pretty sure we want down that before.  Let's just add a helper
> > > just for that check for now as part of this series.  Actually validating
> > > in in blk_queue_logical_block_size seems like a good idea, but returning
> > > errors from that has a long tail.
> >
> > Xie Yongji, I think I will revert this patch for now - can you
> > please work out adding that helper and using it in virtio?
> >
> 
> Fine, I will do it.
> 
> Thanks,
> Yongji

Great, thanks! And while at it, pls research a bit more and mention
in the commit log what is the result of an illegal blk size?
Is it memory corruption? A catastrophic failure?
If it's one of these cases, then it's ok to just fail probe.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization