security issue: data exposure when using block layer secure erase

security issue: data exposure when using block layer secure erase

[Christoph Hellwig]

Hi all,

while staring at the block layer code I found what I think is a major
security issue with the use of REQ_OP_SECURE_ERASE.

The issue is not about the actual protocol implementation, which only
exists for eMMC [1], but about we handle issuing the operation in the
block layer.  That is done through __blkdev_issue_discard, which
takes various parameters into account to align the issue discard
request to what the hardware prefers.  Which is perfectly fine for
discard as an advisory operation, but deadly for an operation that
wants to make data inaccessible.  The problem has existed ever since
secure erase support was added to the kernel with commit
8d57a98ccd0b ("block: add secure discard"), which added secure erase
support as a REQ_SECURE flag to the discard operation.

The ioctl added there also as the only users for a long time, until f2fs
added a second (really strange) user that uses secure erase if offered by 
the device but otherwise plain old discard: 9af846486d78
("f2fs: add F2FS_IOC_SEC_TRIM_FILE ioctl") which seems to treat the
secure discard as nice to have but actually is fine with data leaks
from the use of discard or an incorrect implementation of secure erase.

My preference would be to just remove this ill designed feature entirely.
The alternative 1 in this thead does just that.  Alternative 2 tries to
fix it instead, but I haven't bee nable to get any interested party to
actually test in more than three eeks, suggesting we're better off
removing the code.

[1] which is rather dubious as well, as sector based secure erase in
flash based media can't really work due to the lack of in-place write
support.  At best it is the equivalent for a Write Same or Write Zeroes
command without deterministic data on the next read.

[PATCH, alternative 1] block: remove REQ_OP_SECURE_ERASE erase

[Christoph Hellwig]

The support for this "secure erase" is completely broken, given that
the blk-lib code aligns it to the discard granularity and alignment
and thus skips parts of the two be discarded area.  There's only
two users of this feature:  f2fs just uses it by default for all
discards when supported from the F2FS_IOC_SEC_TRIM_FILE ioctl, and
the BLKSECDISCARD ioctl request it and fails if not supported.

