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 <[email protected]>
---
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
On Tue, Aug 10, 2021 at 11:05 AM Jason Wang <[email protected]> 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 <[email protected]>
> > ---
> > 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
?? 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 <[email protected]>
> ---
> 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,
在 2021/8/10 下午12:59, Yongji Xie 写道:
> On Tue, Aug 10, 2021 at 11:05 AM Jason Wang <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>
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 <[email protected]>
> ---
> 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,
On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <[email protected]> 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/[email protected]/
> 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 <[email protected]>
> > ---
> > 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
On 8/23/2021 7:31 AM, Yongji Xie wrote:
> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <[email protected]> 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/[email protected]/
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 <[email protected]>
>>> ---
>>> 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
On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <[email protected]> wrote:
>
>
> On 8/23/2021 7:31 AM, Yongji Xie wrote:
> > On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <[email protected]> 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/[email protected]/
>
> 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
On 8/23/2021 11:35 AM, Yongji Xie wrote:
> On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <[email protected]> wrote:
>>
>> On 8/23/2021 7:31 AM, Yongji Xie wrote:
>>> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <[email protected]> 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/[email protected]/
>> 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
On Mon, Aug 23, 2021 at 5:04 PM Max Gurtovoy <[email protected]> wrote:
>
>
> On 8/23/2021 11:35 AM, Yongji Xie wrote:
> > On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <[email protected]> wrote:
> >>
> >> On 8/23/2021 7:31 AM, Yongji Xie wrote:
> >>> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <[email protected]> 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/[email protected]/
> >> 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
On 8/23/2021 12:27 PM, Yongji Xie wrote:
> On Mon, Aug 23, 2021 at 5:04 PM Max Gurtovoy <[email protected]> wrote:
>>
>> On 8/23/2021 11:35 AM, Yongji Xie wrote:
>>> On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <[email protected]> wrote:
>>>> On 8/23/2021 7:31 AM, Yongji Xie wrote:
>>>>> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <[email protected]> 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/[email protected]/
>>>> 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
On Mon, Aug 23, 2021 at 5:38 PM Max Gurtovoy <[email protected]> wrote:
>
>
> On 8/23/2021 12:27 PM, Yongji Xie wrote:
> > On Mon, Aug 23, 2021 at 5:04 PM Max Gurtovoy <[email protected]> wrote:
> >>
> >> On 8/23/2021 11:35 AM, Yongji Xie wrote:
> >>> On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <[email protected]> wrote:
> >>>> On 8/23/2021 7:31 AM, Yongji Xie wrote:
> >>>>> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <[email protected]> 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/[email protected]/
> >>>> 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
On 8/23/2021 1:33 PM, Yongji Xie wrote:
> On Mon, Aug 23, 2021 at 5:38 PM Max Gurtovoy <[email protected]> wrote:
>>
>> On 8/23/2021 12:27 PM, Yongji Xie wrote:
>>> On Mon, Aug 23, 2021 at 5:04 PM Max Gurtovoy <[email protected]> wrote:
>>>> On 8/23/2021 11:35 AM, Yongji Xie wrote:
>>>>> On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <[email protected]> wrote:
>>>>>> On 8/23/2021 7:31 AM, Yongji Xie wrote:
>>>>>>> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <[email protected]> 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/[email protected]/
>>>>>> 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
On Mon, Aug 23, 2021 at 6:45 PM Max Gurtovoy <[email protected]> wrote:
>
>
> On 8/23/2021 1:33 PM, Yongji Xie wrote:
> > On Mon, Aug 23, 2021 at 5:38 PM Max Gurtovoy <[email protected]> wrote:
> >>
> >> On 8/23/2021 12:27 PM, Yongji Xie wrote:
> >>> On Mon, Aug 23, 2021 at 5:04 PM Max Gurtovoy <[email protected]> wrote:
> >>>> On 8/23/2021 11:35 AM, Yongji Xie wrote:
> >>>>> On Mon, Aug 23, 2021 at 4:07 PM Max Gurtovoy <[email protected]> wrote:
> >>>>>> On 8/23/2021 7:31 AM, Yongji Xie wrote:
> >>>>>>> On Mon, Aug 23, 2021 at 7:17 AM Max Gurtovoy <[email protected]> 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/[email protected]/
> >>>>>> 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/[email protected]/
[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
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
On Mon, Aug 23, 2021 at 8:13 PM Michael S. Tsirkin <[email protected]> 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
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 <[email protected]> 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
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.
>
On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <[email protected]> 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.
> >
>
On 8/24/2021 5:47 AM, Jason Wang wrote:
> On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <[email protected]> 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.
>>>
On Tue, Aug 24, 2021 at 6:11 PM Max Gurtovoy <[email protected]> wrote:
>
>
> On 8/24/2021 5:47 AM, Jason Wang wrote:
> > On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <[email protected]> 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
On 8/24/2021 3:52 PM, Yongji Xie wrote:
> On Tue, Aug 24, 2021 at 6:11 PM Max Gurtovoy <[email protected]> wrote:
>>
>> On 8/24/2021 5:47 AM, Jason Wang wrote:
>>> On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <[email protected]> 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
On Tue, Aug 24, 2021 at 9:30 PM Max Gurtovoy <[email protected]> wrote:
>
>
> On 8/24/2021 3:52 PM, Yongji Xie wrote:
> > On Tue, Aug 24, 2021 at 6:11 PM Max Gurtovoy <[email protected]> wrote:
> >>
> >> On 8/24/2021 5:47 AM, Jason Wang wrote:
> >>> On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <[email protected]> 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
On 8/24/2021 4:38 PM, Yongji Xie wrote:
> On Tue, Aug 24, 2021 at 9:30 PM Max Gurtovoy <[email protected]> wrote:
>>
>> On 8/24/2021 3:52 PM, Yongji Xie wrote:
>>> On Tue, Aug 24, 2021 at 6:11 PM Max Gurtovoy <[email protected]> wrote:
>>>> On 8/24/2021 5:47 AM, Jason Wang wrote:
>>>>> On Tue, Aug 24, 2021 at 6:31 AM Max Gurtovoy <[email protected]> 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
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 <[email protected]>
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
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 <[email protected]>
>
> 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
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 <[email protected]>
>
> 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
On Mon, Oct 4, 2021 at 11:27 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]>
>
> 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
On Tue, Oct 5, 2021 at 6:42 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]>
> >
> > 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
On Mon, Oct 4, 2021 at 11:39 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]>
> >
> > 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
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
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.
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
On Wed, Oct 13, 2021 at 8:21 PM Michael S. Tsirkin <[email protected]> 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
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 <[email protected]> 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
On Wed, Oct 13, 2021 at 8:51 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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