[PATCH 0/6] block: Consolidate scsi sense buffer usage

[PATCH 0/6] block: Consolidate scsi sense buffer usage

[Kees Cook]

This is a follow-up to commit f7068114d45e ("sr: pass down correctly
sized SCSI sense buffer") which further cleans up and removes needless
sense character array buffers and "struct request_sense" usage in favor
of the common "struct scsi_sense_hdr".

First, drop a bunch of unused sense buffers:
 [PATCH 1/6] ide-cd: Drop unused sense buffers
 [PATCH 2/6] scsi: cxlflash: Drop unused sense buffers

Next, split out struct scsi_sense_hdr:
 [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

Then move all request_sense usage to scsi_sense_hdr:
 [PATCH 4/6] block: Consolidate scsi sense buffer usage

Finally add a build-time check to make sure we don't pass bad buffer sizes:
 [PATCH 5/6] libata-scsi: Move sense buffers onto stack
 [PATCH 6/6] scsi: Check sense buffer size at build time

-Kees

[PATCH 1/6] ide-cd: Drop unused sense buffers

[Kees Cook]

This drops unused sense buffers from:

	cdrom_eject()
	cdrom_read_capacity()
	cdrom_read_tocentry()
	ide_cd_lockdoor()
	ide_cd_read_toc()

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/ide/ide-cd.c       | 36 +++++++++++++++---------------------
 drivers/ide/ide-cd.h       |  2 +-
 drivers/ide/ide-cd_ioctl.c | 34 ++++++++++++----------------------
 3 files changed, 28 insertions(+), 44 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 5a8e8e3c22cd..1d5b35312e33 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -890,8 +890,7 @@ int cdrom_check_status(ide_drive_t *drive, struct request_sense *sense)
 }
 
 static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
-			       unsigned long *sectors_per_frame,
-			       struct request_sense *sense)
+			       unsigned long *sectors_per_frame)
 {
 	struct {
 		__be32 lba;
@@ -908,7 +907,7 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
 	memset(cmd, 0, BLK_MAX_CDB);
 	cmd[0] = GPCMD_READ_CDVD_CAPACITY;
 
-	stat = ide_cd_queue_pc(drive, cmd, 0, &capbuf, &len, sense, 0,
+	stat = ide_cd_queue_pc(drive, cmd, 0, &capbuf, &len, NULL, 0,
 			       RQF_QUIET);
 	if (stat)
 		return stat;
@@ -944,8 +943,7 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
 }
 
 static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,
-				int format, char *buf, int buflen,
-				struct request_sense *sense)
+				int format, char *buf, int buflen)
 {
 	unsigned char cmd[BLK_MAX_CDB];
 
@@ -962,11 +960,11 @@ static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,
 	if (msf_flag)
 		cmd[1] = 2;
 
-	return ide_cd_queue_pc(drive, cmd, 0, buf, &buflen, sense, 0, RQF_QUIET);
+	return ide_cd_queue_pc(drive, cmd, 0, buf, &buflen, NULL, 0, RQF_QUIET);
 }
 
 /* Try to read the entire TOC for the disk into our internal buffer. */
-int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
+int ide_cd_read_toc(ide_drive_t *drive)
 {
 	int stat, ntracks, i;
 	struct cdrom_info *info = drive->driver_data;
@@ -996,14 +994,13 @@ int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
 	 * Check to see if the existing data is still valid. If it is,
 	 * just return.
 	 */
-	(void) cdrom_check_status(drive, sense);
+	(void) cdrom_check_status(drive, NULL);
 
 	if (drive->atapi_flags & IDE_AFLAG_TOC_VALID)
 		return 0;
 
 	/* try to get the total cdrom capacity and sector size */
-	stat = cdrom_read_capacity(drive, &toc->capacity, &sectors_per_frame,
-				   sense);
+	stat = cdrom_read_capacity(drive, &toc->capacity, &sectors_per_frame);
 	if (stat)
 		toc->capacity = 0x1fffff;
 
@@ -1016,7 +1013,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
 
 	/* first read just the header, so we know how long the TOC is */
 	stat = cdrom_read_tocentry(drive, 0, 1, 0, (char *) &toc->hdr,
-				    sizeof(struct atapi_toc_header), sense);
+				    sizeof(struct atapi_toc_header));
 	if (stat)
 		return stat;
 
@@ -1036,7 +1033,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
 				  (char *)&toc->hdr,
 				   sizeof(struct atapi_toc_header) +
 				   (ntracks + 1) *
-				   sizeof(struct atapi_toc_entry), sense);
+				   sizeof(struct atapi_toc_entry));
 
 	if (stat && toc->hdr.first_track > 1) {
 		/*
@@ -1056,8 +1053,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
 					   (char *)&toc->hdr,
 					   sizeof(struct atapi_toc_header) +
 					   (ntracks + 1) *
-					   sizeof(struct atapi_toc_entry),
-					   sense);
+					   sizeof(struct atapi_toc_entry));
 		if (stat)
 			return stat;
 
@@ -1094,7 +1090,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
 	if (toc->hdr.first_track != CDROM_LEADOUT) {
 		/* read the multisession information */
 		stat = cdrom_read_tocentry(drive, 0, 0, 1, (char *)&ms_tmp,
-					   sizeof(ms_tmp), sense);
+					   sizeof(ms_tmp));
 		if (stat)
 			return stat;
 
@@ -1108,7 +1104,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
 	if (drive->atapi_flags & IDE_AFLAG_TOCADDR_AS_BCD) {
 		/* re-read multisession information using MSF format */
 		stat = cdrom_read_tocentry(drive, 0, 1, 1, (char *)&ms_tmp,
-					   sizeof(ms_tmp), sense);
+					   sizeof(ms_tmp));
 		if (stat)
 			return stat;
 
