Re: [PATCH] [PATCH] libata: take scmd->cmd_len into account when translating SCSI commands

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Tejun Heo wrote:
libata depended on SCSI command to have the correct length when
tranlating it into an ATA command.  This generally worked for commands
issued by SCSI HLD but user could issue arbitrary broken command using
sg interface.

Also, when building ATAPI command, full command size was always
copied.  Because some ATAPI devices needs bytes after CDB cleared, if
upper layer doesn't clear bytes after CDB, such devices will
malfunction.  This necessiated recent clear-garbage-after-CDB fix in
sg interfaces.  However, scsi_execute() isn't fixed yet and HL-DT-ST
DVD-RAM GSA-H30N malfunctions on initialization commands issued from
SCSI.

This patch updates libata xlat functions.

* SCSI cmd_len is always considered.  Each translation function checks
  for proper cmd_len and ATAPI translaation clears bytes after CDB.

* Don't pass CDB as argument to xlat functions.  xlat functions need
  to access more than just CDB and they have already been accessing scmd
  via qc->scsicmd.  Just pass in qc.

* Variable names are normalized.  scsi_cmnd is scmd and CDB, cdb.

Signed-off-by: Tejun Heo <htejun@xxxxxxxxx>
Cc: Jens Axboe <jens.axboe@xxxxxxxxxx>
Cc: Douglas Gilbert <dougg@xxxxxxxxxx>
---
 drivers/ata/libata-scsi.c |  187 ++++++++++++++++++++++++---------------------
 1 files changed, 100 insertions(+), 87 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a4790be..f82871c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -51,7 +51,7 @@
#define SECTOR_SIZE 512 -typedef unsigned int (*ata_xlat_func_t)(struct ata_queued_cmd *qc, const u8 *scsicmd);
+typedef unsigned int (*ata_xlat_func_t)(struct ata_queued_cmd *qc);
static struct ata_device * __ata_scsi_find_dev(struct ata_port *ap,
 					const struct scsi_device *scsidev);
@@ -935,7 +935,6 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth)
 /**
  *	ata_scsi_start_stop_xlat - Translate SCSI START STOP UNIT command
  *	@qc: Storage for translated ATA taskfile
- *	@scsicmd: SCSI command to translate
  *
  *	Sets up an ATA taskfile to issue STANDBY (to stop) or READ VERIFY
  *	(to start). Perhaps these commands should be preceded by
@@ -948,22 +947,25 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth)
  *	RETURNS:
  *	Zero on success, non-zero on error.
  */
-
-static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc,
-					     const u8 *scsicmd)
+static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 {
+	struct scsi_cmnd *scmd = qc->scsicmd;
+	const u8 *cdb = scmd->cmnd;
 	struct ata_taskfile *tf = &qc->tf;
+ if (scmd->cmd_len < 5)
+		goto invalid_fld;
+
 	tf->flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
 	tf->protocol = ATA_PROT_NODATA;
-	if (scsicmd[1] & 0x1) {
+	if (cdb[1] & 0x1) {
 		;	/* ignore IMMED bit, violates sat-r05 */
 	}
-	if (scsicmd[4] & 0x2)
-		goto invalid_fld;       /* LOEJ bit set not supported */
-	if (((scsicmd[4] >> 4) & 0xf) != 0)
-		goto invalid_fld;       /* power conditions not supported */
-	if (scsicmd[4] & 0x1) {
+	if (cdb[4] & 0x2)
+		goto invalid_fld;	/* LOEJ bit set not supported */
+	if (((cdb[4] >> 4) & 0xf) != 0)
+		goto invalid_fld;	/* power conditions not supported */
+	if (cdb[4] & 0x1) {

ACK change for #upstream-fixes, but please revise so that the "s/scsicmd/cdb/" rename is moved to a separate patch. You should be able to name the local variables such that this patch becomes much smaller.

	Jeff



-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux