Alvaro Karsz
2022-Sep-29 07:29 UTC
[PATCH v3] virtio_blk: add SECURE ERASE command support
> 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?
Michael S. Tsirkin
2022-Sep-29 07:45 UTC
[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