@@ -1412,7 +1408,7 @@ static sector_t ide_cdrom_capacity(ide_drive_t *drive)
 {
 	unsigned long capacity, sectors_per_frame;
 
-	if (cdrom_read_capacity(drive, &capacity, &sectors_per_frame, NULL))
+	if (cdrom_read_capacity(drive, &capacity, &sectors_per_frame))
 		return 0;
 
 	return capacity * sectors_per_frame;
@@ -1723,9 +1719,8 @@ static unsigned int idecd_check_events(struct gendisk *disk,
 static int idecd_revalidate_disk(struct gendisk *disk)
 {
 	struct cdrom_info *info = ide_drv_g(disk, cdrom_info);
-	struct request_sense sense;
 
-	ide_cd_read_toc(info->drive, &sense);
+	ide_cd_read_toc(info->drive);
 
 	return  0;
 }
@@ -1749,7 +1744,6 @@ static int ide_cd_probe(ide_drive_t *drive)
 {
 	struct cdrom_info *info;
 	struct gendisk *g;
-	struct request_sense sense;
 
 	ide_debug_log(IDE_DBG_PROBE, "driver_req: %s, media: 0x%x",
 				     drive->driver_req, drive->media);
@@ -1798,7 +1792,7 @@ static int ide_cd_probe(ide_drive_t *drive)
 		goto failed;
 	}
 
-	ide_cd_read_toc(drive, &sense);
+	ide_cd_read_toc(drive);
 	g->fops = &idecd_ops;
 	g->flags |= GENHD_FL_REMOVABLE | GENHD_FL_BLOCK_EVENTS_ON_EXCL_WRITE;
 	device_add_disk(&drive->gendev, g);
diff --git a/drivers/ide/ide-cd.h b/drivers/ide/ide-cd.h
index 04f0f310a856..fc162fbb6629 100644
--- a/drivers/ide/ide-cd.h
+++ b/drivers/ide/ide-cd.h
@@ -99,7 +99,7 @@ void ide_cd_log_error(const char *, struct request *, struct request_sense *);
 /* ide-cd.c functions used by ide-cd_ioctl.c */
 int ide_cd_queue_pc(ide_drive_t *, const unsigned char *, int, void *,
 		    unsigned *, struct request_sense *, int, req_flags_t);
-int ide_cd_read_toc(ide_drive_t *, struct request_sense *);
+int ide_cd_read_toc(ide_drive_t *);
 int ide_cdrom_get_capabilities(ide_drive_t *, u8 *);
 void ide_cdrom_update_speed(ide_drive_t *, u8 *);
 int cdrom_check_status(ide_drive_t *, struct request_sense *);
diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
index 2acca12b9c94..c709f4eed0e9 100644
--- a/drivers/ide/ide-cd_ioctl.c
+++ b/drivers/ide/ide-cd_ioctl.c
@@ -105,8 +105,7 @@ unsigned int ide_cdrom_check_events_real(struct cdrom_device_info *cdi,
 /* Eject the disk if EJECTFLAG is 0.
    If EJECTFLAG is 1, try to reload the disk. */
 static
-int cdrom_eject(ide_drive_t *drive, int ejectflag,
-		struct request_sense *sense)
+int cdrom_eject(ide_drive_t *drive, int ejectflag)
 {
 	struct cdrom_info *cd = drive->driver_data;
 	struct cdrom_device_info *cdi = &cd->devinfo;
@@ -129,20 +128,16 @@ int cdrom_eject(ide_drive_t *drive, int ejectflag,
 	cmd[0] = GPCMD_START_STOP_UNIT;
 	cmd[4] = loej | (ejectflag != 0);
 
-	return ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, sense, 0, 0);
+	return ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, NULL, 0, 0);
 }
 
 /* Lock the door if LOCKFLAG is nonzero; unlock it otherwise. */
 static
-int ide_cd_lockdoor(ide_drive_t *drive, int lockflag,
-		    struct request_sense *sense)
+int ide_cd_lockdoor(ide_drive_t *drive, int lockflag)
 {
-	struct request_sense my_sense;
+	struct request_sense my_sense, *sense = &my_sense;
 	int stat;
 
-	if (sense == NULL)
-		sense = &my_sense;
-
 	/* If the drive cannot lock the door, just pretend. */
 	if ((drive->dev_flags & IDE_DFLAG_DOORLOCKING) == 0) {
 		stat = 0;
@@ -186,23 +181,22 @@ int ide_cd_lockdoor(ide_drive_t *drive, int lockflag,
 int ide_cdrom_tray_move(struct cdrom_device_info *cdi, int position)
 {
 	ide_drive_t *drive = cdi->handle;
-	struct request_sense sense;
 
 	if (position) {
-		int stat = ide_cd_lockdoor(drive, 0, &sense);
+		int stat = ide_cd_lockdoor(drive, 0);
 
 		if (stat)
 			return stat;
 	}
 
-	return cdrom_eject(drive, !position, &sense);
+	return cdrom_eject(drive, !position);
 }
 
 int ide_cdrom_lock_door(struct cdrom_device_info *cdi, int lock)
 {
 	ide_drive_t *drive = cdi->handle;
 
-	return ide_cd_lockdoor(drive, lock, NULL);
+	return ide_cd_lockdoor(drive, lock);
 }
 
 /*
@@ -213,7 +207,6 @@ int ide_cdrom_select_speed(struct cdrom_device_info *cdi, int speed)
 {
 	ide_drive_t *drive = cdi->handle;
 	struct cdrom_info *cd = drive->driver_data;
-	struct request_sense sense;
 	u8 buf[ATAPI_CAPABILITIES_PAGE_SIZE];
 	int stat;
 	unsigned char cmd[BLK_MAX_CDB];
@@ -236,7 +229,7 @@ int ide_cdrom_select_speed(struct cdrom_device_info *cdi, int speed)
 		cmd[5] = speed & 0xff;
 	}
 
-	stat = ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, &sense, 0, 0);
+	stat = ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, NULL, 0, 0);
 
 	if (!ide_cdrom_get_capabilities(drive, buf)) {
 		ide_cdrom_update_speed(drive, buf);
@@ -252,11 +245,10 @@ int ide_cdrom_get_last_session(struct cdrom_device_info *cdi,
 	struct atapi_toc *toc;
 	ide_drive_t *drive = cdi->handle;
 	struct cdrom_info *info = drive->driver_data;
-	struct request_sense sense;
 	int ret;
 
 	if ((drive->atapi_flags & IDE_AFLAG_TOC_VALID) == 0 || !info->toc) {
-		ret = ide_cd_read_toc(drive, &sense);
+		ret = ide_cd_read_toc(drive);
 		if (ret)
 			return ret;
 	}
@@ -300,7 +292,6 @@ int ide_cdrom_reset(struct cdrom_device_info *cdi)
 {
 	ide_drive_t *drive = cdi->handle;
 	struct cdrom_info *cd = drive->driver_data;
-	struct request_sense sense;
 	struct request *rq;
 	int ret;
 
@@ -315,7 +306,7 @@ int ide_cdrom_reset(struct cdrom_device_info *cdi)
 	 * lock it again.
 	 */
 	if (drive->atapi_flags & IDE_AFLAG_DOOR_LOCKED)
-		(void)ide_cd_lockdoor(drive, 1, &sense);
+		(void)ide_cd_lockdoor(drive, 1);
 
 	return ret;
 }
@@ -355,7 +346,6 @@ static int ide_cd_fake_play_trkind(ide_drive_t *drive, void *arg)
 	struct atapi_toc_entry *first_toc, *last_toc;
 	unsigned long lba_start, lba_end;
 	int stat;
-	struct request_sense sense;
 	unsigned char cmd[BLK_MAX_CDB];
 
 	stat = ide_cd_get_toc_entry(drive, ti->cdti_trk0, &first_toc);
@@ -380,7 +370,7 @@ static int ide_cd_fake_play_trkind(ide_drive_t *drive, void *arg)
 	lba_to_msf(lba_start,   &cmd[3], &cmd[4], &cmd[5]);
 	lba_to_msf(lba_end - 1, &cmd[6], &cmd[7], &cmd[8]);
 
-	return ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, &sense, 0, 0);
+	return ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, NULL, 0, 0);
 }
 
 static int ide_cd_read_tochdr(ide_drive_t *drive, void *arg)
@@ -391,7 +381,7 @@ static int ide_cd_read_tochdr(ide_drive_t *drive, void *arg)
 	int stat;
 
 	/* Make sure our saved TOC is valid. */
-	stat = ide_cd_read_toc(drive, NULL);
+	stat = ide_cd_read_toc(drive);
 	if (stat)
 		return stat;
 
-- 
2.17.0

[PATCH 2/6] scsi: cxlflash: Drop unused sense buffers

[Kees Cook]

This removes the unused sense buffer in read_cap16() and write_same16().

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/cxlflash/superpipe.c | 8 ++------
 drivers/scsi/cxlflash/vlun.c      | 7 ++-----
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index 2fe79df5c73c..59b9f2023748 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -324,7 +324,6 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
 	struct scsi_sense_hdr sshdr;
 	u8 *cmd_buf = NULL;
 	u8 *scsi_cmd = NULL;
-	u8 *sense_buf = NULL;
 	int rc = 0;
 	int result = 0;
 	int retry_cnt = 0;
@@ -333,8 +332,7 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
 retry:
 	cmd_buf = kzalloc(CMD_BUFSIZE, GFP_KERNEL);
 	scsi_cmd = kzalloc(MAX_COMMAND_SIZE, GFP_KERNEL);
-	sense_buf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
-	if (unlikely(!cmd_buf || !scsi_cmd || !sense_buf)) {
+	if (unlikely(!cmd_buf || !scsi_cmd)) {
 		rc = -ENOMEM;
 		goto out;
 	}
@@ -349,7 +347,7 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
 	/* Drop the ioctl read semahpore across lengthy call */
 	up_read(&cfg->ioctl_rwsem);
 	result = scsi_execute(sdev, scsi_cmd, DMA_FROM_DEVICE, cmd_buf,
-			      CMD_BUFSIZE, sense_buf, &sshdr, to, CMD_RETRIES,
+			      CMD_BUFSIZE, NULL, &sshdr, to, CMD_RETRIES,
 			      0, 0, NULL);
 	down_read(&cfg->ioctl_rwsem);
 	rc = check_state(cfg);
@@ -380,7 +378,6 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
 					if (retry_cnt++ < 1) {
 						kfree(cmd_buf);
 						kfree(scsi_cmd);
-						kfree(sense_buf);
 						goto retry;
 					}
 				}
@@ -411,7 +408,6 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
 out:
 	kfree(cmd_buf);
 	kfree(scsi_cmd);
-	kfree(sense_buf);
 
 	dev_dbg(dev, "%s: maxlba=%lld blklen=%d rc=%d\n",
 		__func__, gli->max_lba, gli->blk_len, rc);
diff --git a/drivers/scsi/cxlflash/vlun.c b/drivers/scsi/cxlflash/vlun.c
index 5deef57a7834..e7e9b2f2ad21 100644
--- a/drivers/scsi/cxlflash/vlun.c
+++ b/drivers/scsi/cxlflash/vlun.c
@@ -425,7 +425,6 @@ static int write_same16(struct scsi_device *sdev,
 {
 	u8 *cmd_buf = NULL;
 	u8 *scsi_cmd = NULL;
-	u8 *sense_buf = NULL;
 	int rc = 0;
 	int result = 0;
 	u64 offset = lba;
@@ -439,8 +438,7 @@ static int write_same16(struct scsi_device *sdev,
 
 	cmd_buf = kzalloc(CMD_BUFSIZE, GFP_KERNEL);
 	scsi_cmd = kzalloc(MAX_COMMAND_SIZE, GFP_KERNEL);
-	sense_buf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
-	if (unlikely(!cmd_buf || !scsi_cmd || !sense_buf)) {
+	if (unlikely(!cmd_buf || !scsi_cmd)) {
 		rc = -ENOMEM;
 		goto out;
 	}
@@ -456,7 +454,7 @@ static int write_same16(struct scsi_device *sdev,
 		/* Drop the ioctl read semahpore across lengthy call */
 		up_read(&cfg->ioctl_rwsem);
 		result = scsi_execute(sdev, scsi_cmd, DMA_TO_DEVICE, cmd_buf,
-				      CMD_BUFSIZE, sense_buf, NULL, to,
+				      CMD_BUFSIZE, NULL, NULL, to,
 				      CMD_RETRIES, 0, 0, NULL);
 		down_read(&cfg->ioctl_rwsem);
 		rc = check_state(cfg);
@@ -481,7 +479,6 @@ static int write_same16(struct scsi_device *sdev,
 out:
 	kfree(cmd_buf);
 	kfree(scsi_cmd);
-	kfree(sense_buf);
 	dev_dbg(dev, "%s: returning rc=%d\n", __func__, rc);
 	return rc;
 }
-- 
2.17.0

[PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

Both SCSI and ATAPI share the sense header. In preparation for using the
struct scsi_sense_hdr more widely, move this into a separate header and
move the helper function to scsi_ioctl.c which is linked with CONFIG_IDE
by way of CONFIG_BLK_SCSI_REQUEST.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 block/scsi_ioctl.c         | 64 ++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_common.c | 64 --------------------------------------
 include/scsi/scsi_cmnd.h   |  1 -
 include/scsi/scsi_common.h | 32 +------------------
 include/scsi/scsi_sense.h  | 44 ++++++++++++++++++++++++++
 5 files changed, 109 insertions(+), 96 deletions(-)
 create mode 100644 include/scsi/scsi_sense.h

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 60b471f8621b..87ff3cc7a364 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -728,6 +728,70 @@ void scsi_req_init(struct scsi_request *req)
 }
 EXPORT_SYMBOL(scsi_req_init);
 
+/**
+ * scsi_normalize_sense - normalize main elements from either fixed or
+ *			descriptor sense data format into a common format.
+ *
+ * @sense_buffer:	byte array containing sense data returned by device
+ * @sb_len:		number of valid bytes in sense_buffer
+ * @sshdr:		pointer to instance of structure that common
+ *			elements are written to.
+ *
+ * Notes:
+ *	The "main elements" from sense data are: response_code, sense_key,
+ *	asc, ascq and additional_length (only for descriptor format).
+ *
+ *	Typically this function can be called after a device has
+ *	responded to a SCSI command with the CHECK_CONDITION status.
+ *
+ * Return value:
+ *	true if valid sense data information found, else false;
+ */
+bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
+			  struct scsi_sense_hdr *sshdr)
+{
+	memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
+
+	if (!sense_buffer || !sb_len)
+		return false;
+
+	sshdr->response_code = (sense_buffer[0] & 0x7f);
+
+	if (!scsi_sense_valid(sshdr))
+		return false;
+
+	if (sshdr->response_code >= 0x72) {
+		/*
+		 * descriptor format
+		 */
+		if (sb_len > 1)
+			sshdr->sense_key = (sense_buffer[1] & 0xf);
+		if (sb_len > 2)
+			sshdr->asc = sense_buffer[2];
+		if (sb_len > 3)
+			sshdr->ascq = sense_buffer[3];
+		if (sb_len > 7)
+			sshdr->additional_length = sense_buffer[7];
+	} else {
+		/*
+		 * fixed format
+		 */
+		if (sb_len > 2)
+			sshdr->sense_key = (sense_buffer[2] & 0xf);
+		if (sb_len > 7) {
+			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
+					 sb_len : (sense_buffer[7] + 8);
+			if (sb_len > 12)
+				sshdr->asc = sense_buffer[12];
+			if (sb_len > 13)
+				sshdr->ascq = sense_buffer[13];
+		}
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(scsi_normalize_sense);
+
 static int __init blk_scsi_ioctl_init(void)
 {
 	blk_set_cmd_filter_defaults(&blk_default_cmd_filter);
diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
index 90349498f686..8621a07cb967 100644
--- a/drivers/scsi/scsi_common.c
+++ b/drivers/scsi/scsi_common.c
@@ -116,70 +116,6 @@ void int_to_scsilun(u64 lun, struct scsi_lun *scsilun)
 }
 EXPORT_SYMBOL(int_to_scsilun);
 
-/**
- * scsi_normalize_sense - normalize main elements from either fixed or
- *			descriptor sense data format into a common format.
- *
- * @sense_buffer:	byte array containing sense data returned by device
- * @sb_len:		number of valid bytes in sense_buffer
- * @sshdr:		pointer to instance of structure that common
- *			elements are written to.
- *
- * Notes:
- *	The "main elements" from sense data are: response_code, sense_key,
- *	asc, ascq and additional_length (only for descriptor format).
- *
- *	Typically this function can be called after a device has
- *	responded to a SCSI command with the CHECK_CONDITION status.
- *
- * Return value:
- *	true if valid sense data information found, else false;
- */
-bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
-			  struct scsi_sense_hdr *sshdr)
-{
-	memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
-
-	if (!sense_buffer || !sb_len)
-		return false;
-
-	sshdr->response_code = (sense_buffer[0] & 0x7f);
-
-	if (!scsi_sense_valid(sshdr))
-		return false;
-
-	if (sshdr->response_code >= 0x72) {
-		/*
-		 * descriptor format
-		 */
-		if (sb_len > 1)
-			sshdr->sense_key = (sense_buffer[1] & 0xf);
-		if (sb_len > 2)
-			sshdr->asc = sense_buffer[2];
-		if (sb_len > 3)
-			sshdr->ascq = sense_buffer[3];
-		if (sb_len > 7)
-			sshdr->additional_length = sense_buffer[7];
-	} else {
-		/*
-		 * fixed format
-		 */
-		if (sb_len > 2)
-			sshdr->sense_key = (sense_buffer[2] & 0xf);
-		if (sb_len > 7) {
-			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
-					 sb_len : (sense_buffer[7] + 8);
-			if (sb_len > 12)
-				sshdr->asc = sense_buffer[12];
-			if (sb_len > 13)
-				sshdr->ascq = sense_buffer[13];
-		}
-	}
-
-	return true;
-}
-EXPORT_SYMBOL(scsi_normalize_sense);
-
 /**
  * scsi_sense_desc_find - search for a given descriptor type in	descriptor sense data format.
  * @sense_buffer:	byte array of descriptor format sense data
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index aaf1e971c6a3..3b9f42f6615c 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -120,7 +120,6 @@ struct scsi_cmnd {
 	struct request *request;	/* The command we are
 				   	   working on */
 
-#define SCSI_SENSE_BUFFERSIZE 	96
 	unsigned char *sense_buffer;
 				/* obtained by REQUEST SENSE when
 				 * CHECK CONDITION is received on original
diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
index 731ac09ed231..82d6209f2e10 100644
--- a/include/scsi/scsi_common.h
+++ b/include/scsi/scsi_common.h
@@ -8,6 +8,7 @@
 
 #include <linux/types.h>
 #include <scsi/scsi_proto.h>
+#include <scsi/scsi_sense.h>
 
 static inline unsigned
 scsi_varlen_cdb_length(const void *hdr)
@@ -31,37 +32,6 @@ extern const char *scsi_device_type(unsigned type);
 extern void int_to_scsilun(u64, struct scsi_lun *);
 extern u64 scsilun_to_int(struct scsi_lun *);
 
-/*
- * This is a slightly modified SCSI sense "descriptor" format header.
- * The addition is to allow the 0x70 and 0x71 response codes. The idea
- * is to place the salient data from either "fixed" or "descriptor" sense
- * format into one structure to ease application processing.
- *
- * The original sense buffer should be kept around for those cases
- * in which more information is required (e.g. the LBA of a MEDIUM ERROR).
- */
-struct scsi_sense_hdr {		/* See SPC-3 section 4.5 */
-	u8 response_code;	/* permit: 0x0, 0x70, 0x71, 0x72, 0x73 */
-	u8 sense_key;
-	u8 asc;
-	u8 ascq;
-	u8 byte4;
-	u8 byte5;
-	u8 byte6;
-	u8 additional_length;	/* always 0 for fixed sense format */
-};
-
-static inline bool scsi_sense_valid(const struct scsi_sense_hdr *sshdr)
-{
-	if (!sshdr)
-		return false;
-
-	return (sshdr->response_code & 0x70) == 0x70;
-}
-
-extern bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
-				 struct scsi_sense_hdr *sshdr);
-
 extern void scsi_build_sense_buffer(int desc, u8 *buf, u8 key, u8 asc, u8 ascq);
 int scsi_set_sense_information(u8 *buf, int buf_len, u64 info);
 int scsi_set_sense_field_pointer(u8 *buf, int buf_len, u16 fp, u8 bp, bool cd);
diff --git a/include/scsi/scsi_sense.h b/include/scsi/scsi_sense.h
new file mode 100644
index 000000000000..be923c67b93e
--- /dev/null
+++ b/include/scsi/scsi_sense.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Structures and functions used by both SCSI and ATAPI sense code.
+ */
+
+#ifndef _SCSI_SENSE_H_
+#define _SCSI_SENSE_H_
+
+#include <linux/types.h>
+
+/*
+ * This is a slightly modified SCSI sense "descriptor" format header.
+ * The addition is to allow the 0x70 and 0x71 response codes. The idea
+ * is to place the salient data from either "fixed" or "descriptor" sense
+ * format into one structure to ease application processing.
+ *
+ * The original sense buffer should be kept around for those cases
+ * in which more information is required (e.g. the LBA of a MEDIUM ERROR).
+ */
+struct scsi_sense_hdr {		/* See SPC-3 section 4.5 */
+	u8 response_code;	/* permit: 0x0, 0x70, 0x71, 0x72, 0x73 */
+	u8 sense_key;
+	u8 asc;
+	u8 ascq;
+	u8 byte4;
+	u8 byte5;
+	u8 byte6;
+	u8 additional_length;	/* always 0 for fixed sense format */
+};
+
+static inline bool scsi_sense_valid(const struct scsi_sense_hdr *sshdr)
+{
+	if (!sshdr)
+		return false;
+
+	return (sshdr->response_code & 0x70) == 0x70;
+}
+
+#define SCSI_SENSE_BUFFERSIZE 	96
+
+extern bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
+				 struct scsi_sense_hdr *sshdr);
+
+#endif /* _SCSI_SENSE_H_ */
-- 
2.17.0

[PATCH 4/6] block: Consolidate scsi sense buffer usage

[Kees Cook]

There is a lot of needless struct request_sense usage in the CDROM
code. These can all be struct scsi_sense_hdr instead, to avoid any
confusion over their respective structure sizes. This patch is a lot
of noise changing "sense" to "sshdr", but the final code is more
readable to distinguish between "sense" meaning "struct request_sense"
and "sshdr" meaning "struct scsi_sense_hdr".

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/block/pktcdvd.c    | 36 ++++++++++++++++++------------------
 drivers/cdrom/cdrom.c      | 22 +++++++++++-----------
 drivers/ide/ide-cd.c       | 11 ++++++-----
 drivers/ide/ide-cd.h       |  5 +++--
 drivers/ide/ide-cd_ioctl.c | 30 +++++++++++++++---------------
 drivers/scsi/sr_ioctl.c    | 22 +++++++++-------------
 include/linux/cdrom.h      |  3 ++-
 7 files changed, 64 insertions(+), 65 deletions(-)

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index c61d20c9f3f8..f91c9f85e29d 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -748,13 +748,13 @@ static const char *sense_key_string(__u8 index)
 static void pkt_dump_sense(struct pktcdvd_device *pd,
 			   struct packet_command *cgc)
 {
-	struct request_sense *sense = cgc->sense;
+	struct scsi_sense_hdr *sshdr = cgc->sshdr;
 
-	if (sense)
+	if (sshdr)
 		pkt_err(pd, "%*ph - sense %02x.%02x.%02x (%s)\n",
 			CDROM_PACKET_SIZE, cgc->cmd,
-			sense->sense_key, sense->asc, sense->ascq,
-			sense_key_string(sense->sense_key));
+			sshdr->sense_key, sshdr->asc, sshdr->ascq,
+			sense_key_string(sshdr->sense_key));
 	else
 		pkt_err(pd, "%*ph - no sense\n", CDROM_PACKET_SIZE, cgc->cmd);
 }
@@ -787,11 +787,11 @@ static noinline_for_stack int pkt_set_speed(struct pktcdvd_device *pd,
 				unsigned write_speed, unsigned read_speed)
 {
 	struct packet_command cgc;
-	struct request_sense sense;
+	struct scsi_sense_hdr sshdr;
 	int ret;
 
 	init_cdrom_command(&cgc, NULL, 0, CGC_DATA_NONE);
-	cgc.sense = &sense;
+	cgc.sshdr = &sshdr;
 	cgc.cmd[0] = GPCMD_SET_SPEED;
 	cgc.cmd[2] = (read_speed >> 8) & 0xff;
 	cgc.cmd[3] = read_speed & 0xff;
@@ -1645,7 +1645,7 @@ static noinline_for_stack int pkt_get_last_written(struct pktcdvd_device *pd,
 static noinline_for_stack int pkt_set_write_settings(struct pktcdvd_device *pd)
 {
 	struct packet_command cgc;
-	struct request_sense sense;
+	struct scsi_sense_hdr sshdr;
 	write_param_page *wp;
 	char buffer[128];
 	int ret, size;
@@ -1656,7 +1656,7 @@ static noinline_for_stack int pkt_set_write_settings(struct pktcdvd_device *pd)
 
 	memset(buffer, 0, sizeof(buffer));
 	init_cdrom_command(&cgc, buffer, sizeof(*wp), CGC_DATA_READ);
-	cgc.sense = &sense;
+	cgc.sshdr = &sshdr;
 	if ((ret = pkt_mode_sense(pd, &cgc, GPMODE_WRITE_PARMS_PAGE, 0))) {
 		pkt_dump_sense(pd, &cgc);
 		return ret;
@@ -1671,7 +1671,7 @@ static noinline_for_stack int pkt_set_write_settings(struct pktcdvd_device *pd)
 	 * now get it all
 	 */
 	init_cdrom_command(&cgc, buffer, size, CGC_DATA_READ);
-	cgc.sense = &sense;
+	cgc.sshdr = &sshdr;
 	if ((ret = pkt_mode_sense(pd, &cgc, GPMODE_WRITE_PARMS_PAGE, 0))) {
 		pkt_dump_sense(pd, &cgc);
 		return ret;
@@ -1905,12 +1905,12 @@ static noinline_for_stack int pkt_write_caching(struct pktcdvd_device *pd,
 						int set)
 {
 	struct packet_command cgc;
-	struct request_sense sense;
+	struct scsi_sense_hdr sshdr;
 	unsigned char buf[64];
 	int ret;
 
 	init_cdrom_command(&cgc, buf, sizeof(buf), CGC_DATA_READ);
-	cgc.sense = &sense;
+	cgc.sshdr = &sshdr;
 	cgc.buflen = pd->mode_offset + 12;
 
 	/*
@@ -1950,14 +1950,14 @@ static noinline_for_stack int pkt_get_max_speed(struct pktcdvd_device *pd,
 						unsigned *write_speed)
 {
 	struct packet_command cgc;
-	struct request_sense sense;
+	struct scsi_sense_hdr sshdr;
 	unsigned char buf[256+18];
 	unsigned char *cap_buf;
 	int ret, offset;
 
 	cap_buf = &buf[sizeof(struct mode_page_header) + pd->mode_offset];
 	init_cdrom_command(&cgc, buf, sizeof(buf), CGC_DATA_UNKNOWN);
-	cgc.sense = &sense;
+	cgc.sshdr = &sshdr;
 
 	ret = pkt_mode_sense(pd, &cgc, GPMODE_CAPABILITIES_PAGE, 0);
 	if (ret) {
@@ -2011,13 +2011,13 @@ static noinline_for_stack int pkt_media_speed(struct pktcdvd_device *pd,
 						unsigned *speed)
 {
 	struct packet_command cgc;
-	struct request_sense sense;
+	struct scsi_sense_hdr sshdr;
 	unsigned char buf[64];
 	unsigned int size, st, sp;
 	int ret;
 
 	init_cdrom_command(&cgc, buf, 2, CGC_DATA_READ);
-	cgc.sense = &sense;
+	cgc.sshdr = &sshdr;
 	cgc.cmd[0] = GPCMD_READ_TOC_PMA_ATIP;
 	cgc.cmd[1] = 2;
 	cgc.cmd[2] = 4; /* READ ATIP */
@@ -2032,7 +2032,7 @@ static noinline_for_stack int pkt_media_speed(struct pktcdvd_device *pd,
 		size = sizeof(buf);
 
 	init_cdrom_command(&cgc, buf, size, CGC_DATA_READ);
-	cgc.sense = &sense;
+	cgc.sshdr = &sshdr;
 	cgc.cmd[0] = GPCMD_READ_TOC_PMA_ATIP;
 	cgc.cmd[1] = 2;
 	cgc.cmd[2] = 4;
@@ -2083,13 +2083,13 @@ static noinline_for_stack int pkt_media_speed(struct pktcdvd_device *pd,
 static noinline_for_stack int pkt_perform_opc(struct pktcdvd_device *pd)
 {
 	struct packet_command cgc;
-	struct request_sense sense;
+	struct scsi_sense_hdr sshdr;
 	int ret;
 
 	pkt_dbg(2, pd, "Performing OPC\n");
 
 	init_cdrom_command(&cgc, NULL, 0, CGC_DATA_NONE);
-	cgc.sense = &sense;
+	cgc.sshdr = &sshdr;
 	cgc.timeout = 60*HZ;
 	cgc.cmd[0] = GPCMD_SEND_OPC;
 	cgc.cmd[1] = 1;
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index bfc566d3f31a..3522d2cae1b6 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -345,10 +345,10 @@ static LIST_HEAD(cdrom_list);
 int cdrom_dummy_generic_packet(struct cdrom_device_info *cdi,
 			       struct packet_command *cgc)
 {
-	if (cgc->sense) {
-		cgc->sense->sense_key = 0x05;
-		cgc->sense->asc = 0x20;
-		cgc->sense->ascq = 0x00;
+	if (cgc->sshdr) {
+		cgc->sshdr->sense_key = 0x05;
+		cgc->sshdr->asc = 0x20;
+		cgc->sshdr->ascq = 0x00;
 	}
 
 	cgc->stat = -EIO;
@@ -2943,7 +2943,7 @@ static noinline int mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi,
 					      struct packet_command *cgc,
 					      int cmd)
 {
-	struct request_sense sense;
+	struct scsi_sense_hdr sshdr;
 	struct cdrom_msf msf;
 	int blocksize = 0, format = 0, lba;
 	int ret;
@@ -2971,13 +2971,13 @@ static noinline int mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi,
 	if (cgc->buffer == NULL)
 		return -ENOMEM;
 
-	memset(&sense, 0, sizeof(sense));
-	cgc->sense = &sense;
+	memset(&sshdr, 0, sizeof(sshdr));
+	cgc->sshdr = &sshdr;
 	cgc->data_direction = CGC_DATA_READ;
 	ret = cdrom_read_block(cdi, cgc, lba, 1, format, blocksize);
-	if (ret && sense.sense_key == 0x05 &&
-	    sense.asc == 0x20 &&
-	    sense.ascq == 0x00) {
+	if (ret && sshdr.sense_key == 0x05 &&
+	    sshdr.asc == 0x20 &&
+	    sshdr.ascq == 0x00) {
 		/*
 		 * SCSI-II devices are not required to support
 		 * READ_CD, so let's try switching block size
@@ -2986,7 +2986,7 @@ static noinline int mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi,
 		ret = cdrom_switch_blocksize(cdi, blocksize);
 		if (ret)
 			goto out;
-		cgc->sense = NULL;
+		cgc->sshdr = NULL;
 		ret = cdrom_read_cd(cdi, cgc, lba, blocksize, 1);
 		ret |= cdrom_switch_blocksize(cdi, blocksize);
 	}
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 1d5b35312e33..cb90560acf6e 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -419,7 +419,7 @@ static void ide_cd_request_sense_fixup(ide_drive_t *drive, struct ide_cmd *cmd)
 
 int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,
 		    int write, void *buffer, unsigned *bufflen,
-		    struct request_sense *sense, int timeout,
+		    struct scsi_sense_hdr *sshdr, int timeout,
 		    req_flags_t rq_flags)
 {
 	struct cdrom_info *info = drive->driver_data;
@@ -456,8 +456,9 @@ int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,
 
 		if (buffer)
 			*bufflen = scsi_req(rq)->resid_len;
-		if (sense)
-			memcpy(sense, scsi_req(rq)->sense, sizeof(*sense));
+		if (sshdr)
+			scsi_normalize_sense(scsi_req(rq)->sense,
+					     scsi_req(rq)->sense_len, sshdr);
 
 		/*
 		 * FIXME: we should probably abort/retry or something in case of
@@ -864,7 +865,7 @@ static void msf_from_bcd(struct atapi_msf *msf)
 	msf->frame  = bcd2bin(msf->frame);
 }
 
-int cdrom_check_status(ide_drive_t *drive, struct request_sense *sense)
+int cdrom_check_status(ide_drive_t *drive, struct scsi_sense_hdr *sshdr)
 {
 	struct cdrom_info *info = drive->driver_data;
 	struct cdrom_device_info *cdi;
@@ -886,7 +887,7 @@ int cdrom_check_status(ide_drive_t *drive, struct request_sense *sense)
 	 */
 	cmd[7] = cdi->sanyo_slot % 3;
 
-	return ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, sense, 0, RQF_QUIET);
+	return ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, sshdr, 0, RQF_QUIET);
 }
 
 static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
diff --git a/drivers/ide/ide-cd.h b/drivers/ide/ide-cd.h
index fc162fbb6629..363e81840c4d 100644
--- a/drivers/ide/ide-cd.h
+++ b/drivers/ide/ide-cd.h
@@ -7,6 +7,7 @@
 #define _IDE_CD_H
 
 #include <linux/cdrom.h>
+#include <scsi/scsi_sense.h>
 #include <asm/byteorder.h>
 
 #define IDECD_DEBUG_LOG		0
@@ -98,11 +99,11 @@ void ide_cd_log_error(const char *, struct request *, struct request_sense *);
 
 /* ide-cd.c functions used by ide-cd_ioctl.c */
 int ide_cd_queue_pc(ide_drive_t *, const unsigned char *, int, void *,
-		    unsigned *, struct request_sense *, int, req_flags_t);
+		    unsigned *, struct scsi_sense_hdr *, int, req_flags_t);
 int ide_cd_read_toc(ide_drive_t *);
 int ide_cdrom_get_capabilities(ide_drive_t *, u8 *);
 void ide_cdrom_update_speed(ide_drive_t *, u8 *);
-int cdrom_check_status(ide_drive_t *, struct request_sense *);
+int cdrom_check_status(ide_drive_t *, struct scsi_sense_hdr *);
 
 /* ide-cd_ioctl.c */
 int ide_cdrom_open_real(struct cdrom_device_info *, int);
diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
index c709f4eed0e9..c945beb9e75a 100644
--- a/drivers/ide/ide-cd_ioctl.c
+++ b/drivers/ide/ide-cd_ioctl.c
@@ -43,14 +43,14 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
 {
 	ide_drive_t *drive = cdi->handle;
 	struct media_event_desc med;
-	struct request_sense sense;
+	struct scsi_sense_hdr sshdr;
 	int stat;
 
 	if (slot_nr != CDSL_CURRENT)
 		return -EINVAL;
 
-	stat = cdrom_check_status(drive, &sense);
-	if (!stat || sense.sense_key == UNIT_ATTENTION)
+	stat = cdrom_check_status(drive, &sshdr);
+	if (!stat || sshdr.sense_key == UNIT_ATTENTION)
 		return CDS_DISC_OK;
 
 	if (!cdrom_get_media_event(cdi, &med)) {
@@ -62,8 +62,8 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
 			return CDS_NO_DISC;
 	}
 
-	if (sense.sense_key == NOT_READY && sense.asc == 0x04
-			&& sense.ascq == 0x04)
+	if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04
+			&& sshdr.ascq == 0x04)
 		return CDS_DISC_OK;
 
 	/*
@@ -71,8 +71,8 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
 	 * just return TRAY_OPEN since ATAPI doesn't provide
 	 * any other way to detect this...
 	 */
-	if (sense.sense_key == NOT_READY) {
-		if (sense.asc == 0x3a && sense.ascq == 1)
+	if (sshdr.sense_key == NOT_READY) {
+		if (sshdr.asc == 0x3a && sshdr.ascq == 1)
 			return CDS_NO_DISC;
 		else
 			return CDS_TRAY_OPEN;
@@ -135,7 +135,7 @@ int cdrom_eject(ide_drive_t *drive, int ejectflag)
 static
 int ide_cd_lockdoor(ide_drive_t *drive, int lockflag)
 {
-	struct request_sense my_sense, *sense = &my_sense;
+	struct scsi_sense_hdr sshdr;
 	int stat;
 
 	/* If the drive cannot lock the door, just pretend. */
@@ -150,14 +150,14 @@ int ide_cd_lockdoor(ide_drive_t *drive, int lockflag)
 		cmd[4] = lockflag ? 1 : 0;
 
 		stat = ide_cd_queue_pc(drive, cmd, 0, NULL, NULL,
-				       sense, 0, 0);
+				       &sshdr, 0, 0);
 	}
 
 	/* If we got an illegal field error, the drive
 	   probably cannot lock the door. */
 	if (stat != 0 &&
-	    sense->sense_key == ILLEGAL_REQUEST &&
-	    (sense->asc == 0x24 || sense->asc == 0x20)) {
+	    sshdr.sense_key == ILLEGAL_REQUEST &&
+	    (sshdr.asc == 0x24 || sshdr.asc == 0x20)) {
 		printk(KERN_ERR "%s: door locking not supported\n",
 			drive->name);
 		drive->dev_flags &= ~IDE_DFLAG_DOORLOCKING;
@@ -165,7 +165,7 @@ int ide_cd_lockdoor(ide_drive_t *drive, int lockflag)
 	}
 
 	/* no medium, that's alright. */
-	if (stat != 0 && sense->sense_key == NOT_READY && sense->asc == 0x3a)
+	if (stat != 0 && sshdr.sense_key == NOT_READY && sshdr.asc == 0x3a)
 		stat = 0;
 
 	if (stat == 0) {
@@ -451,8 +451,8 @@ int ide_cdrom_packet(struct cdrom_device_info *cdi,
 	   layer. the packet must be complete, as we do not
 	   touch it at all. */
 
-	if (cgc->sense)
-		memset(cgc->sense, 0, sizeof(struct request_sense));
+	if (cgc->sshdr)
+		memset(cgc->sshdr, 0, sizeof(*cgc->sshdr));
 
 	if (cgc->quiet)
 		flags |= RQF_QUIET;
@@ -460,7 +460,7 @@ int ide_cdrom_packet(struct cdrom_device_info *cdi,
 	cgc->stat = ide_cd_queue_pc(drive, cgc->cmd,
 				    cgc->data_direction == CGC_DATA_WRITE,
 				    cgc->buffer, &len,
-				    cgc->sense, cgc->timeout, flags);
+				    cgc->sshdr, cgc->timeout, flags);
 	if (!cgc->stat)
 		cgc->buflen -= len;
 	return cgc->stat;
diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index 35fab1e18adc..ffcf902da390 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -186,14 +186,13 @@ static int sr_play_trkind(struct cdrom_device_info *cdi,
 int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
 {
 	struct scsi_device *SDev;
-	struct scsi_sense_hdr sshdr;
+	struct scsi_sense_hdr local_sshdr, *sshdr = &local_sshdr;
 	int result, err = 0, retries = 0;
-	unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE], *senseptr = NULL;
 
 	SDev = cd->device;
 
-	if (cgc->sense)
-		senseptr = sense_buffer;
+	if (cgc->sshdr)
+		sshdr = cgc->sshdr;
 
       retry:
 	if (!scsi_block_when_processing_errors(SDev)) {
@@ -202,15 +201,12 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
 	}
 
 	result = scsi_execute(SDev, cgc->cmd, cgc->data_direction,
-			      cgc->buffer, cgc->buflen, senseptr, &sshdr,
+			      cgc->buffer, cgc->buflen, NULL, sshdr,
 			      cgc->timeout, IOCTL_RETRIES, 0, 0, NULL);
 
-	if (cgc->sense)
-		memcpy(cgc->sense, sense_buffer, sizeof(*cgc->sense));
-
 	/* Minimal error checking.  Ignore cases we know about, and report the rest. */
 	if (driver_byte(result) != 0) {
-		switch (sshdr.sense_key) {
+		switch (sshdr->sense_key) {
 		case UNIT_ATTENTION:
 			SDev->changed = 1;
 			if (!cgc->quiet)
@@ -221,8 +217,8 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
 			err = -ENOMEDIUM;
 			break;
 		case NOT_READY:	/* This happens if there is no disc in drive */
-			if (sshdr.asc == 0x04 &&
-			    sshdr.ascq == 0x01) {
+			if (sshdr->asc == 0x04 &&
+			    sshdr->ascq == 0x01) {
 				/* sense: Logical unit is in process of becoming ready */
 				if (!cgc->quiet)
 					sr_printk(KERN_INFO, cd,
@@ -245,8 +241,8 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
 			break;
 		case ILLEGAL_REQUEST:
 			err = -EIO;
-			if (sshdr.asc == 0x20 &&
-			    sshdr.ascq == 0x00)
+			if (sshdr->asc == 0x20 &&
+			    sshdr->ascq == 0x00)
 				/* sense: Invalid command operation code */
 				err = -EDRIVE_CANT_DO_THIS;
 			break;
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index e75dfd1f1dec..528271c60018 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -13,6 +13,7 @@
 
 #include <linux/fs.h>		/* not really needed, later.. */
 #include <linux/list.h>
+#include <scsi/scsi_common.h>
 #include <uapi/linux/cdrom.h>
 
 struct packet_command
@@ -21,7 +22,7 @@ struct packet_command
 	unsigned char 		*buffer;
 	unsigned int 		buflen;
 	int			stat;
-	struct request_sense	*sense;
+	struct scsi_sense_hdr	*sshdr;
 	unsigned char		data_direction;
 	int			quiet;
 	int			timeout;
-- 
2.17.0

[PATCH 5/6] libata-scsi: Move sense buffers onto stack

[Kees Cook]

Instead of dynamically allocating the sense buffers, put them on the
stack so that future compile-time sizeof() checks will be able to see
their buffer length.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/ata/libata-scsi.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 89a9d4a2efc8..3a43d3a1ce2d 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -597,8 +597,9 @@ static int ata_get_identity(struct ata_port *ap, struct scsi_device *sdev,
 int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 {
 	int rc = 0;
+	u8 sensebuf[SCSI_SENSE_BUFFERSIZE];
 	u8 scsi_cmd[MAX_COMMAND_SIZE];
-	u8 args[4], *argbuf = NULL, *sensebuf = NULL;
+	u8 args[4], *argbuf = NULL;
 	int argsize = 0;
 	enum dma_data_direction data_dir;
 	struct scsi_sense_hdr sshdr;
@@ -610,10 +611,7 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 	if (copy_from_user(args, arg, sizeof(args)))
 		return -EFAULT;
 
-	sensebuf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
-	if (!sensebuf)
-		return -ENOMEM;
-
+	memset(sensebuf, 0, sizeof(sensebuf));
 	memset(scsi_cmd, 0, sizeof(scsi_cmd));
 
 	if (args[3]) {
@@ -685,7 +683,6 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 	 && copy_to_user(arg + sizeof(args), argbuf, argsize))
 		rc = -EFAULT;
 error:
-	kfree(sensebuf);
 	kfree(argbuf);
 	return rc;
 }
@@ -704,8 +701,9 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
 {
 	int rc = 0;
+	u8 sensebuf[SCSI_SENSE_BUFFERSIZE];
 	u8 scsi_cmd[MAX_COMMAND_SIZE];
-	u8 args[7], *sensebuf = NULL;
+	u8 args[7];
 	struct scsi_sense_hdr sshdr;
 	int cmd_result;
 
@@ -715,10 +713,7 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
 	if (copy_from_user(args, arg, sizeof(args)))
 		return -EFAULT;
 
-	sensebuf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
-	if (!sensebuf)
-		return -ENOMEM;
-
+	memset(sensebuf, 0, sizeof(sensebuf));
 	memset(scsi_cmd, 0, sizeof(scsi_cmd));
 	scsi_cmd[0]  = ATA_16;
 	scsi_cmd[1]  = (3 << 1); /* Non-data */
@@ -769,7 +764,6 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
 	}
 
  error:
-	kfree(sensebuf);
 	return rc;
 }
 
-- 
2.17.0

[PATCH 6/6] scsi: Check sense buffer size at build time

[Kees Cook]

To avoid introducing problems like those fixed in commit f7068114d45e
("sr: pass down correctly sized SCSI sense buffer"), this creates a macro
wrapper for scsi_execute() that verifies the size of the sense buffer
similar to what was done for command string sizes in commit 3756f6401c30
("exec: avoid gcc-8 warning for get_task_comm").

Another solution could be to add another argument to scsi_execute(),
but this function already takes a lot of arguments and Jens was not fond
of that approach. As there was only a pair of dynamically allocated sense
buffers, this also moves those 96 bytes onto the stack to avoid triggering
the sizeof() check.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/scsi_lib.c    |  6 +++---
 include/scsi/scsi_device.h | 12 +++++++++++-
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e9b4f279d29c..718c2bec4516 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -238,7 +238,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
 
 
 /**
- * scsi_execute - insert request and wait for the result
+ * __scsi_execute - insert request and wait for the result
  * @sdev:	scsi device
  * @cmd:	scsi command
  * @data_direction: data direction
@@ -255,7 +255,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
  * Returns the scsi_cmnd result field if a command was executed, or a negative
  * Linux error code if we didn't get that far.
  */
-int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 		 int data_direction, void *buffer, unsigned bufflen,
 		 unsigned char *sense, struct scsi_sense_hdr *sshdr,
 		 int timeout, int retries, u64 flags, req_flags_t rq_flags,
@@ -309,7 +309,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 
 	return ret;
 }
-EXPORT_SYMBOL(scsi_execute);
+EXPORT_SYMBOL(__scsi_execute);
 
 /*
  * Function:    scsi_init_cmd_errh()
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7ae177c8e399..1bb87b6c0ad2 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -426,11 +426,21 @@ extern const char *scsi_device_state_name(enum scsi_device_state);
 extern int scsi_is_sdev_device(const struct device *);
 extern int scsi_is_target_device(const struct device *);
 extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
-extern int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+extern int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 			int data_direction, void *buffer, unsigned bufflen,
 			unsigned char *sense, struct scsi_sense_hdr *sshdr,
 			int timeout, int retries, u64 flags,
 			req_flags_t rq_flags, int *resid);
+/* Make sure any sense buffer is the correct size. */
+#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,	\
+		     sshdr, timeout, retries, flags, rq_flags, resid)	\
+({									\
+	BUILD_BUG_ON((sense) != NULL &&					\
+		     sizeof(sense) != SCSI_SENSE_BUFFERSIZE);		\
+	__scsi_execute(sdev, cmd, data_direction, buffer, bufflen,	\
+		       sense, sshdr, timeout, retries, flags, rq_flags,	\
+		       resid);						\
+})
 static inline int scsi_execute_req(struct scsi_device *sdev,
 	const unsigned char *cmd, int data_direction, void *buffer,
 	unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout,
-- 
2.17.0

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Christoph Hellwig]

On Tue, May 22, 2018 at 11:15:09AM -0700, Kees Cook wrote:
> Both SCSI and ATAPI share the sense header. In preparation for using the
> struct scsi_sense_hdr more widely, move this into a separate header and
> move the helper function to scsi_ioctl.c which is linked with CONFIG_IDE
> by way of CONFIG_BLK_SCSI_REQUEST.

Please keep the code where it is and just depend on SCSI on the legacy
ide driver.  No need to do gymnastics just for a legacy case.

Re: [PATCH 1/6] ide-cd: Drop unused sense buffers

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/6] scsi: cxlflash: Drop unused sense buffers

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Martin K. Petersen]

Christoph,

> On Tue, May 22, 2018 at 11:15:09AM -0700, Kees Cook wrote:
>> Both SCSI and ATAPI share the sense header. In preparation for using the
>> struct scsi_sense_hdr more widely, move this into a separate header and
>> move the helper function to scsi_ioctl.c which is linked with CONFIG_IDE
>> by way of CONFIG_BLK_SCSI_REQUEST.
>
> Please keep the code where it is and just depend on SCSI on the legacy
> ide driver.  No need to do gymnastics just for a legacy case.

Yup, I agree.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Tue, May 22, 2018 at 11:50 AM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
> Christoph,
>
>> On Tue, May 22, 2018 at 11:15:09AM -0700, Kees Cook wrote:
>>> Both SCSI and ATAPI share the sense header. In preparation for using the
>>> struct scsi_sense_hdr more widely, move this into a separate header and
>>> move the helper function to scsi_ioctl.c which is linked with CONFIG_IDE
>>> by way of CONFIG_BLK_SCSI_REQUEST.
>>
>> Please keep the code where it is and just depend on SCSI on the legacy
>> ide driver.  No need to do gymnastics just for a legacy case.
>
> Yup, I agree.

Oh, er, this was mainly done at Jens's request. Jens, can you advise?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On 5/22/18 12:59 PM, Kees Cook wrote:
> On Tue, May 22, 2018 at 11:50 AM, Martin K. Petersen
> <martin.petersen@oracle.com> wrote:
>>
>> Christoph,
>>
>>> On Tue, May 22, 2018 at 11:15:09AM -0700, Kees Cook wrote:
>>>> Both SCSI and ATAPI share the sense header. In preparation for using the
>>>> struct scsi_sense_hdr more widely, move this into a separate header and
>>>> move the helper function to scsi_ioctl.c which is linked with CONFIG_IDE
>>>> by way of CONFIG_BLK_SCSI_REQUEST.
>>>
>>> Please keep the code where it is and just depend on SCSI on the legacy
>>> ide driver.  No need to do gymnastics just for a legacy case.
>>
>> Yup, I agree.
> 
> Oh, er, this was mainly done at Jens's request. Jens, can you advise?

I think Martin and Christoph are objecting to moving the code to
block/scsi_ioctl.h. I don't care too much about where the code is, but
think it would be nice to have the definitions in a separate header. But
if they prefer just pulling in all of SCSI for it, well then I guess
it's pointless to move the header bits. Seems very heavy handed to me,
though.

-- 
Jens Axboe

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Christoph Hellwig]

On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
> I think Martin and Christoph are objecting to moving the code to
> block/scsi_ioctl.h. I don't care too much about where the code is, but
> think it would be nice to have the definitions in a separate header. But
> if they prefer just pulling in all of SCSI for it, well then I guess
> it's pointless to move the header bits. Seems very heavy handed to me,
> though.

It might be heavy handed for the 3 remaining users of drivers/ide,
but as long as that stuff just keeps working I'd rather worry about
everyone else, and keep the scsi code where it belongs.

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On 5/22/18 1:13 PM, Christoph Hellwig wrote:
> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>> I think Martin and Christoph are objecting to moving the code to
>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>> think it would be nice to have the definitions in a separate header. But
>> if they prefer just pulling in all of SCSI for it, well then I guess
>> it's pointless to move the header bits. Seems very heavy handed to me,
>> though.
> 
> It might be heavy handed for the 3 remaining users of drivers/ide,

Brutal :-)

> but as long as that stuff just keeps working I'd rather worry about
> everyone else, and keep the scsi code where it belongs.

Fine with me then, hopefully we can some day kill it off.

-- 
Jens Axboe

Re: [PATCH 2/6] scsi: cxlflash: Drop unused sense buffers

[Matthew R. Ochs]

On Tue, May 22, 2018 at 11:15:08AM -0700, Kees Cook wrote:
> This removes the unused sense buffer in read_cap16() and write_same16().
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Looks good.

Acked-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Tue, May 22, 2018 at 12:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>>> I think Martin and Christoph are objecting to moving the code to
>>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>>> think it would be nice to have the definitions in a separate header. But
>>> if they prefer just pulling in all of SCSI for it, well then I guess
>>> it's pointless to move the header bits. Seems very heavy handed to me,
>>> though.
>>
>> It might be heavy handed for the 3 remaining users of drivers/ide,
>
> Brutal :-)

Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
too. Is this okay under the same considerations?

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index ad9b687a236a..220ff321c102 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -79,7 +79,7 @@ config GDROM
        tristate "SEGA Dreamcast GD-ROM drive"
        depends on SH_DREAMCAST
        select CDROM
-       select BLK_SCSI_REQUEST # only for the generic cdrom code
+       select SCSI
        help
          A standard SEGA Dreamcast comes with a modified CD ROM drive called a
          "GD-ROM" by SEGA to signify it is capable of reading special disks
@@ -345,7 +345,7 @@ config CDROM_PKTCDVD
        tristate "Packet writing on CD/DVD media (DEPRECATED)"
        depends on !UML
        select CDROM
-       select BLK_SCSI_REQUEST
+       select SCSI
        help
          Note: This driver is deprecated and will be removed from the
          kernel in the near future!
diff --git a/drivers/block/paride/Kconfig b/drivers/block/paride/Kconfig
index f8bd6ef3605a..7fdfcc5eaca5 100644
--- a/drivers/block/paride/Kconfig
+++ b/drivers/block/paride/Kconfig
@@ -27,7 +27,7 @@ config PARIDE_PCD
        tristate "Parallel port ATAPI CD-ROMs"
        depends on PARIDE
        select CDROM
-       select BLK_SCSI_REQUEST # only for the generic cdrom code
+       select SCSI
        ---help---
          This option enables the high-level driver for ATAPI CD-ROM devices
          connected through a parallel port. If you chose to build PARIDE

>> but as long as that stuff just keeps working I'd rather worry about
>> everyone else, and keep the scsi code where it belongs.
>
> Fine with me then, hopefully we can some day kill it off.

I'll send a v2. I found a few other things to fix up (including the
cdrom.c one).

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Randy Dunlap]

On 05/22/2018 04:31 PM, Kees Cook wrote:
> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
>>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>>>> I think Martin and Christoph are objecting to moving the code to
>>>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>>>> think it would be nice to have the definitions in a separate header. But
>>>> if they prefer just pulling in all of SCSI for it, well then I guess
>>>> it's pointless to move the header bits. Seems very heavy handed to me,
>>>> though.
>>>
>>> It might be heavy handed for the 3 remaining users of drivers/ide,
>>
>> Brutal :-)
> 
> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
> too. Is this okay under the same considerations?

No.  Do not select an entire subsystem.  Use depends on it instead.


> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
> index ad9b687a236a..220ff321c102 100644
> --- a/drivers/block/Kconfig
> +++ b/drivers/block/Kconfig
> @@ -79,7 +79,7 @@ config GDROM
>         tristate "SEGA Dreamcast GD-ROM drive"
>         depends on SH_DREAMCAST
>         select CDROM
> -       select BLK_SCSI_REQUEST # only for the generic cdrom code
> +       select SCSI
>         help
>           A standard SEGA Dreamcast comes with a modified CD ROM drive called a
>           "GD-ROM" by SEGA to signify it is capable of reading special disks
> @@ -345,7 +345,7 @@ config CDROM_PKTCDVD
>         tristate "Packet writing on CD/DVD media (DEPRECATED)"
>         depends on !UML
>         select CDROM
> -       select BLK_SCSI_REQUEST
> +       select SCSI
>         help
>           Note: This driver is deprecated and will be removed from the
>           kernel in the near future!
> diff --git a/drivers/block/paride/Kconfig b/drivers/block/paride/Kconfig
> index f8bd6ef3605a..7fdfcc5eaca5 100644
> --- a/drivers/block/paride/Kconfig
> +++ b/drivers/block/paride/Kconfig
> @@ -27,7 +27,7 @@ config PARIDE_PCD
>         tristate "Parallel port ATAPI CD-ROMs"
>         depends on PARIDE
>         select CDROM
> -       select BLK_SCSI_REQUEST # only for the generic cdrom code
> +       select SCSI
>         ---help---
>           This option enables the high-level driver for ATAPI CD-ROM devices
>           connected through a parallel port. If you chose to build PARIDE
> 
>>> but as long as that stuff just keeps working I'd rather worry about
>>> everyone else, and keep the scsi code where it belongs.
>>
>> Fine with me then, hopefully we can some day kill it off.
> 
> I'll send a v2. I found a few other things to fix up (including the
> cdrom.c one).


-- 
~Randy

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Tue, May 22, 2018 at 4:34 PM, Randy Dunlap <rdunlap@infradead.org> wrote:
> On 05/22/2018 04:31 PM, Kees Cook wrote:
>> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
>>>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>>>>> I think Martin and Christoph are objecting to moving the code to
>>>>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>>>>> think it would be nice to have the definitions in a separate header. But
>>>>> if they prefer just pulling in all of SCSI for it, well then I guess
>>>>> it's pointless to move the header bits. Seems very heavy handed to me,
>>>>> though.
>>>>
>>>> It might be heavy handed for the 3 remaining users of drivers/ide,
>>>
>>> Brutal :-)
>>
>> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
>> too. Is this okay under the same considerations?
>
> No.  Do not select an entire subsystem.  Use depends on it instead.

I looked at that first, but it seems it's designed for that. For
example, "config ATA" already has a "select SCSI".

It does look fishy, though, since "config SCSI" has a "depends" which
would be ignored by "select". Luckily, all these uses already do a
"depends on BLOCK" (directly or indirectly).

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Randy Dunlap]

On 05/22/2018 04:39 PM, Kees Cook wrote:
> On Tue, May 22, 2018 at 4:34 PM, Randy Dunlap <rdunlap@infradead.org> wrote:
>> On 05/22/2018 04:31 PM, Kees Cook wrote:
>>> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
>>>>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>>>>>> I think Martin and Christoph are objecting to moving the code to
>>>>>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>>>>>> think it would be nice to have the definitions in a separate header. But
>>>>>> if they prefer just pulling in all of SCSI for it, well then I guess
>>>>>> it's pointless to move the header bits. Seems very heavy handed to me,
>>>>>> though.
>>>>>
>>>>> It might be heavy handed for the 3 remaining users of drivers/ide,
>>>>
>>>> Brutal :-)
>>>
>>> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
>>> too. Is this okay under the same considerations?
>>
>> No.  Do not select an entire subsystem.  Use depends on it instead.
> 
> I looked at that first, but it seems it's designed for that. For
> example, "config ATA" already has a "select SCSI".
> 
> It does look fishy, though, since "config SCSI" has a "depends" which
> would be ignored by "select". Luckily, all these uses already do a
> "depends on BLOCK" (directly or indirectly).

Linus has railed against selecting subsystems.  We shouldn't be adding
more IMHO, although it is difficult to get rid of ones that we already have.


-- 
~Randy

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On May 22, 2018, at 5:31 PM, Kees Cook <keescook@chromium.org> wrote:
>=20
>> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
>>>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>>>> I think Martin and Christoph are objecting to moving the code to
>>>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>>>> think it would be nice to have the definitions in a separate header. Bu=
t
>>>> if they prefer just pulling in all of SCSI for it, well then I guess
>>>> it's pointless to move the header bits. Seems very heavy handed to me,
>>>> though.
>>>=20
>>> It might be heavy handed for the 3 remaining users of drivers/ide,
>>=20
>> Brutal :-)
>=20
> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
> too. Is this okay under the same considerations?

This is going from somewhat crazy to pretty nuts, imho. I guess in practical=
 terms it doesn=E2=80=99t matter that much, since most folks would be using s=
r. I still think a split would be vastly superior. Just keep the scsi code i=
n drivers/scsi/ and make it independently selectable.

Jens

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On May 22, 2018, at 5:31 PM, Kees Cook <keescook@chromium.org> wrote:
> 
>> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
>>>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>>>> I think Martin and Christoph are objecting to moving the code to
>>>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>>>> think it would be nice to have the definitions in a separate header. But
>>>> if they prefer just pulling in all of SCSI for it, well then I guess
>>>> it's pointless to move the header bits. Seems very heavy handed to me,
>>>> though.
>>> 
>>> It might be heavy handed for the 3 remaining users of drivers/ide,
>> 
>> Brutal :-)
> 
> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
> too. Is this okay under the same considerations?

This is going from somewhat crazy to pretty nuts, imho. I guess in practical terms it doesn’t matter that much, since most folks would be using sr. I still think a split would be vastly superior. Just keep the scsi code in drivers/scsi/ and make it independently selectable.

Jens

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Tue, May 22, 2018 at 4:42 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On May 22, 2018, at 5:31 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>>> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
>>>>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>>>>> I think Martin and Christoph are objecting to moving the code to
>>>>> block/scsi_ioctl.h. I don't care too much about where the code is, bu=
t
>>>>> think it would be nice to have the definitions in a separate header. =
But
>>>>> if they prefer just pulling in all of SCSI for it, well then I guess
>>>>> it's pointless to move the header bits. Seems very heavy handed to me=
,
>>>>> though.
>>>>
>>>> It might be heavy handed for the 3 remaining users of drivers/ide,
>>>
>>> Brutal :-)
>>
>> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
>> too. Is this okay under the same considerations?
>
> This is going from somewhat crazy to pretty nuts, imho. I guess in practi=
cal terms it doesn=E2=80=99t matter that much, since most folks would be us=
ing sr. I still think a split would be vastly superior. Just keep the scsi =
code in drivers/scsi/ and make it independently selectable.

I had originally tied it to BLK_SCSI_REQUEST. Logically speaking,
sense buffers are part of the request, and the CONFIG work is already
done. This is roughly what I tried to do before, since scsi_ioctl.c is
the only code pulled in for CONFIG_BLK_SCSI_REQUEST:

$ git grep BLK_SCSI_REQUEST | grep Makefile
block/Makefile:obj-$(CONFIG_BLK_SCSI_REQUEST)   +=3D scsi_ioctl.o

Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
drivers/scsi/Makefile as:

obj-$(CONFIG_BLK_SCSI_REQUEST)    +=3D scsi_sense.o

Every place I want to use the code is already covered by
CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
put the .c file. :P

-Kees

--=20
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Tue, May 22, 2018 at 4:42 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On May 22, 2018, at 5:31 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>>> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
>>>>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>>>>> I think Martin and Christoph are objecting to moving the code to
>>>>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>>>>> think it would be nice to have the definitions in a separate header. But
>>>>> if they prefer just pulling in all of SCSI for it, well then I guess
>>>>> it's pointless to move the header bits. Seems very heavy handed to me,
>>>>> though.
>>>>
>>>> It might be heavy handed for the 3 remaining users of drivers/ide,
>>>
>>> Brutal :-)
>>
>> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
>> too. Is this okay under the same considerations?
>
> This is going from somewhat crazy to pretty nuts, imho. I guess in practical terms it doesn’t matter that much, since most folks would be using sr. I still think a split would be vastly superior. Just keep the scsi code in drivers/scsi/ and make it independently selectable.

I had originally tied it to BLK_SCSI_REQUEST. Logically speaking,
sense buffers are part of the request, and the CONFIG work is already
done. This is roughly what I tried to do before, since scsi_ioctl.c is
the only code pulled in for CONFIG_BLK_SCSI_REQUEST:

$ git grep BLK_SCSI_REQUEST | grep Makefile
block/Makefile:obj-$(CONFIG_BLK_SCSI_REQUEST)   += scsi_ioctl.o

Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
drivers/scsi/Makefile as:

obj-$(CONFIG_BLK_SCSI_REQUEST)    += scsi_sense.o

Every place I want to use the code is already covered by
CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
put the .c file. :P

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 6/6] scsi: Check sense buffer size at build time

[Sergei Shtylyov]

Hello!

On 5/22/2018 9:15 PM, Kees Cook wrote:

> To avoid introducing problems like those fixed in commit f7068114d45e
> ("sr: pass down correctly sized SCSI sense buffer"), this creates a macro
> wrapper for scsi_execute() that verifies the size of the sense buffer
> similar to what was done for command string sizes in commit 3756f6401c30
> ("exec: avoid gcc-8 warning for get_task_comm").
> 
> Another solution could be to add another argument to scsi_execute(),
> but this function already takes a lot of arguments and Jens was not fond
> of that approach. As there was only a pair of dynamically allocated sense
> buffers, this also moves those 96 bytes onto the stack to avoid triggering
> the sizeof() check.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>   drivers/scsi/scsi_lib.c    |  6 +++---
>   include/scsi/scsi_device.h | 12 +++++++++++-
>   2 files changed, 14 insertions(+), 4 deletions(-)
> 
[...]
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 7ae177c8e399..1bb87b6c0ad2 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -426,11 +426,21 @@ extern const char *scsi_device_state_name(enum scsi_device_state);
>   extern int scsi_is_sdev_device(const struct device *);
>   extern int scsi_is_target_device(const struct device *);
>   extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
> -extern int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
> +extern int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
>   			int data_direction, void *buffer, unsigned bufflen,
>   			unsigned char *sense, struct scsi_sense_hdr *sshdr,
>   			int timeout, int retries, u64 flags,
>   			req_flags_t rq_flags, int *resid);
> +/* Make sure any sense buffer is the correct size. */
> +#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,	\
> +		     sshdr, timeout, retries, flags, rq_flags, resid)	\
> +({									\
> +	BUILD_BUG_ON((sense) != NULL &&					\
> +		     sizeof(sense) != SCSI_SENSE_BUFFERSIZE);		\

    This would only check the size of the 'sense' pointer, no?

> +	__scsi_execute(sdev, cmd, data_direction, buffer, bufflen,	\
> +		       sense, sshdr, timeout, retries, flags, rq_flags,	\
> +		       resid);						\
> +})
>   static inline int scsi_execute_req(struct scsi_device *sdev,
>   	const unsigned char *cmd, int data_direction, void *buffer,
>   	unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout,

MBR, Sergei

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On 5/22/18 5:49 PM, Kees Cook wrote:
> On Tue, May 22, 2018 at 4:42 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On May 22, 2018, at 5:31 PM, Kees Cook <keescook@chromium.org> wrote:
>>>
>>>> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
>>>>>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>>>>>> I think Martin and Christoph are objecting to moving the code to
>>>>>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>>>>>> think it would be nice to have the definitions in a separate header. But
>>>>>> if they prefer just pulling in all of SCSI for it, well then I guess
>>>>>> it's pointless to move the header bits. Seems very heavy handed to me,
>>>>>> though.
>>>>>
>>>>> It might be heavy handed for the 3 remaining users of drivers/ide,
>>>>
>>>> Brutal :-)
>>>
>>> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
>>> too. Is this okay under the same considerations?
>>
>> This is going from somewhat crazy to pretty nuts, imho. I guess in practical terms it doesn’t matter that much, since most folks would be using sr. I still think a split would be vastly superior. Just keep the scsi code in drivers/scsi/ and make it independently selectable.
> 
> I had originally tied it to BLK_SCSI_REQUEST. Logically speaking,
> sense buffers are part of the request, and the CONFIG work is already
> done. This is roughly what I tried to do before, since scsi_ioctl.c is
> the only code pulled in for CONFIG_BLK_SCSI_REQUEST:
> 
> $ git grep BLK_SCSI_REQUEST | grep Makefile
> block/Makefile:obj-$(CONFIG_BLK_SCSI_REQUEST)   += scsi_ioctl.o
> 
> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
> drivers/scsi/Makefile as:
> 
> obj-$(CONFIG_BLK_SCSI_REQUEST)    += scsi_sense.o
> 
> Every place I want to use the code is already covered by
> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
> put the .c file. :P

I think this is so much saner than a SCSI select or dependency, so I'll
have to disagree with Martin and Christoph. Just put it in drivers/scsi,
if it's the location they care about.

-- 
Jens Axboe

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Christoph Hellwig]