Note that even if the rounding errors were to go away, a LBA based
"secure erase" can't actually work on flash media as it has the
same kind of rounding issues in the device due to the lack of
overwrite capabilities.  And eMMC is the only actual hardware
driver supporting it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c                    |  5 --
 block/blk-lib.c                     | 14 ++----
 block/blk-merge.c                   |  6 +--
 block/blk-mq-debugfs.c              |  1 -
 block/blk.h                         |  1 -
 block/bounce.c                      |  1 -
 block/ioctl.c                       |  3 --
 drivers/block/rnbd/rnbd-clt.c       |  2 -
 drivers/block/rnbd/rnbd-proto.h     |  6 ---
 drivers/block/rnbd/rnbd-srv-dev.h   |  5 --
 drivers/block/rnbd/rnbd-srv.c       |  3 +-
 drivers/block/xen-blkback/blkback.c |  6 +--
 drivers/block/xen-blkback/common.h  |  1 -
 drivers/block/xen-blkback/xenbus.c  |  6 +--
 drivers/block/xen-blkfront.c        | 15 ++----
 drivers/md/dm-ebs-target.c          |  1 -
 drivers/md/dm-linear.c              |  1 -
 drivers/md/dm-stripe.c              |  2 -
 drivers/md/dm-table.c               | 31 ------------
 drivers/md/dm.c                     |  4 --
 drivers/mmc/core/block.c            | 73 -----------------------------
 drivers/mmc/core/core.c             |  9 ----
 drivers/mmc/core/core.h             |  1 -
 drivers/mmc/core/queue.c            |  3 --
 fs/f2fs/file.c                      |  4 +-
 include/linux/bio.h                 |  3 --
 include/linux/blk_types.h           |  2 -
 include/linux/blkdev.h              |  7 +--
 include/linux/device-mapper.h       |  6 ---
 kernel/trace/blktrace.c             |  6 +--
 30 files changed, 13 insertions(+), 215 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 779b4a1f66ac2..dd623548ccac1 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -116,7 +116,6 @@ static const char *const blk_op_name[] = {
 	REQ_OP_NAME(WRITE),
 	REQ_OP_NAME(FLUSH),
 	REQ_OP_NAME(DISCARD),
-	REQ_OP_NAME(SECURE_ERASE),
 	REQ_OP_NAME(ZONE_RESET),
 	REQ_OP_NAME(ZONE_RESET_ALL),
 	REQ_OP_NAME(ZONE_OPEN),
@@ -728,10 +727,6 @@ noinline_for_stack bool submit_bio_checks(struct bio *bio)
 		if (!blk_queue_discard(q))
 			goto not_supported;
 		break;
-	case REQ_OP_SECURE_ERASE:
-		if (!blk_queue_secure_erase(q))
-			goto not_supported;
-		break;
 	case REQ_OP_WRITE_SAME:
 		if (!q->limits.max_write_same_sectors)
 			goto not_supported;
diff --git a/block/blk-lib.c b/block/blk-lib.c
index 9f09beadcbe30..dfcd629a8dffa 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -29,7 +29,6 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 {
 	struct request_queue *q = bdev_get_queue(bdev);
 	struct bio *bio = *biop;
-	unsigned int op;
 	sector_t bs_mask, part_offset = 0;
 
 	if (!q)
@@ -38,15 +37,8 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	if (bdev_read_only(bdev))
 		return -EPERM;
 
-	if (flags & BLKDEV_DISCARD_SECURE) {
-		if (!blk_queue_secure_erase(q))
-			return -EOPNOTSUPP;
-		op = REQ_OP_SECURE_ERASE;
-	} else {
-		if (!blk_queue_discard(q))
-			return -EOPNOTSUPP;
-		op = REQ_OP_DISCARD;
-	}
+	if (!blk_queue_discard(q))
+		return -EOPNOTSUPP;
 
 	/* In case the discard granularity isn't set by buggy device driver */
 	if (WARN_ON_ONCE(!q->limits.discard_granularity)) {
@@ -98,7 +90,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		bio = blk_next_bio(bio, 0, gfp_mask);
 		bio->bi_iter.bi_sector = sector;
 		bio_set_dev(bio, bdev);
-		bio_set_op_attrs(bio, op, 0);
+		bio_set_op_attrs(bio, REQ_OP_DISCARD, 0);
 
 		bio->bi_iter.bi_size = req_sects << 9;
 		sector += req_sects;
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 4de34a332c9fd..67c9e65a88ad2 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -344,7 +344,6 @@ void __blk_queue_split(struct request_queue *q, struct bio **bio,
 
 	switch (bio_op(*bio)) {
 	case REQ_OP_DISCARD:
-	case REQ_OP_SECURE_ERASE:
 		split = blk_bio_discard_split(q, *bio, &q->bio_split, nr_segs);
 		break;
 	case REQ_OP_WRITE_ZEROES:
@@ -405,7 +404,6 @@ unsigned int blk_recalc_rq_segments(struct request *rq)
 
 	switch (bio_op(rq->bio)) {
 	case REQ_OP_DISCARD:
-	case REQ_OP_SECURE_ERASE:
 		if (queue_max_discard_segments(rq->q) > 1) {
 			struct bio *bio = rq->bio;
 
@@ -588,9 +586,7 @@ static inline unsigned int blk_rq_get_max_sectors(struct request *rq,
 	if (blk_rq_is_passthrough(rq))
 		return q->limits.max_hw_sectors;
 
-	if (!q->limits.chunk_sectors ||
-	    req_op(rq) == REQ_OP_DISCARD ||
-	    req_op(rq) == REQ_OP_SECURE_ERASE)
+	if (!q->limits.chunk_sectors || req_op(rq) == REQ_OP_DISCARD)
 		return blk_queue_get_max_sectors(q, req_op(rq));
 
 	return min(blk_max_size_offset(q, offset, 0),
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 3a790eb4995c6..2756975f9161c 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -116,7 +116,6 @@ static const char *const blk_queue_flag_name[] = {
 	QUEUE_FLAG_NAME(DISCARD),
 	QUEUE_FLAG_NAME(NOXMERGES),
 	QUEUE_FLAG_NAME(ADD_RANDOM),
-	QUEUE_FLAG_NAME(SECERASE),
 	QUEUE_FLAG_NAME(SAME_FORCE),
 	QUEUE_FLAG_NAME(DEAD),
 	QUEUE_FLAG_NAME(INIT_DONE),
diff --git a/block/blk.h b/block/blk.h
index 8bd43b3ad33d5..59c10afc3702a 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -284,7 +284,6 @@ static inline bool blk_may_split(struct request_queue *q, struct bio *bio)
 {
 	switch (bio_op(bio)) {
 	case REQ_OP_DISCARD:
-	case REQ_OP_SECURE_ERASE:
 	case REQ_OP_WRITE_ZEROES:
 	case REQ_OP_WRITE_SAME:
 		return true; /* non-trivial splitting decisions */
diff --git a/block/bounce.c b/block/bounce.c
index 7af1a72835b99..b6ef0f254bd34 100644
--- a/block/bounce.c
+++ b/block/bounce.c
@@ -178,7 +178,6 @@ static struct bio *bounce_clone_bio(struct bio *bio_src)
 
 	switch (bio_op(bio)) {
 	case REQ_OP_DISCARD:
-	case REQ_OP_SECURE_ERASE:
 	case REQ_OP_WRITE_ZEROES:
 		break;
 	case REQ_OP_WRITE_SAME:
diff --git a/block/ioctl.c b/block/ioctl.c
index 4a86340133e46..b93bbc43d8e51 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -452,9 +452,6 @@ static int blkdev_common_ioctl(struct block_device *bdev, fmode_t mode,
 		return blkdev_roset(bdev, mode, cmd, arg);
 	case BLKDISCARD:
 		return blk_ioctl_discard(bdev, mode, arg, 0);
-	case BLKSECDISCARD:
-		return blk_ioctl_discard(bdev, mode, arg,
-				BLKDEV_DISCARD_SECURE);
 	case BLKZEROOUT:
 		return blk_ioctl_zeroout(bdev, mode, arg);
 	case BLKGETDISKSEQ:
diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c
index c08971de369fc..8f4dd2216500d 100644
--- a/drivers/block/rnbd/rnbd-clt.c
+++ b/drivers/block/rnbd/rnbd-clt.c
@@ -1373,8 +1373,6 @@ static void setup_request_queue(struct rnbd_clt_dev *dev)
 	dev->queue->limits.discard_alignment	= dev->discard_alignment;
 	if (dev->max_discard_sectors)
 		blk_queue_flag_set(QUEUE_FLAG_DISCARD, dev->queue);
-	if (dev->secure_discard)
-		blk_queue_flag_set(QUEUE_FLAG_SECERASE, dev->queue);
 
 	blk_queue_flag_set(QUEUE_FLAG_SAME_COMP, dev->queue);
 	blk_queue_flag_set(QUEUE_FLAG_SAME_FORCE, dev->queue);
diff --git a/drivers/block/rnbd/rnbd-proto.h b/drivers/block/rnbd/rnbd-proto.h
index de5d5a8df81d7..b791338d88d9d 100644
--- a/drivers/block/rnbd/rnbd-proto.h
+++ b/drivers/block/rnbd/rnbd-proto.h
@@ -246,9 +246,6 @@ static inline u32 rnbd_to_bio_flags(u32 rnbd_opf)
 	case RNBD_OP_DISCARD:
 		bio_opf = REQ_OP_DISCARD;
 		break;
-	case RNBD_OP_SECURE_ERASE:
-		bio_opf = REQ_OP_SECURE_ERASE;
-		break;
 	case RNBD_OP_WRITE_SAME:
 		bio_opf = REQ_OP_WRITE_SAME;
 		break;
@@ -281,9 +278,6 @@ static inline u32 rq_to_rnbd_flags(struct request *rq)
 	case REQ_OP_DISCARD:
 		rnbd_opf = RNBD_OP_DISCARD;
 		break;
-	case REQ_OP_SECURE_ERASE:
-		rnbd_opf = RNBD_OP_SECURE_ERASE;
-		break;
 	case REQ_OP_WRITE_SAME:
 		rnbd_opf = RNBD_OP_WRITE_SAME;
 		break;
diff --git a/drivers/block/rnbd/rnbd-srv-dev.h b/drivers/block/rnbd/rnbd-srv-dev.h
index 0eb23850afb95..dcfb4970abeea 100644
--- a/drivers/block/rnbd/rnbd-srv-dev.h
+++ b/drivers/block/rnbd/rnbd-srv-dev.h
@@ -56,11 +56,6 @@ static inline int rnbd_dev_get_max_hw_sects(const struct rnbd_dev *dev)
 	return queue_max_hw_sectors(bdev_get_queue(dev->bdev));
 }
 
-static inline int rnbd_dev_get_secure_discard(const struct rnbd_dev *dev)
-{
-	return blk_queue_secure_erase(bdev_get_queue(dev->bdev));
-}
-
 static inline int rnbd_dev_get_max_discard_sects(const struct rnbd_dev *dev)
 {
 	if (!blk_queue_discard(bdev_get_queue(dev->bdev)))
diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
index 1ee808fc600cf..0300cf1290810 100644
--- a/drivers/block/rnbd/rnbd-srv.c
+++ b/drivers/block/rnbd/rnbd-srv.c
@@ -566,8 +566,7 @@ static void rnbd_srv_fill_msg_open_rsp(struct rnbd_msg_open_rsp *rsp,
 		cpu_to_le32(rnbd_dev_get_discard_granularity(rnbd_dev));
 	rsp->discard_alignment =
 		cpu_to_le32(rnbd_dev_get_discard_alignment(rnbd_dev));
-	rsp->secure_discard =
-		cpu_to_le16(rnbd_dev_get_secure_discard(rnbd_dev));
+	rsp->secure_discard = 0;
 	rsp->rotational = !blk_queue_nonrot(q);
 	rsp->cache_policy = 0;
 	if (test_bit(QUEUE_FLAG_WC, &q->queue_flags))
diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 14e452896d04c..fb9e0a16cfc65 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -987,13 +987,9 @@ static int dispatch_discard_io(struct xen_blkif_ring *ring,
 	}
 	ring->st_ds_req++;
 
-	secure = (blkif->vbd.discard_secure &&
-		 (req->u.discard.flag & BLKIF_DISCARD_SECURE)) ?
-		 BLKDEV_DISCARD_SECURE : 0;
-
 	err = blkdev_issue_discard(bdev, req->u.discard.sector_number,
 				   req->u.discard.nr_sectors,
-				   GFP_KERNEL, secure);
+				   GFP_KERNEL, 0);
 fail_response:
 	if (err == -EOPNOTSUPP) {
 		pr_debug("discard op failed, not supported\n");
diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
index bda5c815e4415..f732d0135f45a 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -225,7 +225,6 @@ struct xen_vbd {
 	/* Cached size parameter. */
 	sector_t		size;
 	unsigned int		flush_support:1;
-	unsigned int		discard_secure:1;
 	unsigned int		feature_gnt_persistent:1;
 	unsigned int		overflow_max_grants:1;
 };
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 62125fd4af4a7..3181a36592192 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -519,9 +519,6 @@ static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
 	if (q && test_bit(QUEUE_FLAG_WC, &q->queue_flags))
 		vbd->flush_support = true;
 
-	if (q && blk_queue_secure_erase(q))
-		vbd->discard_secure = true;
-
 	vbd->feature_gnt_persistent = feature_persistent;
 
 	pr_debug("Successful creation of handle=%04x (dom=%u)\n",
@@ -600,8 +597,7 @@ static void xen_blkbk_discard(struct xenbus_transaction xbt, struct backend_info
 		state = 1;
 		/* Optional. */
 		err = xenbus_printf(xbt, dev->nodename,
-				    "discard-secure", "%d",
-				    blkif->vbd.discard_secure);
+				    "discard-secure", "%d", 0);
 		if (err) {
 			dev_warn(&dev->dev, "writing discard-secure (%d)", err);
 			return;
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index ca71a0585333f..5daac78b09f11 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -557,10 +557,7 @@ static int blkif_queue_discard_req(struct request *req, struct blkfront_ring_inf
 	ring_req->u.discard.nr_sectors = blk_rq_sectors(req);
 	ring_req->u.discard.id = id;
 	ring_req->u.discard.sector_number = (blkif_sector_t)blk_rq_pos(req);
-	if (req_op(req) == REQ_OP_SECURE_ERASE && info->feature_secdiscard)
-		ring_req->u.discard.flag = BLKIF_DISCARD_SECURE;
-	else
-		ring_req->u.discard.flag = 0;
+	ring_req->u.discard.flag = 0;
 
 	/* Copy the request to the ring page. */
 	*final_ring_req = *ring_req;
@@ -863,11 +860,9 @@ static int blkif_queue_request(struct request *req, struct blkfront_ring_info *r
 	if (unlikely(rinfo->dev_info->connected != BLKIF_STATE_CONNECTED))
 		return 1;
 
-	if (unlikely(req_op(req) == REQ_OP_DISCARD ||
-		     req_op(req) == REQ_OP_SECURE_ERASE))
+	if (unlikely(req_op(req) == REQ_OP_DISCARD))
 		return blkif_queue_discard_req(req, rinfo);
-	else
-		return blkif_queue_rw_req(req, rinfo);
+	return blkif_queue_rw_req(req, rinfo);
 }
 
 static inline void flush_requests(struct blkfront_ring_info *rinfo)
@@ -949,8 +944,6 @@ static void blkif_set_queue_limits(struct blkfront_info *info)
 		rq->limits.discard_granularity = info->discard_granularity ?:
 						 info->physical_sector_size;
 		rq->limits.discard_alignment = info->discard_alignment;
-		if (info->feature_secdiscard)
-			blk_queue_flag_set(QUEUE_FLAG_SECERASE, rq);
 	}
 
 	/* Hard sector size and max sectors impersonate the equiv. hardware. */
@@ -1594,7 +1587,6 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 				info->feature_discard = 0;
 				info->feature_secdiscard = 0;
 				blk_queue_flag_clear(QUEUE_FLAG_DISCARD, rq);
-				blk_queue_flag_clear(QUEUE_FLAG_SECERASE, rq);
 			}
 			break;
 		case BLKIF_OP_FLUSH_DISKCACHE:
@@ -2080,7 +2072,6 @@ static int blkfront_resume(struct xenbus_device *dev)
 			 */
 			if (req_op(shadow[j].request) == REQ_OP_FLUSH ||
 			    req_op(shadow[j].request) == REQ_OP_DISCARD ||
-			    req_op(shadow[j].request) == REQ_OP_SECURE_ERASE ||
 			    shadow[j].request->cmd_flags & REQ_FUA) {
 				/*
 				 * Flush operations don't contain bios, so
diff --git a/drivers/md/dm-ebs-target.c b/drivers/md/dm-ebs-target.c
index 7ce5d509b9403..be726c119c60e 100644
--- a/drivers/md/dm-ebs-target.c
+++ b/drivers/md/dm-ebs-target.c
@@ -334,7 +334,6 @@ static int ebs_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 	ti->num_flush_bios = 1;
 	ti->num_discard_bios = 1;
-	ti->num_secure_erase_bios = 0;
 	ti->num_write_same_bios = 0;
 	ti->num_write_zeroes_bios = 0;
 	return 0;
diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c
index 1b97a11d71517..44f3ed19d22c8 100644
--- a/drivers/md/dm-linear.c
+++ b/drivers/md/dm-linear.c
@@ -59,7 +59,6 @@ static int linear_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 	ti->num_flush_bios = 1;
 	ti->num_discard_bios = 1;
-	ti->num_secure_erase_bios = 1;
 	ti->num_write_same_bios = 1;
 	ti->num_write_zeroes_bios = 1;
 	ti->private = lc;
diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
index e566115ec0bb8..fa44a732c9109 100644
--- a/drivers/md/dm-stripe.c
+++ b/drivers/md/dm-stripe.c
@@ -156,7 +156,6 @@ static int stripe_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 	ti->num_flush_bios = stripes;
 	ti->num_discard_bios = stripes;
-	ti->num_secure_erase_bios = stripes;
 	ti->num_write_same_bios = stripes;
 	ti->num_write_zeroes_bios = stripes;
 
@@ -283,7 +282,6 @@ static int stripe_map(struct dm_target *ti, struct bio *bio)
 		return DM_MAPIO_REMAPPED;
 	}
 	if (unlikely(bio_op(bio) == REQ_OP_DISCARD) ||
-	    unlikely(bio_op(bio) == REQ_OP_SECURE_ERASE) ||
 	    unlikely(bio_op(bio) == REQ_OP_WRITE_ZEROES) ||
 	    unlikely(bio_op(bio) == REQ_OP_WRITE_SAME)) {
 		target_bio_nr = dm_bio_get_target_bio_nr(bio);
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index e43096cfe9e22..20e5c3772037b 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1936,34 +1936,6 @@ static bool dm_table_supports_discards(struct dm_table *t)
 	return true;
 }
 
-static int device_not_secure_erase_capable(struct dm_target *ti,
-					   struct dm_dev *dev, sector_t start,
-					   sector_t len, void *data)
-{
-	struct request_queue *q = bdev_get_queue(dev->bdev);
-
-	return !blk_queue_secure_erase(q);
-}
-
-static bool dm_table_supports_secure_erase(struct dm_table *t)
-{
-	struct dm_target *ti;
-	unsigned int i;
-
-	for (i = 0; i < dm_table_get_num_targets(t); i++) {
-		ti = dm_table_get_target(t, i);
-
-		if (!ti->num_secure_erase_bios)
-			return false;
-
-		if (!ti->type->iterate_devices ||
-		    ti->type->iterate_devices(ti, device_not_secure_erase_capable, NULL))
-			return false;
-	}
-
-	return true;
-}
-
 static int device_requires_stable_pages(struct dm_target *ti,
 					struct dm_dev *dev, sector_t start,
 					sector_t len, void *data)
@@ -2000,9 +1972,6 @@ int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	} else
 		blk_queue_flag_set(QUEUE_FLAG_DISCARD, q);
 
-	if (dm_table_supports_secure_erase(t))
-		blk_queue_flag_set(QUEUE_FLAG_SECERASE, q);
-
 	if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_WC))) {
 		wc = true;
 		if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_FUA)))
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 997ace47bbd54..6325f7907a643 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1369,7 +1369,6 @@ static bool is_abnormal_io(struct bio *bio)
 
 	switch (bio_op(bio)) {
 	case REQ_OP_DISCARD:
-	case REQ_OP_SECURE_ERASE:
 	case REQ_OP_WRITE_SAME:
 	case REQ_OP_WRITE_ZEROES:
 		r = true;
@@ -1389,9 +1388,6 @@ static bool __process_abnormal_io(struct clone_info *ci, struct dm_target *ti,
 	case REQ_OP_DISCARD:
 		num_bios = ti->num_discard_bios;
 		break;
-	case REQ_OP_SECURE_ERASE:
-		num_bios = ti->num_secure_erase_bios;
-		break;
 	case REQ_OP_WRITE_SAME:
 		num_bios = ti->num_write_same_bios;
 		break;
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 4e61b28a002f6..7c5db18c8a8c7 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1127,76 +1127,6 @@ static void mmc_blk_issue_discard_rq(struct mmc_queue *mq, struct request *req)
 	blk_mq_end_request(req, status);
 }
 
-static void mmc_blk_issue_secdiscard_rq(struct mmc_queue *mq,
-				       struct request *req)
-{
-	struct mmc_blk_data *md = mq->blkdata;
-	struct mmc_card *card = md->queue.card;
-	unsigned int from, nr, arg;
-	int err = 0, type = MMC_BLK_SECDISCARD;
-	blk_status_t status = BLK_STS_OK;
-
-	if (!(mmc_can_secure_erase_trim(card))) {
-		status = BLK_STS_NOTSUPP;
-		goto out;
-	}
-
-	from = blk_rq_pos(req);
-	nr = blk_rq_sectors(req);
-
-	if (mmc_can_trim(card) && !mmc_erase_group_aligned(card, from, nr))
-		arg = MMC_SECURE_TRIM1_ARG;
-	else
-		arg = MMC_SECURE_ERASE_ARG;
-
-retry:
-	if (card->quirks & MMC_QUIRK_INAND_CMD38) {
-		err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-				 INAND_CMD38_ARG_EXT_CSD,
-				 arg == MMC_SECURE_TRIM1_ARG ?
-				 INAND_CMD38_ARG_SECTRIM1 :
-				 INAND_CMD38_ARG_SECERASE,
-				 card->ext_csd.generic_cmd6_time);
-		if (err)
-			goto out_retry;
-	}
-
-	err = mmc_erase(card, from, nr, arg);
-	if (err == -EIO)
-		goto out_retry;
-	if (err) {
-		status = BLK_STS_IOERR;
-		goto out;
-	}
-
-	if (arg == MMC_SECURE_TRIM1_ARG) {
-		if (card->quirks & MMC_QUIRK_INAND_CMD38) {
-			err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-					 INAND_CMD38_ARG_EXT_CSD,
-					 INAND_CMD38_ARG_SECTRIM2,
-					 card->ext_csd.generic_cmd6_time);
-			if (err)
-				goto out_retry;
-		}
-
-		err = mmc_erase(card, from, nr, MMC_SECURE_TRIM2_ARG);
-		if (err == -EIO)
-			goto out_retry;
-		if (err) {
-			status = BLK_STS_IOERR;
-			goto out;
-		}
-	}
-
-out_retry:
-	if (err && !mmc_blk_reset(md, card->host, type))
-		goto retry;
-	if (!err)
-		mmc_blk_reset_success(md, type);
-out:
-	blk_mq_end_request(req, status);
-}
-
 static void mmc_blk_issue_flush(struct mmc_queue *mq, struct request *req)
 {
 	struct mmc_blk_data *md = mq->blkdata;
@@ -2292,9 +2222,6 @@ enum mmc_issued mmc_blk_mq_issue_rq(struct mmc_queue *mq, struct request *req)
 		case REQ_OP_DISCARD:
 			mmc_blk_issue_discard_rq(mq, req);
 			break;
-		case REQ_OP_SECURE_ERASE:
-			mmc_blk_issue_secdiscard_rq(mq, req);
-			break;
 		case REQ_OP_FLUSH:
 			mmc_blk_issue_flush(mq, req);
 			break;
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 368f10405e132..11276b0186dd4 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1837,15 +1837,6 @@ int mmc_can_sanitize(struct mmc_card *card)
 	return 0;
 }
 
-int mmc_can_secure_erase_trim(struct mmc_card *card)
-{
-	if ((card->ext_csd.sec_feature_support & EXT_CSD_SEC_ER_EN) &&
-	    !(card->quirks & MMC_QUIRK_SEC_ERASE_TRIM_BROKEN))
-		return 1;
-	return 0;
-}
-EXPORT_SYMBOL(mmc_can_secure_erase_trim);
-
 int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
 			    unsigned int nr)
 {
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index f5f3f623ea492..14c3ae1e36ff1 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -107,7 +107,6 @@ int mmc_can_erase(struct mmc_card *card);
 int mmc_can_trim(struct mmc_card *card);
 int mmc_can_discard(struct mmc_card *card);
 int mmc_can_sanitize(struct mmc_card *card);
-int mmc_can_secure_erase_trim(struct mmc_card *card);
 int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
 			unsigned int nr);
 unsigned int mmc_calc_max_discard(struct mmc_card *card);
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index c69b2d9df6f16..d32d3b8764847 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -47,7 +47,6 @@ static enum mmc_issue_type mmc_cqe_issue_type(struct mmc_host *host,
 	case REQ_OP_DRV_IN:
 	case REQ_OP_DRV_OUT:
 	case REQ_OP_DISCARD:
-	case REQ_OP_SECURE_ERASE:
 		return MMC_ISSUE_SYNC;
 	case REQ_OP_FLUSH:
 		return mmc_cqe_can_dcmd(host) ? MMC_ISSUE_DCMD : MMC_ISSUE_SYNC;
@@ -189,8 +188,6 @@ static void mmc_queue_setup_discard(struct request_queue *q,
 	/* granularity must not be greater than max. discard */
 	if (card->pref_erase > max_discard)
 		q->limits.discard_granularity = SECTOR_SIZE;
-	if (mmc_can_secure_erase_trim(card))
-		blk_queue_flag_set(QUEUE_FLAG_SECERASE, q);
 }
 
 static unsigned short mmc_get_max_segments(struct mmc_host *host)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 3c98ef6af97d1..825d5512875c5 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3692,9 +3692,7 @@ static int f2fs_secure_erase(struct block_device *bdev, struct inode *inode,
 		return -ENXIO;
 
 	if (flags & F2FS_TRIM_FILE_DISCARD)
-		ret = blkdev_issue_discard(bdev, sector, nr_sects, GFP_NOFS,
-						blk_queue_secure_erase(q) ?
-						BLKDEV_DISCARD_SECURE : 0);
+		ret = blkdev_issue_discard(bdev, sector, nr_sects, GFP_NOFS, 0);
 
 	if (!ret && (flags & F2FS_TRIM_FILE_ZEROOUT)) {
 		if (IS_ENCRYPTED(inode))
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 117d7f248ac96..68fd79c45e2c0 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -54,7 +54,6 @@ static inline bool bio_has_data(struct bio *bio)
 	if (bio &&
 	    bio->bi_iter.bi_size &&
 	    bio_op(bio) != REQ_OP_DISCARD &&
-	    bio_op(bio) != REQ_OP_SECURE_ERASE &&
 	    bio_op(bio) != REQ_OP_WRITE_ZEROES)
 		return true;
 
@@ -64,7 +63,6 @@ static inline bool bio_has_data(struct bio *bio)
 static inline bool bio_no_advance_iter(const struct bio *bio)
 {
 	return bio_op(bio) == REQ_OP_DISCARD ||
-	       bio_op(bio) == REQ_OP_SECURE_ERASE ||
 	       bio_op(bio) == REQ_OP_WRITE_SAME ||
 	       bio_op(bio) == REQ_OP_WRITE_ZEROES;
 }
@@ -183,7 +181,6 @@ static inline unsigned bio_segments(struct bio *bio)
 
 	switch (bio_op(bio)) {
 	case REQ_OP_DISCARD:
-	case REQ_OP_SECURE_ERASE:
 	case REQ_OP_WRITE_ZEROES:
 		return 0;
 	case REQ_OP_WRITE_SAME:
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index fe065c394fff6..a2983f77f3532 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -352,8 +352,6 @@ enum req_opf {
 	REQ_OP_FLUSH		= 2,
 	/* discard sectors */
 	REQ_OP_DISCARD		= 3,
-	/* securely erase sectors */
-	REQ_OP_SECURE_ERASE	= 5,
 	/* write the same sector many times */
 	REQ_OP_WRITE_SAME	= 7,
 	/* write the zero filled sector many times */
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 16b47035e4b06..505a11f9a03bd 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -395,7 +395,6 @@ struct request_queue {
 #define QUEUE_FLAG_DISCARD	8	/* supports DISCARD */
 #define QUEUE_FLAG_NOXMERGES	9	/* No extended merges */
 #define QUEUE_FLAG_ADD_RANDOM	10	/* Contributes to random pool */
-#define QUEUE_FLAG_SECERASE	11	/* supports secure erase */
 #define QUEUE_FLAG_SAME_FORCE	12	/* force complete on same CPU */
 #define QUEUE_FLAG_DEAD		13	/* queue tear-down finished */
 #define QUEUE_FLAG_INIT_DONE	14	/* queue is initialized */
@@ -437,8 +436,6 @@ bool blk_queue_flag_test_and_set(unsigned int flag, struct request_queue *q);
 #define blk_queue_discard(q)	test_bit(QUEUE_FLAG_DISCARD, &(q)->queue_flags)
 #define blk_queue_zone_resetall(q)	\
 	test_bit(QUEUE_FLAG_ZONE_RESETALL, &(q)->queue_flags)
-#define blk_queue_secure_erase(q) \
-	(test_bit(QUEUE_FLAG_SECERASE, &(q)->queue_flags))
 #define blk_queue_dax(q)	test_bit(QUEUE_FLAG_DAX, &(q)->queue_flags)
 #define blk_queue_pci_p2pdma(q)	\
 	test_bit(QUEUE_FLAG_PCI_P2PDMA, &(q)->queue_flags)
@@ -647,7 +644,7 @@ static inline unsigned int bio_zone_is_seq(struct bio *bio)
 static inline unsigned int blk_queue_get_max_sectors(struct request_queue *q,
 						     int op)
 {
-	if (unlikely(op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE))
+	if (unlikely(op == REQ_OP_DISCARD))
 		return min(q->limits.max_discard_sectors,
 			   UINT_MAX >> SECTOR_SHIFT);
 
@@ -846,8 +843,6 @@ extern void blk_io_schedule(void);
 extern int blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, struct page *page);
 
-#define BLKDEV_DISCARD_SECURE	(1 << 0)	/* issue a secure erase */
-
 extern int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, unsigned long flags);
 extern int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index b26fecf6c8e87..a6347ca31594e 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -310,12 +310,6 @@ struct dm_target {
 	 */
 	unsigned num_discard_bios;
 
-	/*
-	 * The number of secure erase bios that will be submitted to the target.
-	 * The bio number can be accessed with dm_bio_get_target_bio_nr.
-	 */
-	unsigned num_secure_erase_bios;
-
 	/*
 	 * The number of WRITE SAME bios that will be submitted to the target.
 	 * The bio number can be accessed with dm_bio_get_target_bio_nr.
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 21dea90eaa935..546c9e00d67a3 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -237,7 +237,7 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
 	what |= MASK_TC_BIT(op_flags, META);
 	what |= MASK_TC_BIT(op_flags, PREFLUSH);
 	what |= MASK_TC_BIT(op_flags, FUA);
-	if (op == REQ_OP_DISCARD || op == REQ_OP_SECURE_ERASE)
+	if (op == REQ_OP_DISCARD)
 		what |= BLK_TC_ACT(BLK_TC_DISCARD);
 	if (op == REQ_OP_FLUSH)
 		what |= BLK_TC_ACT(BLK_TC_FLUSH);
@@ -1908,10 +1908,6 @@ void blk_fill_rwbs(char *rwbs, unsigned int op)
 	case REQ_OP_DISCARD:
 		rwbs[i++] = 'D';
 		break;
-	case REQ_OP_SECURE_ERASE:
-		rwbs[i++] = 'D';
-		rwbs[i++] = 'E';
-		break;
 	case REQ_OP_FLUSH:
 		rwbs[i++] = 'F';
 		break;
-- 
2.30.2

[PATCH alternative 2] block: fix the REQ_OP_SECURE_ERASE handling to not leak erased data

[Christoph Hellwig]

The support for this "secure erase" is completely broken, given that
the blk-lib code aligns it to the discard granularity and alignment
and thus skips parts of the two be discarded area, leaking plenty of
securely erased data.  Fix this by adding a new blkdev_secure_erase
helper instead.

Note that even if with these rounding errors fixed, a LBA based
"secure erase" can't actually work on flash media.  As flash media
requires erase cycles before writing instead of overwrites there
usually will be copied of this data left somewhere on the media.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-lib.c                     | 55 +++++++++++++++++++++++------
 block/ioctl.c                       | 43 +++++++++++++++++-----
 drivers/block/xen-blkback/blkback.c | 15 ++++----
 fs/f2fs/file.c                      |  9 ++---
 include/linux/blkdev.h              |  4 +--
 5 files changed, 95 insertions(+), 31 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 9f09beadcbe30..5fc2c0bf5c940 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -29,7 +29,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 {
 	struct request_queue *q = bdev_get_queue(bdev);
 	struct bio *bio = *biop;
-	unsigned int op;
+	unsigned int op = REQ_OP_DISCARD;
 	sector_t bs_mask, part_offset = 0;
 
 	if (!q)
@@ -38,15 +38,8 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	if (bdev_read_only(bdev))
 		return -EPERM;
 
-	if (flags & BLKDEV_DISCARD_SECURE) {
-		if (!blk_queue_secure_erase(q))
-			return -EOPNOTSUPP;
-		op = REQ_OP_SECURE_ERASE;
-	} else {
-		if (!blk_queue_discard(q))
-			return -EOPNOTSUPP;
-		op = REQ_OP_DISCARD;
-	}
+	if (!blk_queue_discard(q))
+		return -EOPNOTSUPP;
 
 	/* In case the discard granularity isn't set by buggy device driver */
 	if (WARN_ON_ONCE(!q->limits.discard_granularity)) {
@@ -440,3 +433,45 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
 	return ret;
 }
 EXPORT_SYMBOL(blkdev_issue_zeroout);
+
+int blkdev_issue_secure_erase(struct block_device *bdev, sector_t sector,
+		sector_t nr_sects, gfp_t gfp)
+{
+	sector_t bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
+	unsigned int max_sectors =
+		bdev_get_queue(bdev)->limits.max_discard_sectors;
+	struct bio *bio = NULL;
+	struct blk_plug plug;
+	int ret = 0;
+
+	if (max_sectors == 0)
+		return -EOPNOTSUPP;
+	if ((sector | nr_sects) & bs_mask)
+		return -EINVAL;
+	if (bdev_read_only(bdev))
+		return -EPERM;
+
+	blk_start_plug(&plug);
+	for (;;) {
+		unsigned int len = min_t(sector_t, nr_sects, max_sectors);
+
+		bio = blk_next_bio(bio, 0, gfp);
+		bio_set_dev(bio, bdev);
+		bio->bi_opf = REQ_OP_SECURE_ERASE;
+		bio->bi_iter.bi_sector = sector;
+		bio->bi_iter.bi_size = len;
+
+		sector += len << SECTOR_SHIFT;
+		nr_sects -= len << SECTOR_SHIFT;
+		if (!nr_sects) {
+			ret = submit_bio_wait(bio);
+			bio_put(bio);
+			break;
+		}
+		cond_resched();
+	}
+	blk_finish_plug(&plug);
+
+	return ret;
+}
+EXPORT_SYMBOL(blkdev_issue_secure_erase);
diff --git a/block/ioctl.c b/block/ioctl.c
index 4a86340133e46..0821142f921d7 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -83,7 +83,7 @@ static int compat_blkpg_ioctl(struct block_device *bdev,
 #endif
 
 static int blk_ioctl_discard(struct block_device *bdev, fmode_t mode,
-		unsigned long arg, unsigned long flags)
+		unsigned long arg)
 {
 	uint64_t range[2];
 	uint64_t start, len;
@@ -115,15 +115,43 @@ static int blk_ioctl_discard(struct block_device *bdev, fmode_t mode,
 	err = truncate_bdev_range(bdev, mode, start, start + len - 1);
 	if (err)
 		goto fail;
-
-	err = blkdev_issue_discard(bdev, start >> 9, len >> 9,
-				   GFP_KERNEL, flags);
-
+	err = blkdev_issue_discard(bdev, start >> 9, len >> 9, GFP_KERNEL, 0);
 fail:
 	filemap_invalidate_unlock(inode->i_mapping);
 	return err;
 }
 
+static int blk_ioctl_secure_erase(struct block_device *bdev, fmode_t mode,
+		void __user *argp)
+{
+	uint64_t start, len;
+	uint64_t range[2];
+	int err;
+
+	if (!(mode & FMODE_WRITE))
+		return -EBADF;
+	if (!blk_queue_discard(bdev_get_queue(bdev)))
+		return -EOPNOTSUPP;
+	if (copy_from_user(range, argp, sizeof(range)))
+		return -EFAULT;
+
+	start = range[0];
+	len = range[1];
+	if ((start & 511) || (len & 511))
+		return -EINVAL;
+	if (start + len > bdev_nr_bytes(bdev))
+		return -EINVAL;
+
+	filemap_invalidate_lock(bdev->bd_inode->i_mapping);
+	err = truncate_bdev_range(bdev, mode, start, start + len - 1);
+	if (!err)
+		err = blkdev_issue_secure_erase(bdev, start >> 9, len >> 9,
+						GFP_KERNEL);
+	filemap_invalidate_unlock(bdev->bd_inode->i_mapping);
+	return err;
+}
+
+
 static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
 		unsigned long arg)
 {
@@ -451,10 +479,9 @@ static int blkdev_common_ioctl(struct block_device *bdev, fmode_t mode,
 	case BLKROSET:
 		return blkdev_roset(bdev, mode, cmd, arg);
 	case BLKDISCARD:
-		return blk_ioctl_discard(bdev, mode, arg, 0);
+		return blk_ioctl_discard(bdev, mode, arg);
 	case BLKSECDISCARD:
-		return blk_ioctl_discard(bdev, mode, arg,
-				BLKDEV_DISCARD_SECURE);
+		return blk_ioctl_secure_erase(bdev, mode, argp);
 	case BLKZEROOUT:
 		return blk_ioctl_zeroout(bdev, mode, arg);
 	case BLKGETDISKSEQ:
diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 14e452896d04c..12f741068bcdf 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -970,7 +970,6 @@ static int dispatch_discard_io(struct xen_blkif_ring *ring,
 	int status = BLKIF_RSP_OKAY;
 	struct xen_blkif *blkif = ring->blkif;
 	struct block_device *bdev = blkif->vbd.bdev;
-	unsigned long secure;
 	struct phys_req preq;
 
 	xen_blkif_get(blkif);
@@ -987,13 +986,15 @@ static int dispatch_discard_io(struct xen_blkif_ring *ring,
 	}
 	ring->st_ds_req++;
 
-	secure = (blkif->vbd.discard_secure &&
-		 (req->u.discard.flag & BLKIF_DISCARD_SECURE)) ?
-		 BLKDEV_DISCARD_SECURE : 0;
+	if (blkif->vbd.discard_secure &&
+	    (req->u.discard.flag & BLKIF_DISCARD_SECURE))
+		err = blkdev_issue_secure_erase(bdev,
+				req->u.discard.sector_number,
+				req->u.discard.nr_sectors, GFP_KERNEL);
+	else
+		err = blkdev_issue_discard(bdev, req->u.discard.sector_number,
+				req->u.discard.nr_sectors, GFP_KERNEL, 0);
 
-	err = blkdev_issue_discard(bdev, req->u.discard.sector_number,
-				   req->u.discard.nr_sectors,
-				   GFP_KERNEL, secure);
 fail_response:
 	if (err == -EOPNOTSUPP) {
 		pr_debug("discard op failed, not supported\n");
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 3c98ef6af97d1..a83548ad7171f 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -3691,10 +3691,11 @@ static int f2fs_secure_erase(struct block_device *bdev, struct inode *inode,
 	if (!q)
 		return -ENXIO;
 
-	if (flags & F2FS_TRIM_FILE_DISCARD)
-		ret = blkdev_issue_discard(bdev, sector, nr_sects, GFP_NOFS,
-						blk_queue_secure_erase(q) ?
-						BLKDEV_DISCARD_SECURE : 0);
+	if ((flags & F2FS_TRIM_FILE_DISCARD) && blk_queue_secure_erase(q))
+		ret = blkdev_issue_secure_erase(bdev, sector, nr_sects,
+						GFP_NOFS);
+	else if (flags & F2FS_TRIM_FILE_DISCARD)
+		ret = blkdev_issue_discard(bdev, sector, nr_sects, GFP_NOFS, 0);
 
 	if (!ret && (flags & F2FS_TRIM_FILE_ZEROOUT)) {
 		if (IS_ENCRYPTED(inode))
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 16b47035e4b06..6cfc60090b119 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -846,13 +846,13 @@ extern void blk_io_schedule(void);
 extern int blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, struct page *page);
 
-#define BLKDEV_DISCARD_SECURE	(1 << 0)	/* issue a secure erase */
-
 extern int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, unsigned long flags);
 extern int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, int flags,
 		struct bio **biop);
+int blkdev_issue_secure_erase(struct block_device *bdev, sector_t sector,
+		sector_t nr_sects, gfp_t gfp);
 
 #define BLKDEV_ZERO_NOUNMAP	(1 << 0)  /* do not free blocks */
 #define BLKDEV_ZERO_NOFALLBACK	(1 << 1)  /* don't write explicit zeroes */
-- 
2.30.2

Re: security issue: data exposure when using block layer secure erase

[Eric Biggers]

On Wed, Mar 16, 2022 at 10:37:40AM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> while staring at the block layer code I found what I think is a major
> security issue with the use of REQ_OP_SECURE_ERASE.
> 
> The issue is not about the actual protocol implementation, which only
> exists for eMMC [1], but about we handle issuing the operation in the
> block layer.  That is done through __blkdev_issue_discard, which
> takes various parameters into account to align the issue discard
> request to what the hardware prefers.  Which is perfectly fine for
> discard as an advisory operation, but deadly for an operation that
> wants to make data inaccessible.  The problem has existed ever since
> secure erase support was added to the kernel with commit
> 8d57a98ccd0b ("block: add secure discard"), which added secure erase
> support as a REQ_SECURE flag to the discard operation.

__blkdev_issue_discard() can break up the region into multiple bios, but I don't
see where it actually skips parts of the region.  Can you explain more
specifically where the problem is?

- Eric

Re: [PATCH alternative 2] block: fix the REQ_OP_SECURE_ERASE handling to not leak erased data

[Ulf Hansson]

On Wed, 16 Mar 2022 at 10:38, Christoph Hellwig <hch@lst.de> wrote:
>
> The support for this "secure erase" is completely broken, given that
> the blk-lib code aligns it to the discard granularity and alignment
> and thus skips parts of the two be discarded area, leaking plenty of
> securely erased data.  Fix this by adding a new blkdev_secure_erase
> helper instead.
>
> Note that even if with these rounding errors fixed, a LBA based
> "secure erase" can't actually work on flash media.  As flash media
> requires erase cycles before writing instead of overwrites there
> usually will be copied of this data left somewhere on the media.

Stating that it can't work is probably not a correct statement.
Certainly it can, but it depends on how "secure" (or clever) the
implementation of the FTL is in the flash media. I mean, nothing
prevents the FTL from doing a real erase on erase block level and
simply let the "secure erase" request wait on that operation to be
completed.

It looks like the use-cases for "secure erase" are just trying with a
"best effort" in mind. There are no guarantees that the data is really
wiped out from flash, but if it can, it's better than keeping it
around. I guess the real problem comes when the use-case actually
believes that the data is guaranteed to be wiped out, while it may
not.

I really don't have a strong opinion on what way we want to go with
this. Both alternative 1 and alternative 2 work for me, so I leave the
call to you and others.

Kind regards
Uffe

>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-lib.c                     | 55 +++++++++++++++++++++++------
>  block/ioctl.c                       | 43 +++++++++++++++++-----
>  drivers/block/xen-blkback/blkback.c | 15 ++++----
>  fs/f2fs/file.c                      |  9 ++---
>  include/linux/blkdev.h              |  4 +--
>  5 files changed, 95 insertions(+), 31 deletions(-)
>
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 9f09beadcbe30..5fc2c0bf5c940 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -29,7 +29,7 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>  {
>         struct request_queue *q = bdev_get_queue(bdev);
>         struct bio *bio = *biop;
> -       unsigned int op;
> +       unsigned int op = REQ_OP_DISCARD;
>         sector_t bs_mask, part_offset = 0;
>
>         if (!q)
> @@ -38,15 +38,8 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>         if (bdev_read_only(bdev))
>                 return -EPERM;
>
> -       if (flags & BLKDEV_DISCARD_SECURE) {
> -               if (!blk_queue_secure_erase(q))
> -                       return -EOPNOTSUPP;
> -               op = REQ_OP_SECURE_ERASE;
> -       } else {
> -               if (!blk_queue_discard(q))
> -                       return -EOPNOTSUPP;
> -               op = REQ_OP_DISCARD;
> -       }
> +       if (!blk_queue_discard(q))
> +               return -EOPNOTSUPP;
>
>         /* In case the discard granularity isn't set by buggy device driver */
>         if (WARN_ON_ONCE(!q->limits.discard_granularity)) {
> @@ -440,3 +433,45 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
>         return ret;
>  }
>  EXPORT_SYMBOL(blkdev_issue_zeroout);
> +
> +int blkdev_issue_secure_erase(struct block_device *bdev, sector_t sector,
> +               sector_t nr_sects, gfp_t gfp)
> +{
> +       sector_t bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
> +       unsigned int max_sectors =
> +               bdev_get_queue(bdev)->limits.max_discard_sectors;
> +       struct bio *bio = NULL;
> +       struct blk_plug plug;
> +       int ret = 0;
> +
> +       if (max_sectors == 0)
> +               return -EOPNOTSUPP;
> +       if ((sector | nr_sects) & bs_mask)
> +               return -EINVAL;
> +       if (bdev_read_only(bdev))
> +               return -EPERM;
> +
> +       blk_start_plug(&plug);
> +       for (;;) {
> +               unsigned int len = min_t(sector_t, nr_sects, max_sectors);
> +
> +               bio = blk_next_bio(bio, 0, gfp);
> +               bio_set_dev(bio, bdev);
> +               bio->bi_opf = REQ_OP_SECURE_ERASE;
> +               bio->bi_iter.bi_sector = sector;
> +               bio->bi_iter.bi_size = len;
> +
> +               sector += len << SECTOR_SHIFT;
> +               nr_sects -= len << SECTOR_SHIFT;
> +               if (!nr_sects) {
> +                       ret = submit_bio_wait(bio);
> +                       bio_put(bio);
> +                       break;
> +               }
> +               cond_resched();
> +       }
> +       blk_finish_plug(&plug);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL(blkdev_issue_secure_erase);
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 4a86340133e46..0821142f921d7 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -83,7 +83,7 @@ static int compat_blkpg_ioctl(struct block_device *bdev,
>  #endif
>
>  static int blk_ioctl_discard(struct block_device *bdev, fmode_t mode,
> -               unsigned long arg, unsigned long flags)
> +               unsigned long arg)
>  {
>         uint64_t range[2];
>         uint64_t start, len;
> @@ -115,15 +115,43 @@ static int blk_ioctl_discard(struct block_device *bdev, fmode_t mode,
>         err = truncate_bdev_range(bdev, mode, start, start + len - 1);
>         if (err)
>                 goto fail;
> -
> -       err = blkdev_issue_discard(bdev, start >> 9, len >> 9,
> -                                  GFP_KERNEL, flags);
> -
> +       err = blkdev_issue_discard(bdev, start >> 9, len >> 9, GFP_KERNEL, 0);
>  fail:
>         filemap_invalidate_unlock(inode->i_mapping);
>         return err;
>  }
>
> +static int blk_ioctl_secure_erase(struct block_device *bdev, fmode_t mode,
> +               void __user *argp)
> +{
> +       uint64_t start, len;
> +       uint64_t range[2];
> +       int err;
> +
> +       if (!(mode & FMODE_WRITE))
> +               return -EBADF;
> +       if (!blk_queue_discard(bdev_get_queue(bdev)))
> +               return -EOPNOTSUPP;
> +       if (copy_from_user(range, argp, sizeof(range)))
> +               return -EFAULT;
> +
> +       start = range[0];
> +       len = range[1];
> +       if ((start & 511) || (len & 511))
> +               return -EINVAL;
> +       if (start + len > bdev_nr_bytes(bdev))
> +               return -EINVAL;
> +
> +       filemap_invalidate_lock(bdev->bd_inode->i_mapping);
> +       err = truncate_bdev_range(bdev, mode, start, start + len - 1);
> +       if (!err)
> +               err = blkdev_issue_secure_erase(bdev, start >> 9, len >> 9,
> +                                               GFP_KERNEL);
> +       filemap_invalidate_unlock(bdev->bd_inode->i_mapping);
> +       return err;
> +}
> +
> +
>  static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
>                 unsigned long arg)
>  {
> @@ -451,10 +479,9 @@ static int blkdev_common_ioctl(struct block_device *bdev, fmode_t mode,
>         case BLKROSET:
>                 return blkdev_roset(bdev, mode, cmd, arg);
>         case BLKDISCARD:
> -               return blk_ioctl_discard(bdev, mode, arg, 0);
> +               return blk_ioctl_discard(bdev, mode, arg);
>         case BLKSECDISCARD:
> -               return blk_ioctl_discard(bdev, mode, arg,
> -                               BLKDEV_DISCARD_SECURE);
> +               return blk_ioctl_secure_erase(bdev, mode, argp);
>         case BLKZEROOUT:
>                 return blk_ioctl_zeroout(bdev, mode, arg);
>         case BLKGETDISKSEQ:
> diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
> index 14e452896d04c..12f741068bcdf 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -970,7 +970,6 @@ static int dispatch_discard_io(struct xen_blkif_ring *ring,
>         int status = BLKIF_RSP_OKAY;
>         struct xen_blkif *blkif = ring->blkif;
>         struct block_device *bdev = blkif->vbd.bdev;
> -       unsigned long secure;
>         struct phys_req preq;
>
>         xen_blkif_get(blkif);
> @@ -987,13 +986,15 @@ static int dispatch_discard_io(struct xen_blkif_ring *ring,
>         }
>         ring->st_ds_req++;
>
> -       secure = (blkif->vbd.discard_secure &&
> -                (req->u.discard.flag & BLKIF_DISCARD_SECURE)) ?
> -                BLKDEV_DISCARD_SECURE : 0;
> +       if (blkif->vbd.discard_secure &&
> +           (req->u.discard.flag & BLKIF_DISCARD_SECURE))
> +               err = blkdev_issue_secure_erase(bdev,
> +                               req->u.discard.sector_number,
> +                               req->u.discard.nr_sectors, GFP_KERNEL);
> +       else
> +               err = blkdev_issue_discard(bdev, req->u.discard.sector_number,
> +                               req->u.discard.nr_sectors, GFP_KERNEL, 0);
>
> -       err = blkdev_issue_discard(bdev, req->u.discard.sector_number,
> -                                  req->u.discard.nr_sectors,
> -                                  GFP_KERNEL, secure);
>  fail_response:
>         if (err == -EOPNOTSUPP) {
>                 pr_debug("discard op failed, not supported\n");
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 3c98ef6af97d1..a83548ad7171f 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3691,10 +3691,11 @@ static int f2fs_secure_erase(struct block_device *bdev, struct inode *inode,
>         if (!q)
>                 return -ENXIO;
>
> -       if (flags & F2FS_TRIM_FILE_DISCARD)
> -               ret = blkdev_issue_discard(bdev, sector, nr_sects, GFP_NOFS,
> -                                               blk_queue_secure_erase(q) ?
> -                                               BLKDEV_DISCARD_SECURE : 0);
> +       if ((flags & F2FS_TRIM_FILE_DISCARD) && blk_queue_secure_erase(q))
> +               ret = blkdev_issue_secure_erase(bdev, sector, nr_sects,
> +                                               GFP_NOFS);
> +       else if (flags & F2FS_TRIM_FILE_DISCARD)
> +               ret = blkdev_issue_discard(bdev, sector, nr_sects, GFP_NOFS, 0);
>
>         if (!ret && (flags & F2FS_TRIM_FILE_ZEROOUT)) {
>                 if (IS_ENCRYPTED(inode))
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 16b47035e4b06..6cfc60090b119 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -846,13 +846,13 @@ extern void blk_io_schedule(void);
>  extern int blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
>                 sector_t nr_sects, gfp_t gfp_mask, struct page *page);
>
> -#define BLKDEV_DISCARD_SECURE  (1 << 0)        /* issue a secure erase */
> -
>  extern int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>                 sector_t nr_sects, gfp_t gfp_mask, unsigned long flags);
>  extern int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>                 sector_t nr_sects, gfp_t gfp_mask, int flags,
>                 struct bio **biop);
> +int blkdev_issue_secure_erase(struct block_device *bdev, sector_t sector,
> +               sector_t nr_sects, gfp_t gfp);
>
>  #define BLKDEV_ZERO_NOUNMAP    (1 << 0)  /* do not free blocks */
>  #define BLKDEV_ZERO_NOFALLBACK (1 << 1)  /* don't write explicit zeroes */
> --
> 2.30.2
>

Re: security issue: data exposure when using block layer secure erase

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 1534 bytes --]

On Wed, Mar 16, 2022 at 06:05:29PM +0000, Eric Biggers wrote:
> On Wed, Mar 16, 2022 at 10:37:40AM +0100, Christoph Hellwig wrote:
> > Hi all,
> > 
> > while staring at the block layer code I found what I think is a major
> > security issue with the use of REQ_OP_SECURE_ERASE.
> > 
> > The issue is not about the actual protocol implementation, which only
> > exists for eMMC [1], but about we handle issuing the operation in the
> > block layer.  That is done through __blkdev_issue_discard, which
> > takes various parameters into account to align the issue discard
> > request to what the hardware prefers.  Which is perfectly fine for
> > discard as an advisory operation, but deadly for an operation that
> > wants to make data inaccessible.  The problem has existed ever since
> > secure erase support was added to the kernel with commit
> > 8d57a98ccd0b ("block: add secure discard"), which added secure erase
> > support as a REQ_SECURE flag to the discard operation.
> 
> __blkdev_issue_discard() can break up the region into multiple bios, but I don't
> see where it actually skips parts of the region.  Can you explain more
> specifically where the problem is?
> 
> - Eric

I'm also not seeing it.

As I read the __blkdev_issue_discard() function it uses
discard_granularity to define the required sectors (req_sects) for each
bio. req_sects can change on every iteration of the while loop, but
all consecutive bios then start where the previous one ended.

Am I missing something?

Joel

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

Re: security issue: data exposure when using block layer secure erase

[Christoph Hellwig]

On Wed, Mar 16, 2022 at 06:05:29PM +0000, Eric Biggers wrote:
> __blkdev_issue_discard() can break up the region into multiple bios, but I don't
> see where it actually skips parts of the region.  Can you explain more
> specifically where the problem is?

You're right.  We used to skip misaligned parts, but that was actually
fixed two years ago.

Re: [PATCH alternative 2] block: fix the REQ_OP_SECURE_ERASE handling to not leak erased data

[Christoph Hellwig]

On Thu, Mar 17, 2022 at 10:44:01AM +0100, Ulf Hansson wrote:
> Stating that it can't work is probably not a correct statement.
> Certainly it can, but it depends on how "secure" (or clever) the
> implementation of the FTL is in the flash media. I mean, nothing
> prevents the FTL from doing a real erase on erase block level and
> simply let the "secure erase" request wait on that operation to be
> completed.

Well, that assumes it can find all the previous copied of the data.
Having worked with various higher end SSDs FTLs I know they can't,
so if an eMMC device could that would very much surpise me given
the overhead.

Re: [PATCH alternative 2] block: fix the REQ_OP_SECURE_ERASE handling to not leak erased data

[Ulf Hansson]

On Fri, 18 Mar 2022 at 10:11, Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Mar 17, 2022 at 10:44:01AM +0100, Ulf Hansson wrote:
> > Stating that it can't work is probably not a correct statement.
> > Certainly it can, but it depends on how "secure" (or clever) the
> > implementation of the FTL is in the flash media. I mean, nothing
> > prevents the FTL from doing a real erase on erase block level and
> > simply let the "secure erase" request wait on that operation to be
> > completed.
>
> Well, that assumes it can find all the previous copied of the data.
> Having worked with various higher end SSDs FTLs I know they can't,
> so if an eMMC device could that would very much surpise me given
> the overhead.

An eMMC is no different from an SSD in this regard, so you are most
definitely correct. BTW, I was one of those guys working with FTLs
myself, but it was a long time ago, when NAND/NOR flashes were less
complicated to manage.

Anyway, to really make things work, one would need some additional low
level partitioning  - or commands to tag the data for special
purposes. eMMCs do have some support for things like this, but whether
it actually works to serve this particular use case (secure erase), I
really can't tell.

Kind regards
Uffe