[PATCH v3] virtio_blk: add SECURE ERASE command support

[PATCH v3] virtio_blk: add SECURE ERASE command support

[Alvaro Karsz]

Support for the VIRTIO_BLK_F_SECURE_ERASE VirtIO feature.

A device that offers this feature can receive VIRTIO_BLK_T_SECURE_ERASE
commands.

A device which supports this feature has the following fields in the
virtio config:

- max_secure_erase_sectors
- max_secure_erase_seg
- secure_erase_sector_alignment

max_secure_erase_sectors and secure_erase_sector_alignment are expressed
in 512-byte units.

Every secure erase command has the following fields:

- sectors: The starting offset in 512-byte units.
- num_sectors: The number of sectors.

Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
---
v2:
	- Set queue max discard segments as the minimum between
	  max_secure_erase_seg and max_discard_seg.
	- Set queue discard granularity as the minimum between
	  secure_erase_sector_alignment and discard_sector_alignment.

v3:
	- Usage of min_not_zero.
	- Fail probe if any of the secure erase parameters in the virtio
	  config is 0.
	- Add a comment explaining why we use the minimum between the discard
	  and secure erase limits.
---
 drivers/block/virtio_blk.c      | 110 ++++++++++++++++++++++++++------
 include/uapi/linux/virtio_blk.h |  19 ++++++
 2 files changed, 111 insertions(+), 18 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 30255fcaf18..dd57612f4a6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -130,7 +130,7 @@ static int virtblk_add_req(struct virtqueue *vq, struct virtblk_req *vbr)
 	return virtqueue_add_sgs(vq, sgs, num_out, num_in, vbr, GFP_ATOMIC);
 }
 
-static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
+static int virtblk_setup_discard_write_zeroes_erase(struct request *req, bool unmap)
 {
 	unsigned short segments = blk_rq_nr_discard_segments(req);
 	unsigned short n = 0;
@@ -240,6 +240,9 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
 		type = VIRTIO_BLK_T_WRITE_ZEROES;
 		unmap = !(req->cmd_flags & REQ_NOUNMAP);
 		break;
+	case REQ_OP_SECURE_ERASE:
+		type = VIRTIO_BLK_T_SECURE_ERASE;
+		break;
 	case REQ_OP_DRV_IN:
 		type = VIRTIO_BLK_T_GET_ID;
 		break;
@@ -251,8 +254,9 @@ static blk_status_t virtblk_setup_cmd(struct virtio_device *vdev,
 	vbr->out_hdr.type = cpu_to_virtio32(vdev, type);
 	vbr->out_hdr.ioprio = cpu_to_virtio32(vdev, req_get_ioprio(req));
 
-	if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES) {
-		if (virtblk_setup_discard_write_zeroes(req, unmap))
+	if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES ||
+	    type == VIRTIO_BLK_T_SECURE_ERASE) {
+		if (virtblk_setup_discard_write_zeroes_erase(req, unmap))
 			return BLK_STS_RESOURCE;
 	}
 
@@ -889,6 +893,8 @@ static int virtblk_probe(struct virtio_device *vdev)
 	int err, index;
 
 	u32 v, blk_size, max_size, sg_elems, opt_io_size;
+	u32 max_discard_segs = 0;
+	u32 discard_granularity = 0;
 	u16 min_io_size;
 	u8 physical_block_exp, alignment_offset;
 	unsigned int queue_depth;
@@ -1046,27 +1052,14 @@ static int virtblk_probe(struct virtio_device *vdev)
 
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
 		virtio_cread(vdev, struct virtio_blk_config,
-			     discard_sector_alignment, &v);
-		if (v)
-			q->limits.discard_granularity = v << SECTOR_SHIFT;
-		else
-			q->limits.discard_granularity = blk_size;
+			     discard_sector_alignment, &discard_granularity);
 
 		virtio_cread(vdev, struct virtio_blk_config,
 			     max_discard_sectors, &v);
 		blk_queue_max_discard_sectors(q, v ? v : UINT_MAX);
 
 		virtio_cread(vdev, struct virtio_blk_config, max_discard_seg,
-			     &v);
-
-		/*
-		 * max_discard_seg == 0 is out of spec but we always
-		 * handled it.
-		 */
-		if (!v)
-			v = sg_elems;
-		blk_queue_max_discard_segments(q,
-					       min(v, MAX_DISCARD_SEGMENTS));
+			     &max_discard_segs);
 	}
 
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_WRITE_ZEROES)) {
@@ -1075,6 +1068,85 @@ static int virtblk_probe(struct virtio_device *vdev)
 		blk_queue_max_write_zeroes_sectors(q, v ? v : UINT_MAX);
 	}
 