On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
> > Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
> > drivers/scsi/Makefile as:
> > 
> > obj-$(CONFIG_BLK_SCSI_REQUEST)    += scsi_sense.o
> > 
> > Every place I want to use the code is already covered by
> > CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
> > put the .c file. :P
> 
> I think this is so much saner than a SCSI select or dependency, so I'll
> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
> if it's the location they care about.

I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
or two.  The only users are scsi and the ide layer, (virtio_blk
support has already been accidentally disabled for a while), and getting
rid of it allows to to shrink and simply the scsi data structures.

But if you want this for now lets keep scsi_sense.c in drivers/scsi
but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On 5/23/18 8:25 AM, Christoph Hellwig wrote:
> On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
>>> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
>>> drivers/scsi/Makefile as:
>>>
>>> obj-$(CONFIG_BLK_SCSI_REQUEST)    += scsi_sense.o
>>>
>>> Every place I want to use the code is already covered by
>>> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
>>> put the .c file. :P
>>
>> I think this is so much saner than a SCSI select or dependency, so I'll
>> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
>> if it's the location they care about.
> 
> I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
> or two.  The only users are scsi and the ide layer, (virtio_blk
> support has already been accidentally disabled for a while), and getting
> rid of it allows to to shrink and simply the scsi data structures.
> 
> But if you want this for now lets keep scsi_sense.c in drivers/scsi
> but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.

