Virtualization - Oct 2022 - [PATCH v3] virtio_blk: add SECURE ERASE command support

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

> 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.

I can't think of any security problems.
Stefan, what do you think?
Are you ok with clearing the feature?

On Thu, Sep 29, 2022 at 11:51:22AM +0300, Alvaro Karsz
wrote:
> > 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.
> 
> 
> I can't think of any security problems.
> Stefan, what do you think?
> Are you ok with clearing the feature?
I applied as is for now, using validate callback can be done on top.

-- 
MSR

? 2022/9/29 16:51, Alvaro Karsz ??:
>> 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.
>
> I can't think of any security problems.

Yes. And even if the device try to mandate VIRTIO_BLK_F_SECURE_ERASE, 
there's no guarantee that it has been implemented as what guest want.

Guest might need to do encryption for extra assurance.

Thanks

> Stefan, what do you think?
> Are you ok with clearing the feature?
>