+	/* The discard and secure erase limits are combined since the Linux
+	 * block layer uses the same limit for both commands.
+	 *
+	 * If both VIRTIO_BLK_F_SECURE_ERASE and VIRTIO_BLK_F_DISCARD features
+	 * are negotiated, we will use the minimum between the limits.
+	 *
+	 * discard sector alignment is set to the minimum between discard_sector_alignment
+	 * and secure_erase_sector_alignment.
+	 *
+	 * max discard sectors is set to the minimum between max_discard_seg and
+	 * max_secure_erase_seg.
+	 */
+	if (virtio_has_feature(vdev, VIRTIO_BLK_F_SECURE_ERASE)) {
+
+		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;
+		}
+
+		discard_granularity = min_not_zero(discard_granularity, v);
+
+		virtio_cread(vdev, struct virtio_blk_config,
+			     max_secure_erase_sectors, &v);
+
+		/* max_secure_erase_sectors should not be zero, the device should set a
+		 * valid number of sectors.
+		 */
+		if (!v) {
+			dev_err(&vdev->dev,
+				"virtio_blk: max_secure_erase_sectors can't be 0\n");
+			err = -EINVAL;
+			goto out_cleanup_disk;
+		}
+
+		blk_queue_max_secure_erase_sectors(q, v);
+
+		virtio_cread(vdev, struct virtio_blk_config,
+			     max_secure_erase_seg, &v);
+
+		/* max_secure_erase_seg should not be zero, the device should set a
+		 * valid number of segments
+		 */
+		if (!v) {
+			dev_err(&vdev->dev,
+				"virtio_blk: max_secure_erase_seg can't be 0\n");
+			err = -EINVAL;
+			goto out_cleanup_disk;
+		}
+
+		max_discard_segs = min_not_zero(max_discard_segs, v);
+	}
+
+	if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD) ||
+	    virtio_has_feature(vdev, VIRTIO_BLK_F_SECURE_ERASE)) {
+		/* max_discard_seg and discard_granularity will be 0 only
+		 * if max_discard_seg and discard_sector_alignment fields in the virtio
+		 * config are 0 and VIRTIO_BLK_F_SECURE_ERASE feature is not negotiated.
+		 * In this case, we use default values.
+		 */
+		if (!max_discard_segs)
+			max_discard_segs = sg_elems;
+
+		blk_queue_max_discard_segments(q,
+					       min(max_discard_segs, MAX_DISCARD_SEGMENTS));
+
+		if (discard_granularity)
+			q->limits.discard_granularity = discard_granularity << SECTOR_SHIFT;
+		else
+			q->limits.discard_granularity = blk_size;
+	}
+
 	virtblk_update_capacity(vblk, false);
 	virtio_device_ready(vdev);
 
