[PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[zhengbin]

v1->v2: modify the comments suggested by Bart
v2->v3: fix bug in sr_do_ioctl
v3->v4: let "fix bug in sr_do_ioctl" be a separate patch
v4->v5: fix uninit-value access bug in callers, not in __scsi_execute

zhengbin (13):
  scsi: core: need to check the result of scsi_execute in
    scsi_report_opcode
  scsi: core: need to check the result of scsi_execute in
    scsi_test_unit_ready
  scsi: core: need to check the result of scsi_execute in
    scsi_report_lun_scan
  scsi: sr: need to check the result of scsi_execute in sr_get_events
  scsi: sr: need to check the result of scsi_execute in sr_do_ioctl
  scsi: scsi_dh_emc: need to check the result of scsi_execute in
    send_trespass_cmd
  scsi: scsi_dh_rdac: need to check the result of scsi_execute in
    send_mode_select
  scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in
    hp_sw_tur,hp_sw_start_stop
  scsi: scsi_dh_alua: need to check the result of scsi_execute in
    alua_rtpg,alua_stpg
  scsi: scsi_transport_spi: need to check whether sshdr is valid in
    spi_execute
  scsi: cxlflash: need to check whether sshdr is valid in read_cap16
  scsi: ufs: need to check whether sshdr is valid in
    ufshcd_set_dev_pwr_mode
  scsi: ch: need to check whether sshdr is valid in ch_do_scsi

 drivers/scsi/ch.c                           | 6 ++++--
 drivers/scsi/cxlflash/superpipe.c           | 2 +-
 drivers/scsi/device_handler/scsi_dh_alua.c  | 6 ++++--
 drivers/scsi/device_handler/scsi_dh_emc.c   | 3 ++-
 drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++--
 drivers/scsi/device_handler/scsi_dh_rdac.c  | 8 +++++---
 drivers/scsi/scsi.c                         | 2 +-
 drivers/scsi/scsi_lib.c                     | 3 +++
 drivers/scsi/scsi_scan.c                    | 3 ++-
 drivers/scsi/scsi_transport_spi.c           | 1 +
 drivers/scsi/sr.c                           | 3 ++-
 drivers/scsi/sr_ioctl.c                     | 6 ++++++
 drivers/scsi/ufs/ufshcd.c                   | 3 ++-
 13 files changed, 37 insertions(+), 15 deletions(-)

--
2.7.4

[PATCH v5 01/13] scsi: core: need to check the result of scsi_execute in scsi_report_opcode

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 4f76841..15d1fe9 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -509,7 +509,7 @@ int scsi_report_opcode(struct scsi_device *sdev, unsigned char *buffer,
 	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer, len,
 				  &sshdr, 30 * HZ, 3, NULL);

-	if (result && scsi_sense_valid(&sshdr) &&
+	if (driver_byte(result) == DRIVER_SENSE && scsi_sense_valid(&sshdr) &&
 	    sshdr.sense_key == ILLEGAL_REQUEST &&
 	    (sshdr.asc == 0x20 || sshdr.asc == 0x24) && sshdr.ascq == 0x00)
 		return -EINVAL;
--
2.7.4

[PATCH v5 02/13] scsi: core: need to check the result of scsi_execute in scsi_test_unit_ready

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/scsi_lib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5447738..2970190 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2216,6 +2216,9 @@ scsi_test_unit_ready(struct scsi_device *sdev, int timeout, int retries,
 	do {
 		result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL, 0, sshdr,
 					  timeout, 1, NULL);
+		if (driver_byte(result) != DRIVER_SENSE)
+			break;
+
 		if (sdev->removable && scsi_sense_valid(sshdr) &&
 		    sshdr->sense_key == UNIT_ATTENTION)
 			sdev->changed = 1;
--
2.7.4

[PATCH v5 03/13] scsi: core: need to check the result of scsi_execute in scsi_report_lun_scan

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/scsi_scan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 058079f..781f82a 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1385,7 +1385,8 @@ static int scsi_report_lun_scan(struct scsi_target *starget, blist_flags_t bflag
 				retries, result));
 		if (result == 0)
 			break;
-		else if (scsi_sense_valid(&sshdr)) {
+		else if (driver_byte(result) == DRIVER_SENSE &&
+			 scsi_sense_valid(&sshdr)) {
 			if (sshdr.sense_key != UNIT_ATTENTION)
 				break;
 		}
--
2.7.4

[PATCH v5 04/13] scsi: sr: need to check the result of scsi_execute in sr_get_events

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/sr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 4664fdf..5c5476b 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -205,7 +205,8 @@ static unsigned int sr_get_events(struct scsi_device *sdev)

 	result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, sizeof(buf),
 				  &sshdr, SR_TIMEOUT, MAX_RETRIES, NULL);
-	if (scsi_sense_valid(&sshdr) && sshdr.sense_key == UNIT_ATTENTION)
+	if (driver_byte(result) == DRIVER_SENSE && scsi_sense_valid(&sshdr) &&
+	    sshdr.sense_key == UNIT_ATTENTION)
 		return DISK_EVENT_MEDIA_CHANGE;

 	if (result || be16_to_cpu(eh->data_len) < sizeof(*med))
--
2.7.4

[PATCH v5 05/13] scsi: sr: need to check the result of scsi_execute in sr_do_ioctl

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute, also need to check whether sshdr is valid.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/sr_ioctl.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index ffcf902..7672fc1 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -206,6 +206,12 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)

 	/* Minimal error checking.  Ignore cases we know about, and report the rest. */
 	if (driver_byte(result) != 0) {
+		if (driver_byte(result) != DRIVER_SENSE ||
+		    !scsi_sense_valid(sshdr)) {
+			err = -EIO;
+			goto out;
+		}
+
 		switch (sshdr->sense_key) {
 		case UNIT_ATTENTION:
 			SDev->changed = 1;
--
2.7.4

[PATCH v5 06/13] scsi: scsi_dh_emc: need to check the result of scsi_execute in send_trespass_cmd

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/device_handler/scsi_dh_emc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index caa685c..cf49378 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -267,7 +267,8 @@ static int send_trespass_cmd(struct scsi_device *sdev,
 			&sshdr, CLARIION_TIMEOUT * HZ, CLARIION_RETRIES,
 			req_flags, 0, NULL);
 	if (err) {
-		if (scsi_sense_valid(&sshdr))
+		if (driver_byte(err) == DRIVER_SENSE &&
+		    scsi_sense_valid(&sshdr))
 			res = trespass_endio(sdev, &sshdr);
 		else {
 			sdev_printk(KERN_INFO, sdev,
--
2.7.4

[PATCH v5 07/13] scsi: scsi_dh_rdac: need to check the result of scsi_execute in send_mode_select

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/device_handler/scsi_dh_rdac.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index 5efc959..2a43985 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -531,6 +531,7 @@ static void send_mode_select(struct work_struct *work)
 	struct scsi_device *sdev = ctlr->ms_sdev;
 	struct rdac_dh_data *h = sdev->handler_data;
 	int err = SCSI_DH_OK, retry_cnt = RDAC_RETRY_COUNT;
+	int result;
 	struct rdac_queue_data *tmp, *qdata;
 	LIST_HEAD(list);
 	unsigned char cdb[MAX_COMMAND_SIZE];
@@ -555,9 +556,10 @@ static void send_mode_select(struct work_struct *work)
 		(char *) h->ctlr->array_name, h->ctlr->index,
 		(retry_cnt == RDAC_RETRY_COUNT) ? "queueing" : "retrying");

-	if (scsi_execute(sdev, cdb, DMA_TO_DEVICE, &h->ctlr->mode_select,
-			data_size, NULL, &sshdr, RDAC_TIMEOUT * HZ,
-			RDAC_RETRIES, req_flags, 0, NULL)) {
+	result = scsi_execute(sdev, cdb, DMA_TO_DEVICE, &h->ctlr->mode_select,
+			     data_size, NULL, &sshdr, RDAC_TIMEOUT * HZ,
+			     RDAC_RETRIES, req_flags, 0, NULL);
+	if (driver_byte(result) == DRIVER_SENSE) {
 		err = mode_select_handle_sense(sdev, &sshdr);
 		if (err == SCSI_DH_RETRY && retry_cnt--)
 			goto retry;
--
2.7.4

[PATCH v5 08/13] scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in hp_sw_tur,hp_sw_start_stop

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index 8acd4bb..be8c139 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -90,7 +90,8 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
 	res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
 			HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
 	if (res) {
-		if (scsi_sense_valid(&sshdr))
+		if (driver_byte(res) == DRIVER_SENSE &&
+		    scsi_sense_valid(&sshdr))
 			ret = tur_done(sdev, h, &sshdr);
 		else {
 			sdev_printk(KERN_WARNING, sdev,
@@ -128,7 +129,8 @@ static int hp_sw_start_stop(struct hp_sw_dh_data *h)
 	res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
 			HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
 	if (res) {
-		if (!scsi_sense_valid(&sshdr)) {
+		if (driver_byte(result) != DRIVER_SENSE ||
+		    !scsi_sense_valid(&sshdr)) {
 			sdev_printk(KERN_WARNING, sdev,
 				    "%s: sending start_stop_unit failed, "
 				    "no sense available\n", HP_SW_NAME);
--
2.7.4

[PATCH v5 09/13] scsi: scsi_dh_alua: need to check the result of scsi_execute in alua_rtpg,alua_stpg

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.

submit_rtpg/submit_stpg
  scsi_execute

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index f32da0c..97c38ed 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -548,7 +548,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 			kfree(buff);
 			return SCSI_DH_OK;
 		}
-		if (!scsi_sense_valid(&sense_hdr)) {
+		if (driver_byte(retval) != DRIVER_SENSE ||
+		    !scsi_sense_valid(&sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev,
 				    "%s: rtpg failed, result %d\n",
 				    ALUA_DH_NAME, retval);
@@ -773,7 +774,8 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	retval = submit_stpg(sdev, pg->group_id, &sense_hdr);

 	if (retval) {
-		if (!scsi_sense_valid(&sense_hdr)) {
+		if (driver_byte(retval) != DRIVER_SENSE ||
+		    !scsi_sense_valid(&sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev,
 				    "%s: stpg failed, result %d",
 				    ALUA_DH_NAME, retval);
--
2.7.4

[PATCH v5 10/13] scsi: scsi_transport_spi: need to check whether sshdr is valid in spi_execute

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check whether
sshdr is valid.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/scsi_transport_spi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
index f866106..428f9b8 100644
--- a/drivers/scsi/scsi_transport_spi.c
+++ b/drivers/scsi/scsi_transport_spi.c
@@ -124,6 +124,7 @@ static int spi_execute(struct scsi_device *sdev, const void *cmd,
 				      REQ_FAILFAST_DRIVER,
 				      0, NULL);
 		if (driver_byte(result) != DRIVER_SENSE ||
+		    !scsi_sense_valid(sshdr) ||
 		    sshdr->sense_key != UNIT_ATTENTION)
 			break;
 	}
--
2.7.4

[PATCH v5 11/13] scsi: cxlflash: need to check whether sshdr is valid in read_cap16

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check whether
sshdr is valid.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/cxlflash/superpipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index 593669a..1e90ff3 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -369,7 +369,7 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
 		goto out;
 	}

-	if (driver_byte(result) == DRIVER_SENSE) {
+	if (driver_byte(result) == DRIVER_SENSE && scsi_sense_valid(&sshdr)) {
 		result &= ~(0xFF<<24); /* DRIVER_SENSE is not an error */
 		if (result & SAM_STAT_CHECK_CONDITION) {
 			switch (sshdr.sense_key) {
--
2.7.4

[PATCH v5 12/13] scsi: ufs: need to check whether sshdr is valid in ufshcd_set_dev_pwr_mode

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check whether
sshdr is valid.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/ufs/ufshcd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c28c144..5afc426 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7600,7 +7600,8 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
 		sdev_printk(KERN_WARNING, sdp,
 			    "START_STOP failed for power mode: %d, result %x\n",
 			    pwr_mode, ret);
-		if (driver_byte(ret) == DRIVER_SENSE)
+		if (driver_byte(ret) == DRIVER_SENSE &&
+		    scsi_sense_valid(&sshdr))
 			scsi_print_sense_hdr(sdp, NULL, &sshdr);
 	}

--
2.7.4

[PATCH v5 13/13] scsi: ch: need to check whether sshdr is valid in ch_do_scsi

[zhengbin]

Like sd_pr_command, before use sshdr, we need to check whether
sshdr is valid.

Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
 drivers/scsi/ch.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 76751d6..dba6fe2 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -200,7 +200,7 @@ ch_do_scsi(scsi_changer *ch, unsigned char *cmd, int cmd_len,
 				  buflength, &sshdr, timeout * HZ,
 				  MAX_RETRIES, NULL);

-	if (driver_byte(result) == DRIVER_SENSE) {
+	if (driver_byte(result) == DRIVER_SENSE && scsi_sense_valid(&sshdr)) {
 		if (debug)
 			scsi_print_sense_hdr(ch->device, ch->name, &sshdr);
 		errno = ch_find_errno(&sshdr);
@@ -212,7 +212,9 @@ ch_do_scsi(scsi_changer *ch, unsigned char *cmd, int cmd_len,
 				goto retry;
 			break;
 		}
-	}
+	} else if (result)
+		errno = -EIO;
+
 	return errno;
 }

--
2.7.4

Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[Hannes Reinecke]

On 10/18/19 10:24 AM, zhengbin wrote:
> v1->v2: modify the comments suggested by Bart
> v2->v3: fix bug in sr_do_ioctl
> v3->v4: let "fix bug in sr_do_ioctl" be a separate patch
> v4->v5: fix uninit-value access bug in callers, not in __scsi_execute
> 
> zhengbin (13):
>   scsi: core: need to check the result of scsi_execute in
>     scsi_report_opcode
>   scsi: core: need to check the result of scsi_execute in
>     scsi_test_unit_ready
>   scsi: core: need to check the result of scsi_execute in
>     scsi_report_lun_scan
>   scsi: sr: need to check the result of scsi_execute in sr_get_events
>   scsi: sr: need to check the result of scsi_execute in sr_do_ioctl
>   scsi: scsi_dh_emc: need to check the result of scsi_execute in
>     send_trespass_cmd
>   scsi: scsi_dh_rdac: need to check the result of scsi_execute in
>     send_mode_select
>   scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in
>     hp_sw_tur,hp_sw_start_stop
>   scsi: scsi_dh_alua: need to check the result of scsi_execute in
>     alua_rtpg,alua_stpg
>   scsi: scsi_transport_spi: need to check whether sshdr is valid in
>     spi_execute
>   scsi: cxlflash: need to check whether sshdr is valid in read_cap16
>   scsi: ufs: need to check whether sshdr is valid in
>     ufshcd_set_dev_pwr_mode
>   scsi: ch: need to check whether sshdr is valid in ch_do_scsi
> 
>  drivers/scsi/ch.c                           | 6 ++++--
>  drivers/scsi/cxlflash/superpipe.c           | 2 +-
>  drivers/scsi/device_handler/scsi_dh_alua.c  | 6 ++++--
>  drivers/scsi/device_handler/scsi_dh_emc.c   | 3 ++-
>  drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++--
>  drivers/scsi/device_handler/scsi_dh_rdac.c  | 8 +++++---
>  drivers/scsi/scsi.c                         | 2 +-
>  drivers/scsi/scsi_lib.c                     | 3 +++
>  drivers/scsi/scsi_scan.c                    | 3 ++-
>  drivers/scsi/scsi_transport_spi.c           | 1 +
>  drivers/scsi/sr.c                           | 3 ++-
>  drivers/scsi/sr_ioctl.c                     | 6 ++++++
>  drivers/scsi/ufs/ufshcd.c                   | 3 ++-
>  13 files changed, 37 insertions(+), 15 deletions(-)
> 
> --
> 2.7.4
> 
Nope.

The one thing which I patently don't like is the ambivalence between
DRIVER_SENSE and scsi_sense_valid().
What shall we do if only _one_ of them is set?
IE what would be the correct way of action if DRIVER_SENSE is not set,
but we have a valid sense code?
Or the other way around?

But more important, from a quick glance not all drivers set the
DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
never evaluated after this patchset.

I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on
scsi_sense_valid() only.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[Damien Le Moal]

On 2019/10/18 17:17, zhengbin wrote:
> v1->v2: modify the comments suggested by Bart
> v2->v3: fix bug in sr_do_ioctl
> v3->v4: let "fix bug in sr_do_ioctl" be a separate patch
> v4->v5: fix uninit-value access bug in callers, not in __scsi_execute

An explanation text of the series would be great...

What about defining a little helper function for checking

driver_byte(err) == DRIVER_SENSE && scsi_sense_valid(&sshdr)

instead of having that hard-coded everywhere ? That would make the code
a lot cleaner and more readable.

Something like:

static inline bool scsi_driver_sense_valid(int result,
					   struct scsi_sense_hdr *sshdr)
{
	return driver_byte(result) == DRIVER_SENSE &&
	       scsi_sense_valid(sshdr);
}


> 
> zhengbin (13):
>   scsi: core: need to check the result of scsi_execute in
>     scsi_report_opcode
>   scsi: core: need to check the result of scsi_execute in
>     scsi_test_unit_ready
>   scsi: core: need to check the result of scsi_execute in
>     scsi_report_lun_scan
>   scsi: sr: need to check the result of scsi_execute in sr_get_events
>   scsi: sr: need to check the result of scsi_execute in sr_do_ioctl
>   scsi: scsi_dh_emc: need to check the result of scsi_execute in
>     send_trespass_cmd
>   scsi: scsi_dh_rdac: need to check the result of scsi_execute in
>     send_mode_select
>   scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in
>     hp_sw_tur,hp_sw_start_stop
>   scsi: scsi_dh_alua: need to check the result of scsi_execute in
>     alua_rtpg,alua_stpg
>   scsi: scsi_transport_spi: need to check whether sshdr is valid in
>     spi_execute
>   scsi: cxlflash: need to check whether sshdr is valid in read_cap16
>   scsi: ufs: need to check whether sshdr is valid in
>     ufshcd_set_dev_pwr_mode
>   scsi: ch: need to check whether sshdr is valid in ch_do_scsi
> 
>  drivers/scsi/ch.c                           | 6 ++++--
>  drivers/scsi/cxlflash/superpipe.c           | 2 +-
>  drivers/scsi/device_handler/scsi_dh_alua.c  | 6 ++++--
>  drivers/scsi/device_handler/scsi_dh_emc.c   | 3 ++-
>  drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++--
>  drivers/scsi/device_handler/scsi_dh_rdac.c  | 8 +++++---
>  drivers/scsi/scsi.c                         | 2 +-
>  drivers/scsi/scsi_lib.c                     | 3 +++
>  drivers/scsi/scsi_scan.c                    | 3 ++-
>  drivers/scsi/scsi_transport_spi.c           | 1 +
>  drivers/scsi/sr.c                           | 3 ++-
>  drivers/scsi/sr_ioctl.c                     | 6 ++++++
>  drivers/scsi/ufs/ufshcd.c                   | 3 ++-
>  13 files changed, 37 insertions(+), 15 deletions(-)
> 
> --
> 2.7.4
> 
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[Martin K. Petersen]

Hannes,

> The one thing which I patently don't like is the ambivalence between
> DRIVER_SENSE and scsi_sense_valid().  What shall we do if only _one_
> of them is set?  IE what would be the correct way of action if
> DRIVER_SENSE is not set, but we have a valid sense code?  Or the other
> way around?

I agree, it's a mess.

(Sorry, zhengbin, you opened a can of worms. This is some of our oldest
and most arcane code in SCSI)

> But more important, from a quick glance not all drivers set the
> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
> never evaluated after this patchset.

And yet we appear to have several code paths where sense evaluation is
contingent on DRIVER_SENSE. So no matter what, behavior might
change if we enforce consistent semantics. *sigh*

> I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on
> scsi_sense_valid() only.

I would really like to get rid of DRIVER_* completely. Except for
DRIVER_SENSE, few are actually in use:

DRIVER_OK: 	0
DRIVER_BUSY:	0
DRIVER_SOFT:	0
DRIVER_MEDIA:	0
DRIVER_ERROR:	6
DRIVER_INVALID:	4
DRIVER_TIMEOUT:	1
DRIVER_SENSE:	58

Johannes: Whatever happened to your efforts at cleaning all this up? Do
you have a patch series or a working tree we could use as starting
point?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[Hannes Reinecke]

On 10/18/19 3:43 PM, Martin K. Petersen wrote:
> 
> Hannes,
> 
>> The one thing which I patently don't like is the ambivalence between
>> DRIVER_SENSE and scsi_sense_valid().  What shall we do if only _one_
>> of them is set?  IE what would be the correct way of action if
>> DRIVER_SENSE is not set, but we have a valid sense code?  Or the other
>> way around?
> 
> I agree, it's a mess.
> 
> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
> and most arcane code in SCSI)
> 
>> But more important, from a quick glance not all drivers set the
>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
>> never evaluated after this patchset.
> 
> And yet we appear to have several code paths where sense evaluation is
> contingent on DRIVER_SENSE. So no matter what, behavior might
> change if we enforce consistent semantics. *sigh*
> 
>> I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on
>> scsi_sense_valid() only.
> 
> I would really like to get rid of DRIVER_* completely. Except for
> DRIVER_SENSE, few are actually in use:
> 
> DRIVER_OK: 	0
> DRIVER_BUSY:	0
> DRIVER_SOFT:	0
> DRIVER_MEDIA:	0
> DRIVER_ERROR:	6

Three less now :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

[RFC] scsi: Avoid sign extension when setting command result bytes, was Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[Finn Thain]

On Fri, 18 Oct 2019, Martin K. Petersen wrote:

>
> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
> and most arcane code in SCSI)
>

A call to set_host_byte(cmd, x) or set_msg_byte(cmd, x) when x & 0x80 is 
set clobbers the most significant bytes in cmd->result.

Avoid this implicit sign extension when shifting bits by using an
unsigned formal parameter.

This is a theoretical bug, found by inspection, so I'm sending an untested 
RFC patch.

I didn't try to find possible callers that might pass a negative parameter
and neither did I check whether the clobber might be intentional...

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 76ed5e4acd38..ae814fa68bb8 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -306,17 +306,17 @@ static inline struct scsi_data_buffer *scsi_prot(struct scsi_cmnd *cmd)
 #define scsi_for_each_prot_sg(cmd, sg, nseg, __i)		\
 	for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
 
-static inline void set_msg_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_msg_byte(struct scsi_cmnd *cmd, unsigned char status)
 {
 	cmd->result = (cmd->result & 0xffff00ff) | (status << 8);
 }
 
-static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_host_byte(struct scsi_cmnd *cmd, unsigned char status)
 {
 	cmd->result = (cmd->result & 0xff00ffff) | (status << 16);
 }
 
-static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_driver_byte(struct scsi_cmnd *cmd, unsigned char status)
 {
 	cmd->result = (cmd->result & 0x00ffffff) | (status << 24);
 }

Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[zhengbin (A)]

On 2019/10/18 21:43, Martin K. Petersen wrote:
> Hannes,
>
>> The one thing which I patently don't like is the ambivalence between
>> DRIVER_SENSE and scsi_sense_valid().  What shall we do if only _one_
>> of them is set?  IE what would be the correct way of action if
>> DRIVER_SENSE is not set, but we have a valid sense code?  Or the other
>> way around?
> I agree, it's a mess.
>
> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
> and most arcane code in SCSI)
>
>> But more important, from a quick glance not all drivers set the
>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
>> never evaluated after this patchset.
> And yet we appear to have several code paths where sense evaluation is
> contingent on DRIVER_SENSE. So no matter what, behavior might
> change if we enforce consistent semantics. *sigh*

So what should we do to prevent unit-value access of sshdr?

1. still init sshdr in __scsi_execute?

@@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 	struct scsi_request *rq;
 	int ret = DRIVER_ERROR << 24;

+	/*
+	 * Zero-initialize sshdr for those callers that check the *sshdr
+	 * contents even if no sense data is available.
+	 */
+	if (sshdr)
+		memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
+
 	req = blk_get_request(sdev->request_queue,
 			data_direction == DMA_TO_DEVICE ?
 			REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);

2. init sshdr in callers of scsi_execute, instead of check the result of scsi_execute and check
whether sshdr is valid? for example:
@@ -506,6 +506,7 @@ int scsi_report_opcode(struct scsi_device *sdev, unsigned char *buffer,
        put_unaligned_be32(len, &cmd[6]);
        memset(buffer, 0, len);

+       memset(&sshdr, 0 ,sizeof(sshdr));
        result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer, len,
                                  &sshdr, 30 * HZ, 3, NULL)

Besides, should sd_pr_command init sshdr?? cause sd_pr_command  have checked the result of scsi_execute 
and check whether sshdr is valid.


3. get rid of DRIVER_* completely? even this, we should init sshdr first. 


sshdr is just 8 bytes, init it does not affect performance

My advice is 2.

>
>> I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on
>> scsi_sense_valid() only.
> I would really like to get rid of DRIVER_* completely. Except for
> DRIVER_SENSE, few are actually in use:
>
> DRIVER_OK: 	0
> DRIVER_BUSY:	0
> DRIVER_SOFT:	0
> DRIVER_MEDIA:	0
> DRIVER_ERROR:	6
> DRIVER_INVALID:	4
> DRIVER_TIMEOUT:	1
> DRIVER_SENSE:	58
>
> Johannes: Whatever happened to your efforts at cleaning all this up? Do
> you have a patch series or a working tree we could use as starting
> point?
>

Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[Johannes Thumshirn]

On 18/10/2019 15:43, Martin K. Petersen wrote:
[...]
> Johannes: Whatever happened to your efforts at cleaning all this up? Do
> you have a patch series or a working tree we could use as starting
> point?

I'll have to look. I think it's still floating around in some git tree.

Let me search for it.

	Johannes

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v5 08/13] scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in hp_sw_tur,hp_sw_start_stop

[kbuild test robot]

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

Hi zhengbin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mkp-scsi/for-next]
[cannot apply to v5.4-rc4 next-20191018]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/zhengbin/scsi-core-fix-uninit-value-access-of-variable-sshdr/20191021-160007
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/scsi/device_handler/scsi_dh_hp_sw.c:13:0:
   drivers/scsi/device_handler/scsi_dh_hp_sw.c: In function 'hp_sw_start_stop':
>> drivers/scsi/device_handler/scsi_dh_hp_sw.c:132:19: error: 'result' undeclared (first use in this function); did you mean 'res'?
      if (driver_byte(result) != DRIVER_SENSE ||
                      ^
   include/scsi/scsi.h:213:32: note: in definition of macro 'driver_byte'
    #define driver_byte(result) (((result) >> 24) & 0xff)
                                   ^~~~~~
   drivers/scsi/device_handler/scsi_dh_hp_sw.c:132:19: note: each undeclared identifier is reported only once for each function it appears in
      if (driver_byte(result) != DRIVER_SENSE ||
                      ^
   include/scsi/scsi.h:213:32: note: in definition of macro 'driver_byte'
    #define driver_byte(result) (((result) >> 24) & 0xff)
                                   ^~~~~~

vim +132 drivers/scsi/device_handler/scsi_dh_hp_sw.c

  > 13	#include <scsi/scsi.h>
    14	#include <scsi/scsi_dbg.h>
    15	#include <scsi/scsi_eh.h>
    16	#include <scsi/scsi_dh.h>
    17	
    18	#define HP_SW_NAME			"hp_sw"
    19	
    20	#define HP_SW_TIMEOUT			(60 * HZ)
    21	#define HP_SW_RETRIES			3
    22	
    23	#define HP_SW_PATH_UNINITIALIZED	-1
    24	#define HP_SW_PATH_ACTIVE		0
    25	#define HP_SW_PATH_PASSIVE		1
    26	
    27	struct hp_sw_dh_data {
    28		int path_state;
    29		int retries;
    30		int retry_cnt;
    31		struct scsi_device *sdev;
    32	};
    33	
    34	static int hp_sw_start_stop(struct hp_sw_dh_data *);
    35	
    36	/*
    37	 * tur_done - Handle TEST UNIT READY return status
    38	 * @sdev: sdev the command has been sent to
    39	 * @errors: blk error code
    40	 *
    41	 * Returns SCSI_DH_DEV_OFFLINED if the sdev is on the passive path
    42	 */
    43	static int tur_done(struct scsi_device *sdev, struct hp_sw_dh_data *h,
    44			    struct scsi_sense_hdr *sshdr)
    45	{
    46		int ret = SCSI_DH_IO;
    47	
    48		switch (sshdr->sense_key) {
    49		case UNIT_ATTENTION:
    50			ret = SCSI_DH_IMM_RETRY;
    51			break;
    52		case NOT_READY:
    53			if (sshdr->asc == 0x04 && sshdr->ascq == 2) {
    54				/*
    55				 * LUN not ready - Initialization command required
    56				 *
    57				 * This is the passive path
    58				 */
    59				h->path_state = HP_SW_PATH_PASSIVE;
    60				ret = SCSI_DH_OK;
    61				break;
    62			}
    63			/* Fallthrough */
    64		default:
    65			sdev_printk(KERN_WARNING, sdev,
    66				   "%s: sending tur failed, sense %x/%x/%x\n",
    67				   HP_SW_NAME, sshdr->sense_key, sshdr->asc,
    68				   sshdr->ascq);
    69			break;
    70		}
    71		return ret;
    72	}
    73	
    74	/*
    75	 * hp_sw_tur - Send TEST UNIT READY
    76	 * @sdev: sdev command should be sent to
    77	 *
    78	 * Use the TEST UNIT READY command to determine
    79	 * the path state.
    80	 */
    81	static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
    82	{
    83		unsigned char cmd[6] = { TEST_UNIT_READY };
    84		struct scsi_sense_hdr sshdr;
    85		int ret = SCSI_DH_OK, res;
    86		u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
    87			REQ_FAILFAST_DRIVER;
    88	
    89	retry:
    90		res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
    91				HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
    92		if (res) {
    93			if (driver_byte(res) == DRIVER_SENSE &&
    94			    scsi_sense_valid(&sshdr))
    95				ret = tur_done(sdev, h, &sshdr);
    96			else {
    97				sdev_printk(KERN_WARNING, sdev,
    98					    "%s: sending tur failed with %x\n",
    99					    HP_SW_NAME, res);
   100				ret = SCSI_DH_IO;
   101			}
   102		} else {
   103			h->path_state = HP_SW_PATH_ACTIVE;
   104			ret = SCSI_DH_OK;
   105		}
   106		if (ret == SCSI_DH_IMM_RETRY)
   107			goto retry;
   108	
   109		return ret;
   110	}
   111	
   112	/*
   113	 * hp_sw_start_stop - Send START STOP UNIT command
   114	 * @sdev: sdev command should be sent to
   115	 *
   116	 * Sending START STOP UNIT activates the SP.
   117	 */
   118	static int hp_sw_start_stop(struct hp_sw_dh_data *h)
   119	{
   120		unsigned char cmd[6] = { START_STOP, 0, 0, 0, 1, 0 };
   121		struct scsi_sense_hdr sshdr;
   122		struct scsi_device *sdev = h->sdev;
   123		int res, rc = SCSI_DH_OK;
   124		int retry_cnt = HP_SW_RETRIES;
   125		u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
   126			REQ_FAILFAST_DRIVER;
   127	
   128	retry:
   129		res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
   130				HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
   131		if (res) {
 > 132			if (driver_byte(result) != DRIVER_SENSE ||
   133			    !scsi_sense_valid(&sshdr)) {
   134				sdev_printk(KERN_WARNING, sdev,
   135					    "%s: sending start_stop_unit failed, "
   136					    "no sense available\n", HP_SW_NAME);
   137				return SCSI_DH_IO;
   138			}
   139			switch (sshdr.sense_key) {
   140			case NOT_READY:
   141				if (sshdr.asc == 0x04 && sshdr.ascq == 3) {
   142					/*
   143					 * LUN not ready - manual intervention required
   144					 *
   145					 * Switch-over in progress, retry.
   146					 */
   147					if (--retry_cnt)
   148						goto retry;
   149					rc = SCSI_DH_RETRY;
   150					break;
   151				}
   152				/* fall through */
   153			default:
   154				sdev_printk(KERN_WARNING, sdev,
   155					    "%s: sending start_stop_unit failed, "
   156					    "sense %x/%x/%x\n", HP_SW_NAME,
   157					    sshdr.sense_key, sshdr.asc, sshdr.ascq);
   158				rc = SCSI_DH_IO;
   159			}
   160		}
   161		return rc;
   162	}
   163	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52246 bytes --]

Re: [PATCH v5 08/13] scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in hp_sw_tur, hp_sw_start_stop

[kbuild test robot]

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

Hi zhengbin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mkp-scsi/for-next]
[cannot apply to v5.4-rc4 next-20191018]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/zhengbin/scsi-core-fix-uninit-value-access-of-variable-sshdr/20191021-160007
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/scsi/device_handler/scsi_dh_hp_sw.c:13:0:
   drivers/scsi/device_handler/scsi_dh_hp_sw.c: In function 'hp_sw_start_stop':
>> drivers/scsi/device_handler/scsi_dh_hp_sw.c:132:19: error: 'result' undeclared (first use in this function); did you mean 'res'?
      if (driver_byte(result) != DRIVER_SENSE ||
                      ^
   include/scsi/scsi.h:213:32: note: in definition of macro 'driver_byte'
    #define driver_byte(result) (((result) >> 24) & 0xff)
                                   ^~~~~~
   drivers/scsi/device_handler/scsi_dh_hp_sw.c:132:19: note: each undeclared identifier is reported only once for each function it appears in
      if (driver_byte(result) != DRIVER_SENSE ||
                      ^
   include/scsi/scsi.h:213:32: note: in definition of macro 'driver_byte'
    #define driver_byte(result) (((result) >> 24) & 0xff)
                                   ^~~~~~

vim +132 drivers/scsi/device_handler/scsi_dh_hp_sw.c

  > 13	#include <scsi/scsi.h>
    14	#include <scsi/scsi_dbg.h>
    15	#include <scsi/scsi_eh.h>
    16	#include <scsi/scsi_dh.h>
    17	
    18	#define HP_SW_NAME			"hp_sw"
    19	
    20	#define HP_SW_TIMEOUT			(60 * HZ)
    21	#define HP_SW_RETRIES			3
    22	
    23	#define HP_SW_PATH_UNINITIALIZED	-1
    24	#define HP_SW_PATH_ACTIVE		0
    25	#define HP_SW_PATH_PASSIVE		1
    26	
    27	struct hp_sw_dh_data {
    28		int path_state;
    29		int retries;
    30		int retry_cnt;
    31		struct scsi_device *sdev;
    32	};
    33	
    34	static int hp_sw_start_stop(struct hp_sw_dh_data *);
    35	
    36	/*
    37	 * tur_done - Handle TEST UNIT READY return status
    38	 * @sdev: sdev the command has been sent to
    39	 * @errors: blk error code
    40	 *
    41	 * Returns SCSI_DH_DEV_OFFLINED if the sdev is on the passive path
    42	 */
    43	static int tur_done(struct scsi_device *sdev, struct hp_sw_dh_data *h,
    44			    struct scsi_sense_hdr *sshdr)
    45	{
    46		int ret = SCSI_DH_IO;
    47	
    48		switch (sshdr->sense_key) {
    49		case UNIT_ATTENTION:
    50			ret = SCSI_DH_IMM_RETRY;
    51			break;
    52		case NOT_READY:
    53			if (sshdr->asc == 0x04 && sshdr->ascq == 2) {
    54				/*
    55				 * LUN not ready - Initialization command required
    56				 *
    57				 * This is the passive path
    58				 */
    59				h->path_state = HP_SW_PATH_PASSIVE;
    60				ret = SCSI_DH_OK;
    61				break;
    62			}
    63			/* Fallthrough */
    64		default:
    65			sdev_printk(KERN_WARNING, sdev,
    66				   "%s: sending tur failed, sense %x/%x/%x\n",
    67				   HP_SW_NAME, sshdr->sense_key, sshdr->asc,
    68				   sshdr->ascq);
    69			break;
    70		}
    71		return ret;
    72	}
    73	
    74	/*
    75	 * hp_sw_tur - Send TEST UNIT READY
    76	 * @sdev: sdev command should be sent to
    77	 *
    78	 * Use the TEST UNIT READY command to determine
    79	 * the path state.
    80	 */
    81	static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
    82	{
    83		unsigned char cmd[6] = { TEST_UNIT_READY };
    84		struct scsi_sense_hdr sshdr;
    85		int ret = SCSI_DH_OK, res;
    86		u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
    87			REQ_FAILFAST_DRIVER;
    88	
    89	retry:
    90		res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
    91				HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
    92		if (res) {
    93			if (driver_byte(res) == DRIVER_SENSE &&
    94			    scsi_sense_valid(&sshdr))
    95				ret = tur_done(sdev, h, &sshdr);
    96			else {
    97				sdev_printk(KERN_WARNING, sdev,
    98					    "%s: sending tur failed with %x\n",
    99					    HP_SW_NAME, res);
   100				ret = SCSI_DH_IO;
   101			}
   102		} else {
   103			h->path_state = HP_SW_PATH_ACTIVE;
   104			ret = SCSI_DH_OK;
   105		}
   106		if (ret == SCSI_DH_IMM_RETRY)
   107			goto retry;
   108	
   109		return ret;
   110	}
   111	
   112	/*
   113	 * hp_sw_start_stop - Send START STOP UNIT command
   114	 * @sdev: sdev command should be sent to
   115	 *
   116	 * Sending START STOP UNIT activates the SP.
   117	 */
   118	static int hp_sw_start_stop(struct hp_sw_dh_data *h)
   119	{
   120		unsigned char cmd[6] = { START_STOP, 0, 0, 0, 1, 0 };
   121		struct scsi_sense_hdr sshdr;
   122		struct scsi_device *sdev = h->sdev;
   123		int res, rc = SCSI_DH_OK;
   124		int retry_cnt = HP_SW_RETRIES;
   125		u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
   126			REQ_FAILFAST_DRIVER;
   127	
   128	retry:
   129		res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
   130				HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
   131		if (res) {
 > 132			if (driver_byte(result) != DRIVER_SENSE ||
   133			    !scsi_sense_valid(&sshdr)) {
   134				sdev_printk(KERN_WARNING, sdev,
   135					    "%s: sending start_stop_unit failed, "
   136					    "no sense available\n", HP_SW_NAME);
   137				return SCSI_DH_IO;
   138			}
   139			switch (sshdr.sense_key) {
   140			case NOT_READY:
   141				if (sshdr.asc == 0x04 && sshdr.ascq == 3) {
   142					/*
   143					 * LUN not ready - manual intervention required
   144					 *
   145					 * Switch-over in progress, retry.
   146					 */
   147					if (--retry_cnt)
   148						goto retry;
   149					rc = SCSI_DH_RETRY;
   150					break;
   151				}
   152				/* fall through */
   153			default:
   154				sdev_printk(KERN_WARNING, sdev,
   155					    "%s: sending start_stop_unit failed, "
   156					    "sense %x/%x/%x\n", HP_SW_NAME,
   157					    sshdr.sense_key, sshdr.asc, sshdr.ascq);
   158				rc = SCSI_DH_IO;
   159			}
   160		}
   161		return rc;
   162	}
   163	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 52246 bytes --]

Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[Hannes Reinecke]

On 10/21/19 3:49 AM, zhengbin (A) wrote:
> 
> On 2019/10/18 21:43, Martin K. Petersen wrote:
>> Hannes,
>>
>>> The one thing which I patently don't like is the ambivalence between
>>> DRIVER_SENSE and scsi_sense_valid().  What shall we do if only _one_
>>> of them is set?  IE what would be the correct way of action if
>>> DRIVER_SENSE is not set, but we have a valid sense code?  Or the other
>>> way around?
>> I agree, it's a mess.
>>
>> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
>> and most arcane code in SCSI)
>>
>>> But more important, from a quick glance not all drivers set the
>>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
>>> never evaluated after this patchset.
>> And yet we appear to have several code paths where sense evaluation is
>> contingent on DRIVER_SENSE. So no matter what, behavior might
>> change if we enforce consistent semantics. *sigh*
> 
> So what should we do to prevent unit-value access of sshdr?
> 
Where do you see it?
From my reading, __scsi_execute() is clearing sshdr by way of

__scsi_execute()
-> scsi_normalize_sense()
    -> memset(sshdr)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[zhengbin (A)]

On 2019/10/21 21:06, Hannes Reinecke wrote:
> On 10/21/19 3:49 AM, zhengbin (A) wrote:
>> On 2019/10/18 21:43, Martin K. Petersen wrote:
>>> Hannes,
>>>
>>>> The one thing which I patently don't like is the ambivalence between
>>>> DRIVER_SENSE and scsi_sense_valid().  What shall we do if only _one_
>>>> of them is set?  IE what would be the correct way of action if
>>>> DRIVER_SENSE is not set, but we have a valid sense code?  Or the other
>>>> way around?
>>> I agree, it's a mess.
>>>
>>> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
>>> and most arcane code in SCSI)
>>>
>>>> But more important, from a quick glance not all drivers set the
>>>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
>>>> never evaluated after this patchset.
>>> And yet we appear to have several code paths where sense evaluation is
>>> contingent on DRIVER_SENSE. So no matter what, behavior might
>>> change if we enforce consistent semantics. *sigh*
>> So what should we do to prevent unit-value access of sshdr?
>>
> Where do you see it?
> >From my reading, __scsi_execute() is clearing sshdr by way of
>
> __scsi_execute()
> -> scsi_normalize_sense()
>     -> memset(sshdr)

__scsi_execute

      req = blk_get_request(sdev->request_queue,
            data_direction == DMA_TO_DEVICE ?
            REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
    if (IS_ERR(req))
        return ret;   -->just return
    rq = scsi_req(req);

    if (bufflen &&    blk_rq_map_kern(sdev->request_queue, req,
                    buffer, bufflen, GFP_NOIO))
        goto out;  -->just goto out

>
> Cheers,
>
> Hannes

Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

[zhengbin (A)]

On 2019/10/22 9:59, zhengbin (A) wrote:
> On 2019/10/21 21:06, Hannes Reinecke wrote:
>> On 10/21/19 3:49 AM, zhengbin (A) wrote:
>>> On 2019/10/18 21:43, Martin K. Petersen wrote:
>>>> Hannes,
>>>>
>>>>> The one thing which I patently don't like is the ambivalence between
>>>>> DRIVER_SENSE and scsi_sense_valid().  What shall we do if only _one_
>>>>> of them is set?  IE what would be the correct way of action if
>>>>> DRIVER_SENSE is not set, but we have a valid sense code?  Or the other
>>>>> way around?
>>>> I agree, it's a mess.
>>>>
>>>> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
>>>> and most arcane code in SCSI)
>>>>
>>>>> But more important, from a quick glance not all drivers set the
>>>>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
>>>>> never evaluated after this patchset.
>>>> And yet we appear to have several code paths where sense evaluation is
>>>> contingent on DRIVER_SENSE. So no matter what, behavior might
>>>> change if we enforce consistent semantics. *sigh*
>>> So what should we do to prevent unit-value access of sshdr?
>>>
>> Where do you see it?
>> >From my reading, __scsi_execute() is clearing sshdr by way of
>>
>> __scsi_execute()
>> -> scsi_normalize_sense()
>>     -> memset(sshdr)
> __scsi_execute
>
>       req = blk_get_request(sdev->request_queue,
>             data_direction == DMA_TO_DEVICE ?
>             REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
>     if (IS_ERR(req))
>         return ret;   -->just return
>     rq = scsi_req(req);
>
>     if (bufflen &&    blk_rq_map_kern(sdev->request_queue, req,
>                     buffer, bufflen, GFP_NOIO))
>         goto out;  -->just goto out


may be we should init sshdr in __scsi_execute? which is the simplest way, and do not lose anyone.

If we init sshdr in the callers, maybe we will lose some function.

+	/*
+	 * Zero-initialize sshdr for those callers that check the *sshdr
+	 * contents even if no sense data is available.
+	 */
+	if (sshdr)
+		memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
+

>
>> Cheers,
>>
>> Hannes