It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST.
BLA_SCSI_SENSE or something would do. I don't care too much about that,
mostly getting rid of the entire stack dependency.

-- 
Jens Axboe

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Wed, May 23, 2018 at 7:31 AM, Jens Axboe <axboe@kernel.dk> wrote:
> On 5/23/18 8:25 AM, Christoph Hellwig wrote:
>> On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
>>>> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
>>>> drivers/scsi/Makefile as:
>>>>
>>>> obj-$(CONFIG_BLK_SCSI_REQUEST)    += scsi_sense.o
>>>>
>>>> Every place I want to use the code is already covered by
>>>> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
>>>> put the .c file. :P
>>>
>>> I think this is so much saner than a SCSI select or dependency, so I'll
>>> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
>>> if it's the location they care about.
>>
>> I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
>> or two.  The only users are scsi and the ide layer, (virtio_blk
>> support has already been accidentally disabled for a while), and getting
>> rid of it allows to to shrink and simply the scsi data structures.
>>
>> But if you want this for now lets keep scsi_sense.c in drivers/scsi
>> but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.
>
> It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST.
> BLA_SCSI_SENSE or something would do. I don't care too much about that,
> mostly getting rid of the entire stack dependency.

Aaand, I can't do this and leave it in drivers/scsi because of drivers/Makefile:

obj-$(CONFIG_SCSI)              += scsi/

So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
still need to move the code from drivers/scsi/ to block/. Is this
okay?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Martin K. Petersen]

Kees,

> obj-$(CONFIG_SCSI)              += scsi/
>
> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
> still need to move the code from drivers/scsi/ to block/. Is this
> okay?

The reason this sucks is that scsi_normalize_sense() is an inherent core
feature in the SCSI layer. Dealing with sense data for ioctls is just a
fringe use case.

I don't want to get too hung up on what goes where. But architecturally
it really irks me to move a core piece of SCSI state machine
functionality out of the subsystem to accommodate ioctl handling.

I'm traveling today so I probably won't get a chance to look closely
until tomorrow morning.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 6/6] scsi: Check sense buffer size at build time

[Kees Cook]

On Wed, May 23, 2018 at 1:25 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello!
>
> On 5/22/2018 9:15 PM, Kees Cook wrote:
>
>> To avoid introducing problems like those fixed in commit f7068114d45e
>> ("sr: pass down correctly sized SCSI sense buffer"), this creates a macro
>> wrapper for scsi_execute() that verifies the size of the sense buffer
>> similar to what was done for command string sizes in commit 3756f6401c30
>> ("exec: avoid gcc-8 warning for get_task_comm").
>>
>> Another solution could be to add another argument to scsi_execute(),
>> but this function already takes a lot of arguments and Jens was not fond
>> of that approach. As there was only a pair of dynamically allocated sense
>> buffers, this also moves those 96 bytes onto the stack to avoid triggering
>> the sizeof() check.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>   drivers/scsi/scsi_lib.c    |  6 +++---
>>   include/scsi/scsi_device.h | 12 +++++++++++-
>>   2 files changed, 14 insertions(+), 4 deletions(-)
>>
> [...]
>>
>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>> index 7ae177c8e399..1bb87b6c0ad2 100644
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -426,11 +426,21 @@ extern const char *scsi_device_state_name(enum
>> scsi_device_state);
>>   extern int scsi_is_sdev_device(const struct device *);
>>   extern int scsi_is_target_device(const struct device *);
>>   extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
>> -extern int scsi_execute(struct scsi_device *sdev, const unsigned char
>> *cmd,
>> +extern int __scsi_execute(struct scsi_device *sdev, const unsigned char
>> *cmd,
>>                         int data_direction, void *buffer, unsigned
>> bufflen,
>>                         unsigned char *sense, struct scsi_sense_hdr
>> *sshdr,
>>                         int timeout, int retries, u64 flags,
>>                         req_flags_t rq_flags, int *resid);
>> +/* Make sure any sense buffer is the correct size. */
>> +#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,
>> \
>> +                    sshdr, timeout, retries, flags, rq_flags, resid)   \
>> +({                                                                     \
>> +       BUILD_BUG_ON((sense) != NULL &&                                 \
>> +                    sizeof(sense) != SCSI_SENSE_BUFFERSIZE);           \
>
>
>    This would only check the size of the 'sense' pointer, no?

Correct. Passing in a variable that was declared as:

char sense[SCSI_SENSE_BUFFERSIZE];

would pass this check. Dynamically allocated would not (and we don't
have any cases of that left after the prior patch in this series), and
it should cast improper casts.

If people don't like this bit of robustness vs complexity/limit, I'm
happy to leave it off the series.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On 5/23/18 2:52 PM, Kees Cook wrote:
> On Wed, May 23, 2018 at 7:31 AM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 5/23/18 8:25 AM, Christoph Hellwig wrote:
>>> On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
>>>>> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
>>>>> drivers/scsi/Makefile as:
>>>>>
>>>>> obj-$(CONFIG_BLK_SCSI_REQUEST)    += scsi_sense.o
>>>>>
>>>>> Every place I want to use the code is already covered by
>>>>> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
>>>>> put the .c file. :P
>>>>
>>>> I think this is so much saner than a SCSI select or dependency, so I'll
>>>> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
>>>> if it's the location they care about.
>>>
>>> I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
>>> or two.  The only users are scsi and the ide layer, (virtio_blk
>>> support has already been accidentally disabled for a while), and getting
>>> rid of it allows to to shrink and simply the scsi data structures.
>>>
>>> But if you want this for now lets keep scsi_sense.c in drivers/scsi
>>> but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.
>>
>> It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST.
>> BLA_SCSI_SENSE or something would do. I don't care too much about that,
>> mostly getting rid of the entire stack dependency.
> 
> Aaand, I can't do this and leave it in drivers/scsi because of drivers/Makefile:
> 
> obj-$(CONFIG_SCSI)              += scsi/
> 
> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
> still need to move the code from drivers/scsi/ to block/. Is this
> okay?

Ugh, so that would necessitate a change there too. As I said before,
I don't really care where it lives. I know the SCSI folks seem bothered
by moving it, but in reality, it's not like this stuff will likely ever
really change. Of the two choices (select entire SCSI stack, or just move
this little bit), I know what I would consider the saner option...

-- 
Jens Axboe

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Wed, May 23, 2018 at 2:06 PM, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
> Kees,
>
>> obj-$(CONFIG_SCSI)              += scsi/
>>
>> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
>> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
>> still need to move the code from drivers/scsi/ to block/. Is this
>> okay?
>
> The reason this sucks is that scsi_normalize_sense() is an inherent core
> feature in the SCSI layer. Dealing with sense data for ioctls is just a
> fringe use case.

True, though I'm finding other robustness issues in the CDROM code.
They're probably all insane corner cases, but it seems like it'd be
nice to just fix them:

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 3522d2cae1b6..7726c8618c30 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2222,9 +2223,12 @@ static int cdrom_read_cdda_bpc(struct
cdrom_device_info *cdi, __u8 __user *ubuf,

                blk_execute_rq(q, cdi->disk, rq, 0);
                if (scsi_req(rq)->result) {
-                       struct request_sense *s = req->sense;
+                       struct scsi_sense_hdr sshdr;
+
                        ret = -EIO;
-                       cdi->last_sense = s->sense_key;
+                       scsi_normalize_sense(req->sense, req->sense_len,
+                                            &sshdr);
+                       cdi->last_sense = sshdr.sense_key;
                }

                if (blk_rq_unmap_user(bio))

This code wasn't checking req->sense_len, for example. It'll just get
stale data at worst case, but it's still ugly, especially since we
have a solution to do it right.

> I don't want to get too hung up on what goes where. But architecturally
> it really irks me to move a core piece of SCSI state machine
> functionality out of the subsystem to accommodate ioctl handling.

It looks like there is more in block/scsi_ioctl.c than just ioctl
handling, which is why I put the function in there originally.
Honestly, it's almost so small I could make it a static inline. :P

> I'm traveling today so I probably won't get a chance to look closely
> until tomorrow morning.

No worries; thanks for looking at it!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Randy Dunlap]

On 05/23/2018 02:14 PM, Jens Axboe wrote:
> On 5/23/18 2:52 PM, Kees Cook wrote:
>> On Wed, May 23, 2018 at 7:31 AM, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 5/23/18 8:25 AM, Christoph Hellwig wrote:
>>>> On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
>>>>>> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
>>>>>> drivers/scsi/Makefile as:
>>>>>>
>>>>>> obj-$(CONFIG_BLK_SCSI_REQUEST)    += scsi_sense.o
>>>>>>
>>>>>> Every place I want to use the code is already covered by
>>>>>> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
>>>>>> put the .c file. :P
>>>>>
>>>>> I think this is so much saner than a SCSI select or dependency, so I'll
>>>>> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
>>>>> if it's the location they care about.
>>>>
>>>> I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
>>>> or two.  The only users are scsi and the ide layer, (virtio_blk
>>>> support has already been accidentally disabled for a while), and getting
>>>> rid of it allows to to shrink and simply the scsi data structures.
>>>>
>>>> But if you want this for now lets keep scsi_sense.c in drivers/scsi
>>>> but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.
>>>
>>> It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST.
>>> BLA_SCSI_SENSE or something would do. I don't care too much about that,
>>> mostly getting rid of the entire stack dependency.
>>
>> Aaand, I can't do this and leave it in drivers/scsi because of drivers/Makefile:
>>
>> obj-$(CONFIG_SCSI)              += scsi/
>>
>> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
>> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
>> still need to move the code from drivers/scsi/ to block/. Is this
>> okay?
> 
> Ugh, so that would necessitate a change there too. As I said before,
> I don't really care where it lives. I know the SCSI folks seem bothered
> by moving it, but in reality, it's not like this stuff will likely ever
> really change. Of the two choices (select entire SCSI stack, or just move
> this little bit), I know what I would consider the saner option...
> 

or option 3:

obj-y                           += scsi/

so that make descends into drivers/scsi/ and then builds whatever is needed,
depending on individual kconfig options.

-- 
~Randy

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On 5/23/18 3:20 PM, Randy Dunlap wrote:
> On 05/23/2018 02:14 PM, Jens Axboe wrote:
>> On 5/23/18 2:52 PM, Kees Cook wrote:
>>> On Wed, May 23, 2018 at 7:31 AM, Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 5/23/18 8:25 AM, Christoph Hellwig wrote:
>>>>> On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
>>>>>>> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
>>>>>>> drivers/scsi/Makefile as:
>>>>>>>
>>>>>>> obj-$(CONFIG_BLK_SCSI_REQUEST)    += scsi_sense.o
>>>>>>>
>>>>>>> Every place I want to use the code is already covered by
>>>>>>> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
>>>>>>> put the .c file. :P
>>>>>>
>>>>>> I think this is so much saner than a SCSI select or dependency, so I'll
>>>>>> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
>>>>>> if it's the location they care about.
>>>>>
>>>>> I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
>>>>> or two.  The only users are scsi and the ide layer, (virtio_blk
>>>>> support has already been accidentally disabled for a while), and getting
>>>>> rid of it allows to to shrink and simply the scsi data structures.
>>>>>
>>>>> But if you want this for now lets keep scsi_sense.c in drivers/scsi
>>>>> but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.
>>>>
>>>> It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST.
>>>> BLA_SCSI_SENSE or something would do. I don't care too much about that,
>>>> mostly getting rid of the entire stack dependency.
>>>
>>> Aaand, I can't do this and leave it in drivers/scsi because of drivers/Makefile:
>>>
>>> obj-$(CONFIG_SCSI)              += scsi/
>>>
>>> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
>>> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
>>> still need to move the code from drivers/scsi/ to block/. Is this
>>> okay?
>>
>> Ugh, so that would necessitate a change there too. As I said before,
>> I don't really care where it lives. I know the SCSI folks seem bothered
>> by moving it, but in reality, it's not like this stuff will likely ever
>> really change. Of the two choices (select entire SCSI stack, or just move
>> this little bit), I know what I would consider the saner option...
>>
> 
> or option 3:
> 
> obj-y                           += scsi/
> 
> so that make descends into drivers/scsi/ and then builds whatever is needed,
> depending on individual kconfig options.

Right, that was the initial option, the later two are the other options.

-- 
Jens Axboe

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Randy Dunlap]

On 05/23/2018 02:22 PM, Jens Axboe wrote:
> On 5/23/18 3:20 PM, Randy Dunlap wrote:
>> On 05/23/2018 02:14 PM, Jens Axboe wrote:
>>> On 5/23/18 2:52 PM, Kees Cook wrote:
>>>> On Wed, May 23, 2018 at 7:31 AM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>> On 5/23/18 8:25 AM, Christoph Hellwig wrote:
>>>>>> On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote:
>>>>>>>> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
>>>>>>>> drivers/scsi/Makefile as:
>>>>>>>>
>>>>>>>> obj-$(CONFIG_BLK_SCSI_REQUEST)    += scsi_sense.o
>>>>>>>>
>>>>>>>> Every place I want to use the code is already covered by
>>>>>>>> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
>>>>>>>> put the .c file. :P
>>>>>>>
>>>>>>> I think this is so much saner than a SCSI select or dependency, so I'll
>>>>>>> have to disagree with Martin and Christoph. Just put it in drivers/scsi,
>>>>>>> if it's the location they care about.
>>>>>>
>>>>>> I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window
>>>>>> or two.  The only users are scsi and the ide layer, (virtio_blk
>>>>>> support has already been accidentally disabled for a while), and getting
>>>>>> rid of it allows to to shrink and simply the scsi data structures.
>>>>>>
>>>>>> But if you want this for now lets keep scsi_sense.c in drivers/scsi
>>>>>> but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up.
>>>>>
>>>>> It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST.
>>>>> BLA_SCSI_SENSE or something would do. I don't care too much about that,
>>>>> mostly getting rid of the entire stack dependency.
>>>>
>>>> Aaand, I can't do this and leave it in drivers/scsi because of drivers/Makefile:
>>>>
>>>> obj-$(CONFIG_SCSI)              += scsi/
>>>>
>>>> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
>>>> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
>>>> still need to move the code from drivers/scsi/ to block/. Is this
>>>> okay?
>>>
>>> Ugh, so that would necessitate a change there too. As I said before,
>>> I don't really care where it lives. I know the SCSI folks seem bothered
>>> by moving it, but in reality, it's not like this stuff will likely ever
>>> really change. Of the two choices (select entire SCSI stack, or just move
>>> this little bit), I know what I would consider the saner option...
>>>
>>
>> or option 3:
>>
>> obj-y                           += scsi/
>>
>> so that make descends into drivers/scsi/ and then builds whatever is needed,
>> depending on individual kconfig options.
> 
> Right, that was the initial option, the later two are the other options.
> 

Sorry, I'm late to the party.

-- 
~Randy

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Christoph Hellwig]