@@ -1170,6 +1242,7 @@ static unsigned int features_legacy[] = {
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
+	VIRTIO_BLK_F_SECURE_ERASE,
 }
 ;
 static unsigned int features[] = {
@@ -1177,6 +1250,7 @@ static unsigned int features[] = {
 	VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
 	VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
 	VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
+	VIRTIO_BLK_F_SECURE_ERASE,
 };
 
 static struct virtio_driver virtio_blk = {
diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index d888f013d9f..58e70b24b50 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h
@@ -40,6 +40,7 @@
 #define VIRTIO_BLK_F_MQ		12	/* support more than one vq */
 #define VIRTIO_BLK_F_DISCARD	13	/* DISCARD is supported */
 #define VIRTIO_BLK_F_WRITE_ZEROES	14	/* WRITE ZEROES is supported */
+#define VIRTIO_BLK_F_SECURE_ERASE	16 /* Secure Erase is supported */
 
 /* Legacy feature bits */
 #ifndef VIRTIO_BLK_NO_LEGACY
@@ -121,6 +122,21 @@ struct virtio_blk_config {
 	__u8 write_zeroes_may_unmap;
 
 	__u8 unused1[3];
+
+	/* the next 3 entries are guarded by VIRTIO_BLK_F_SECURE_ERASE */
+	/*
+	 * The maximum secure erase sectors (in 512-byte sectors) for
+	 * one segment.
+	 */
+	__virtio32 max_secure_erase_sectors;
+	/*
+	 * The maximum number of secure erase segments in a
+	 * secure erase command.
+	 */
+	__virtio32 max_secure_erase_seg;
+	/* Secure erase commands must be aligned to this number of sectors. */
+	__virtio32 secure_erase_sector_alignment;
+
 } __attribute__((packed));
 
 /*
@@ -155,6 +171,9 @@ struct virtio_blk_config {
 /* Write zeroes command */
 #define VIRTIO_BLK_T_WRITE_ZEROES	13
 
+/* Secure erase command */
+#define VIRTIO_BLK_T_SECURE_ERASE	14
+
 #ifndef VIRTIO_BLK_NO_LEGACY
 /* Barrier before this op. */
 #define VIRTIO_BLK_T_BARRIER	0x80000000
-- 
2.32.0

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

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

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 1450 bytes --]

On Wed, Sep 21, 2022 at 11:27:29AM +0300, Alvaro Karsz wrote:
> Support for the VIRTIO_BLK_F_SECURE_ERASE VirtIO feature.
> 
> A device that offers this feature can receive VIRTIO_BLK_T_SECURE_ERASE
> commands.
> 
> A device which supports this feature has the following fields in the
> virtio config:
> 
> - max_secure_erase_sectors
> - max_secure_erase_seg
> - secure_erase_sector_alignment
> 
> max_secure_erase_sectors and secure_erase_sector_alignment are expressed
> in 512-byte units.
> 
> Every secure erase command has the following fields:
> 
> - sectors: The starting offset in 512-byte units.
> - num_sectors: The number of sectors.
> 
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> ---
> v2:
> 	- Set queue max discard segments as the minimum between
> 	  max_secure_erase_seg and max_discard_seg.
> 	- Set queue discard granularity as the minimum between
> 	  secure_erase_sector_alignment and discard_sector_alignment.
> 
> v3:
> 	- Usage of min_not_zero.
> 	- Fail probe if any of the secure erase parameters in the virtio
> 	  config is 0.
> 	- Add a comment explaining why we use the minimum between the discard
> 	  and secure erase limits.
> ---
>  drivers/block/virtio_blk.c      | 110 ++++++++++++++++++++++++++------
>  include/uapi/linux/virtio_blk.h |  19 ++++++
>  2 files changed, 111 insertions(+), 18 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

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

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

[Alvaro Karsz]

> What I worry about is that down the road we might want to add
> special meaning to currently unused values.
> If doing that just clears VIRTIO_BLK_F_SECURE_ERASE then
> we have forward compatibility. If it fails probe then we
> won't be able to do use these values.


They are not exactly unused, we are using them to calculate the
"discard sector alignment" and the "max discard sectors" values.
The values are even used directly if VIRTIO_BLK_F_DISCARD is not negotiated.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Wed, Sep 28, 2022 at 01:48:43PM +0300, Alvaro Karsz wrote:
> > What I worry about is that down the road we might want to add
> > special meaning to currently unused values.
> > If doing that just clears VIRTIO_BLK_F_SECURE_ERASE then
> > we have forward compatibility. If it fails probe then we
> > won't be able to do use these values.
> 
> 
> They are not exactly unused, we are using them to calculate the
> "discard sector alignment" and the "max discard sectors" values.
> The values are even used directly if VIRTIO_BLK_F_DISCARD is not negotiated.

Could you explain this last part? Why are they used without
VIRTIO_BLK_F_DISCARD?

-- 
MST

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

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

[Alvaro Karsz]

> Could you explain this last part? Why are they used without
> VIRTIO_BLK_F_DISCARD?


Sure,

If both  VIRTIO_BLK_F_DISCARD and VIRTIO_BLK_F_SECURE_ERASE are negotiated:
q->limits.max_discard_segments =
min(virtio_blk_config->max_discard_seg,
virtio_blk_config->max_secure_erase_seg)

If VIRTIO_BLK_F_DISCARD is negotiated and VIRTIO_BLK_F_SECURE_ERASE isn't
q->limits.max_discard_segments = virtio_blk_config->max_discard_seg

If VIRTIO_BLK_F_SECURE_ERASE is negotiated and VIRTIO_BLK_F_DISCARD isn't
q->limits.max_discard_segments = virtio_blk_config->max_secure_erase_seg
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Thu, Sep 29, 2022 at 10:13:47AM +0300, Alvaro Karsz wrote:
> > Could you explain this last part? Why are they used without
> > VIRTIO_BLK_F_DISCARD?
> 
> 
> Sure,
> 
> If both  VIRTIO_BLK_F_DISCARD and VIRTIO_BLK_F_SECURE_ERASE are negotiated:
> q->limits.max_discard_segments =
> min(virtio_blk_config->max_discard_seg,
> virtio_blk_config->max_secure_erase_seg)
> 
> If VIRTIO_BLK_F_DISCARD is negotiated and VIRTIO_BLK_F_SECURE_ERASE isn't
> q->limits.max_discard_segments = virtio_blk_config->max_discard_seg
> 
> If VIRTIO_BLK_F_SECURE_ERASE is negotiated and VIRTIO_BLK_F_DISCARD isn't
> q->limits.max_discard_segments = virtio_blk_config->max_secure_erase_seg

OK so virtio_blk_config->max_discard_seg is unused without
VIRTIO_BLK_F_DISCARD.

-- 
MST

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

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

[Alvaro Karsz]

> 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?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

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

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

[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.
Stefan, what do you think?
Are you ok with clearing the feature?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

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

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

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

[Jason Wang]

在 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?
>

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