Re: [PATCH v3] virtio_blk: add SECURE ERASE command support

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Sep 29, 2022 at 10:29:09AM +0300, Alvaro Karsz wrote:
> > OK so virtio_blk_config->max_discard_seg is unused without
> > VIRTIO_BLK_F_DISCARD.
> 
> 
> Yes, if I understood the spec correctly,
> virtio_blk_config->max_discard_seg is relevant if VIRTIO_BLK_F_DISCARD
> is negotiated, and virtio_blk_config->max_secure_erase_seg is relevant
> if VIRTIO_BLK_F_SECURE_ERASE is negotiated.
> 
> What should I do?
> Should I fix the patch?

I don't know. You guys are storage experts I'm a virtio guy.
And from virtio POV I have a question about this code:

+               virtio_cread(vdev, struct virtio_blk_config,
+                            secure_erase_sector_alignment, &v);
+
+               /* secure_erase_sector_alignment should not be zero, the device should set a
+                * valid number of sectors.
+                */
+               if (!v) {
+                       dev_err(&vdev->dev,
+                               "virtio_blk: secure_erase_sector_alignment can't be 0\n");
+                       err = -EINVAL;
+                       goto out_cleanup_disk;
+               }

So this will prevent us from ever exposing a device
with secure_erase_sector_alignment set to 0.
Same for max_secure_erase_sectors and max_secure_erase_seg.
What can the value 0 mean here? I don't know, maybe "get actual
value from some other place".


An alternative is to put this code in a validate
callback and clear VIRTIO_BLK_F_SECURE_ERASE.

However, this means that even if host exposes VIRTIO_BLK_F_SECURE_ERASE
the host can not be sure guest will use secure erase.
Is this or can be a security problem?
If yes let's be strict and fail probe as current code does.
If not let's be flexible and ensure forward compatibility.



-- 
MST

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