security issue: data exposure when using block layer secure erase
Subsystems (8)
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 1/1 block: remove REQ_OP_SECURE_ERASE erase
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.
--- 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(-)
block/blk-core.c +0 -5
@@ -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;
block/blk-lib.c +3 -11
@@ -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;
block/blk-merge.c +1 -5
@@ -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),
block/blk-mq-debugfs.c +0 -1
@@ -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),
block/blk.h +0 -1
@@ -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 */
block/bounce.c +0 -1
@@ -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 :
block/ioctl.c +0 -3
@@ -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 :
drivers/block/rnbd/rnbd-clt.c +0 -2
@@ -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);
drivers/block/rnbd/rnbd-proto.h +0 -6
@@ -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 ;
drivers/block/rnbd/rnbd-srv-dev.h +0 -5
@@ -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)))
drivers/block/rnbd/rnbd-srv.c +1 -2
@@ -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))
drivers/block/xen-blkback/blkback.c +1 -5
@@ -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" );
drivers/block/xen-blkback/common.h +0 -1
@@ -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; };
drivers/block/xen-blkback/xenbus.c +1 -5
@@ -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 ;
drivers/block/xen-blkfront.c +3 -12
@@ -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 *rif (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
drivers/md/dm-ebs-target.c +0 -1
@@ -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 ;
drivers/md/dm-linear.c +0 -1
@@ -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;
drivers/md/dm-stripe.c +0 -2
@@ -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);
drivers/md/dm-table.c +0 -31
@@ -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 )))
drivers/md/dm.c +0 -4
@@ -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 ;
drivers/mmc/core/block.c +0 -73
85 lines not on this page · raw message
drivers/mmc/core/core.c +0 -9
@@ -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) {
drivers/mmc/core/core.h +0 -1
@@ -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);
drivers/mmc/core/queue.c +0 -3
@@ -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)
fs/f2fs/file.c +1 -3
@@ -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))
include/linux/bio.h +0 -3
@@ -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 :
include/linux/blk_types.h +0 -2
@@ -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 */
include/linux/blkdev.h +1 -6
@@ -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,
include/linux/device-mapper.h +0 -6
@@ -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.
kernel/trace/blktrace.c +1 -5
@@ -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 ;
Signature · 1 line
2.30.2
PATCH 1/1 block: fix the REQ_OP_SECURE_ERASE handling to not leak erased data
In reply to Christoph HellwigThe 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.
--- 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(-)
block/blk-lib.c +45 -10
@@ -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);
block/ioctl.c +35 -8
67 lines not on this page · raw message
drivers/block/xen-blkback/blkback.c +8 -7
@@ -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" );
fs/f2fs/file.c +5 -4
@@ -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))
include/linux/blkdev.h +2 -2
@@ -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 */
Signature · 1 line
2.30.2
Re: [PATCH alternative 2] block: fix the REQ_OP_SECURE_ERASE handling to not leak erased data
Quoted 11 lines · 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_erasehelper 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
Quoted 238 lines
Signed-off-by Christoph Hellwig <[email protected]>--- 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(-)block/blk-lib.c +45
-10@@ -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);block/ioctl.c +35
-8@@ -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 :drivers/block/xen-blkback/blkback.c +8
-7@@ -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" );fs/f2fs/file.c +5
-4@@ -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))include/linux/blkdev.h +2
-3@@ -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: [PATCH alternative 2] block: fix the REQ_OP_SECURE_ERASE handling to not leak erased data
Quoted 6 lines · Ulf Hansson
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
Quoted 9 lines · Christoph Hellwig
Quoted 6 lines · Ulf Hansson
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.
Quoted 4 lines
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
Quoted 5 lines · 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.
Quoted 10 lines
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 commit8d57a98ccd0b("block: add secure discard"), which added secure erase support as aREQ_SECUREflag 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
Quoted 17 lines · Eric Biggers
Quoted 5 lines · 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.Quoted 10 lines
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 commit8d57a98ccd0b("block: add secure discard"), which added secure erase support as aREQ_SECUREflag to the discard operation.
Quoted 5 lines
__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
Quoted 3 lines · Eric Biggers
__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.