On Wed, May 23, 2018 at 02:17:14PM -0700, Kees Cook wrote:
> 
> True, though I'm finding other robustness issues in the CDROM code.
> They're probably all insane corner cases, but it seems like it'd be
> nice to just fix them:
> 
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 3522d2cae1b6..7726c8618c30 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -2222,9 +2223,12 @@ static int cdrom_read_cdda_bpc(struct
> cdrom_device_info *cdi, __u8 __user *ubuf,
> 
>                 blk_execute_rq(q, cdi->disk, rq, 0);
>                 if (scsi_req(rq)->result) {
> -                       struct request_sense *s = req->sense;
> +                       struct scsi_sense_hdr sshdr;
> +
>                         ret = -EIO;
> -                       cdi->last_sense = s->sense_key;
> +                       scsi_normalize_sense(req->sense, req->sense_len,
> +                                            &sshdr);
> +                       cdi->last_sense = sshdr.sense_key;
>                 }
> 
>                 if (blk_rq_unmap_user(bio))

The proper fix here is to rewrite this function to use the the
->generic_packet cdrom_device_ops method.  Is is the only caller
going straight to the scsi passthrough requests, which is a layering
violation.

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Christoph Hellwig]

On Wed, May 23, 2018 at 03:14:19PM -0600, Jens Axboe wrote:
> Ugh, so that would necessitate a change there too. As I said before,
> I don't really care where it lives. I know the SCSI folks seem bothered
> by moving it, but in reality, it's not like this stuff will likely ever
> really change. Of the two choices (select entire SCSI stack, or just move
> this little bit), I know what I would consider the saner option...

Oh well.  How about something like this respin of Kees' series?

http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/sense-cleanup

Re: [PATCH 6/6] scsi: Check sense buffer size at build time

[Christoph Hellwig]

> +/* Make sure any sense buffer is the correct size. */
> +#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,	\
> +		     sshdr, timeout, retries, flags, rq_flags, resid)	\
> +({									\
> +	BUILD_BUG_ON((sense) != NULL &&					\
> +		     sizeof(sense) != SCSI_SENSE_BUFFERSIZE);		\
> +	__scsi_execute(sdev, cmd, data_direction, buffer, bufflen,	\
> +		       sense, sshdr, timeout, retries, flags, rq_flags,	\
> +		       resid);						\
> +})

This macro gets evaluated in the scsi_execute_req inline function just
below.  So either we need to include scsi_sense.h/scsi_common.h in
scsi_device.h, or just move scsi_execute_req out of line.  The latter
sounds better to me a it's not really used in a fast path.

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Thu, May 24, 2018 at 1:00 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, May 23, 2018 at 03:14:19PM -0600, Jens Axboe wrote:
>> Ugh, so that would necessitate a change there too. As I said before,
>> I don't really care where it lives. I know the SCSI folks seem bothered
>> by moving it, but in reality, it's not like this stuff will likely ever
>> really change. Of the two choices (select entire SCSI stack, or just move
>> this little bit), I know what I would consider the saner option...
>
> Oh well.  How about something like this respin of Kees' series?
>
> http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/sense-cleanup

Does the CONFIG_PCMCIA in drivers/scsi/Makefile now get exposed in
weird config cases?

Otherwise, yeah, looks good to me. Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Christoph Hellwig]

On Thu, May 24, 2018 at 10:06:59AM -0700, Kees Cook wrote:
> On Thu, May 24, 2018 at 1:00 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Wed, May 23, 2018 at 03:14:19PM -0600, Jens Axboe wrote:
> >> Ugh, so that would necessitate a change there too. As I said before,
> >> I don't really care where it lives. I know the SCSI folks seem bothered
> >> by moving it, but in reality, it's not like this stuff will likely ever
> >> really change. Of the two choices (select entire SCSI stack, or just move
> >> this little bit), I know what I would consider the saner option...
> >
> > Oh well.  How about something like this respin of Kees' series?
> >
> > http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/sense-cleanup
> 
> Does the CONFIG_PCMCIA in drivers/scsi/Makefile now get exposed in
> weird config cases?

That just includes drivers/scsi/pcmcia/Makefile, which only
has three conditional modules in it, so it should be fine.

> Otherwise, yeah, looks good to me. Thanks!

Can you pick up my tweaks and ressend?

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Christoph Hellwig]

On Thu, May 24, 2018 at 10:06:59AM -0700, Kees Cook wrote:
> On Thu, May 24, 2018 at 1:00 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Wed, May 23, 2018 at 03:14:19PM -0600, Jens Axboe wrote:
> >> Ugh, so that would necessitate a change there too. As I said before,
> >> I don't really care where it lives. I know the SCSI folks seem bothered
> >> by moving it, but in reality, it's not like this stuff will likely ever
> >> really change. Of the two choices (select entire SCSI stack, or just move
> >> this little bit), I know what I would consider the saner option...
> >
> > Oh well.  How about something like this respin of Kees' series?
> >
> > http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/sense-cleanup
> 
> Does the CONFIG_PCMCIA in drivers/scsi/Makefile now get exposed in
> weird config cases?
> 
> Otherwise, yeah, looks good to me. Thanks!

Btw, did you plan to resend the series with this and your changes
applied?

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

Hi Christoph,

On Sun, Jul 8, 2018 at 1:23 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, May 24, 2018 at 10:06:59AM -0700, Kees Cook wrote:
>> On Thu, May 24, 2018 at 1:00 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> > On Wed, May 23, 2018 at 03:14:19PM -0600, Jens Axboe wrote:
>> >> Ugh, so that would necessitate a change there too. As I said before,
>> >> I don't really care where it lives. I know the SCSI folks seem bothered
>> >> by moving it, but in reality, it's not like this stuff will likely ever
>> >> really change. Of the two choices (select entire SCSI stack, or just move
>> >> this little bit), I know what I would consider the saner option...
>> >
>> > Oh well.  How about something like this respin of Kees' series?
>> >
>> > http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/sense-cleanup
>>
>> Does the CONFIG_PCMCIA in drivers/scsi/Makefile now get exposed in
>> weird config cases?
>>
>> Otherwise, yeah, looks good to me. Thanks!
>
> Btw, did you plan to resend the series with this and your changes
> applied?

Yeah, I've gotten distracted. I'll try to get to it in the next few days.

Thanks for the reminder!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Christoph Hellwig]

On Mon, Jul 09, 2018 at 08:56:50AM -0700, Kees Cook wrote:
> > Btw, did you plan to resend the series with this and your changes
> > applied?
> 
> Yeah, I've gotten distracted. I'll try to get to it in the next few days.
> 
> Thanks for the reminder!

Another little reminder.  I'd hate to miss it for 4.19.

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Tue, Jul 31, 2018 at 12:53 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Jul 09, 2018 at 08:56:50AM -0700, Kees Cook wrote:
>> > Btw, did you plan to resend the series with this and your changes
>> > applied?
>>
>> Yeah, I've gotten distracted. I'll try to get to it in the next few days.
>>
>> Thanks for the reminder!
>
> Another little reminder.  I'd hate to miss it for 4.19.

Okay, finally sent. Thanks for the reminders. :)

-Kees

-- 
Kees Cook
Pixel Security