[PATCH v3 0/3] target: fix NULL pointer dereference

[PATCH v3 0/3] target: fix NULL pointer dereference

[Sudhakar Panneerselvam]

The following set of commits address a NULL pointer dereference and some
refactoring around this issue.

v3:
 - fix NULL pointer dereference when cdb initialization fails

v2:
 - new helper is named as target_cmd_init_cdb()
 - existing function, target_setup_cmd_from_cdb is renamed as
   target_cmd_parse_cdb()

Sudhakar Panneerselvam (3):
  target: factor out a new helper, target_cmd_init_cdb()
  target: fix NULL pointer dereference
  target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()

 drivers/target/iscsi/iscsi_target.c    | 21 +++++++++++-------
 drivers/target/target_core_transport.c | 40 +++++++++++++++++++++++++++-------
 drivers/target/target_core_xcopy.c     |  5 ++++-
 include/target/target_core_fabric.h    |  3 ++-
 4 files changed, 51 insertions(+), 18 deletions(-)

-- 
1.8.3.1

[PATCH v3 0/3] target: fix NULL pointer dereference

[Sudhakar Panneerselvam]

The following set of commits address a NULL pointer dereference and some
refactoring around this issue.

v3:
 - fix NULL pointer dereference when cdb initialization fails

v2:
 - new helper is named as target_cmd_init_cdb()
 - existing function, target_setup_cmd_from_cdb is renamed as
   target_cmd_parse_cdb()

Sudhakar Panneerselvam (3):
  target: factor out a new helper, target_cmd_init_cdb()
  target: fix NULL pointer dereference
  target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()

 drivers/target/iscsi/iscsi_target.c    | 21 +++++++++++-------
 drivers/target/target_core_transport.c | 40 +++++++++++++++++++++++++++-------
 drivers/target/target_core_xcopy.c     |  5 ++++-
 include/target/target_core_fabric.h    |  3 ++-
 4 files changed, 51 insertions(+), 18 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/3] target: factor out a new helper, target_cmd_init_cdb()

[Sudhakar Panneerselvam]

target_setup_cmd_from_cdb() is called after a successful call to
transport_lookup_cmd_lun(). The new helper factors out the code that can
be called before the call to transport_lookup_cmd_lun(). This helper
will be used in an upcoming commit to address NULL pointer dereference.

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/target_core_transport.c | 16 ++++++++++++----
 include/target/target_core_fabric.h    |  1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index e6b448f43071..f2f7c5b818cc 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1410,11 +1410,8 @@ void transport_init_se_cmd(
 }
 
 sense_reason_t
-target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
+target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
 {
-	struct se_device *dev = cmd->se_dev;
-	sense_reason_t ret;
-
 	/*
 	 * Ensure that the received CDB is less than the max (252 + 8) bytes
 	 * for VARIABLE_LENGTH_CMD
@@ -1448,6 +1445,17 @@ void transport_init_se_cmd(
 	memcpy(cmd->t_task_cdb, cdb, scsi_command_size(cdb));
 
 	trace_target_sequencer_start(cmd);
+	return 0;
+}
+EXPORT_SYMBOL(target_cmd_init_cdb);
+
+sense_reason_t
+target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
+{
+	struct se_device *dev = cmd->se_dev;
+	sense_reason_t ret;
+
+	target_cmd_init_cdb(cmd, cdb);
 
 	ret = dev->transport->parse_cdb(cmd);
 	if (ret = TCM_UNSUPPORTED_SCSI_OPCODE)
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 063f133e47c2..6a2bfcca0c98 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -152,6 +152,7 @@ void	transport_init_se_cmd(struct se_cmd *,
 		const struct target_core_fabric_ops *,
 		struct se_session *, u32, int, int, unsigned char *);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u64);
+sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
 sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
 int	target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *,
 		unsigned char *, unsigned char *, u64, u32, int, int, int,
-- 
1.8.3.1

[PATCH v3 1/3] target: factor out a new helper, target_cmd_init_cdb()

[Sudhakar Panneerselvam]

target_setup_cmd_from_cdb() is called after a successful call to
transport_lookup_cmd_lun(). The new helper factors out the code that can
be called before the call to transport_lookup_cmd_lun(). This helper
will be used in an upcoming commit to address NULL pointer dereference.

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/target_core_transport.c | 16 ++++++++++++----
 include/target/target_core_fabric.h    |  1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index e6b448f43071..f2f7c5b818cc 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1410,11 +1410,8 @@ void transport_init_se_cmd(
 }
 
 sense_reason_t
-target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
+target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
 {
-	struct se_device *dev = cmd->se_dev;
-	sense_reason_t ret;
-
 	/*
 	 * Ensure that the received CDB is less than the max (252 + 8) bytes
 	 * for VARIABLE_LENGTH_CMD
@@ -1448,6 +1445,17 @@ void transport_init_se_cmd(
 	memcpy(cmd->t_task_cdb, cdb, scsi_command_size(cdb));
 
 	trace_target_sequencer_start(cmd);
+	return 0;
+}
+EXPORT_SYMBOL(target_cmd_init_cdb);
+
+sense_reason_t
+target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
+{
+	struct se_device *dev = cmd->se_dev;
+	sense_reason_t ret;
+
+	target_cmd_init_cdb(cmd, cdb);
 
 	ret = dev->transport->parse_cdb(cmd);
 	if (ret == TCM_UNSUPPORTED_SCSI_OPCODE)
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 063f133e47c2..6a2bfcca0c98 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -152,6 +152,7 @@ void	transport_init_se_cmd(struct se_cmd *,
 		const struct target_core_fabric_ops *,
 		struct se_session *, u32, int, int, unsigned char *);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u64);
+sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
 sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
 int	target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *,
 		unsigned char *, unsigned char *, u64, u32, int, int, int,
-- 
1.8.3.1

[PATCH v3 2/3] target: fix NULL pointer dereference

[Sudhakar Panneerselvam]

NULL pointer dereference happens when the following conditions are met
1) A SCSI command is received for a non-existing LU or cdb
initialization fails in target_setup_cmd_from_cdb().
2) Tracing is enabled.

The following call sequences lead to NULL pointer dereference:

1) iscsit_setup_scsi_cmd
     transport_lookup_cmd_lun <-- lookup fails.
          or
     target_setup_cmd_from_cdb() <-- cdb initialization fails
   iscsit_process_scsi_cmd
     iscsit_sequence_cmd
       transport_send_check_condition_and_sense
         trace_target_cmd_complete <-- NULL dereference

2) target_submit_cmd_map_sgls
     transport_lookup_cmd_lun <-- lookup fails
          or
     target_setup_cmd_from_cdb() <-- cdb initialization fails
       transport_send_check_condition_and_sense
         trace_target_cmd_complete <-- NULL dereference

In the above sequence, cmd->t_task_cdb is uninitialized which when
referenced in trace_target_cmd_complete() causes NULL pointer dereference.

The fix is to use the helper, target_cmd_init_cdb() and call it after
transport_init_se_cmd() is called, so that cmd->t_task_cdb can
be initialized and hence can be referenced in trace_target_cmd_complete().

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/iscsi/iscsi_target.c    | 19 ++++++++++++-------
 drivers/target/target_core_transport.c | 28 ++++++++++++++++++++++------
 drivers/target/target_core_xcopy.c     |  3 +++
 3 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 59379d662626..1110ea507b83 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1167,6 +1167,17 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	target_get_sess_cmd(&cmd->se_cmd, true);
 
+	cmd->se_cmd.orig_fe_lun = scsilun_to_int(&hdr->lun);
+	cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb);
+	if (cmd->sense_reason) {
+		if (cmd->sense_reason = TCM_OUT_OF_RESOURCES) {
+			return iscsit_add_reject_cmd(cmd,
+				ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);
+		}
+
+		goto attach_cmd;
+	}
+
 	cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd,
 						     scsilun_to_int(&hdr->lun));
 	if (cmd->sense_reason)
@@ -1175,14 +1186,8 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	/* only used for printks or comparing with ->ref_task_tag */
 	cmd->se_cmd.tag = (__force u32)cmd->init_task_tag;
 	cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb);
-	if (cmd->sense_reason) {
-		if (cmd->sense_reason = TCM_OUT_OF_RESOURCES) {
-			return iscsit_add_reject_cmd(cmd,
-					ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);
-		}
-
+	if (cmd->sense_reason)
 		goto attach_cmd;
-	}
 
 	if (iscsit_build_pdu_and_seq_lists(cmd, payload_length) < 0) {
 		return iscsit_add_reject_cmd(cmd,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index f2f7c5b818cc..4282fa98ff35 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1412,6 +1412,9 @@ void transport_init_se_cmd(
 sense_reason_t
 target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
 {
+	sense_reason_t ret;
+
+	cmd->t_task_cdb = &cmd->__t_task_cdb[0];
 	/*
 	 * Ensure that the received CDB is less than the max (252 + 8) bytes
 	 * for VARIABLE_LENGTH_CMD
@@ -1420,7 +1423,8 @@ void transport_init_se_cmd(
 		pr_err("Received SCSI CDB with command_size: %d that"
 			" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
 			scsi_command_size(cdb), SCSI_MAX_VARLEN_CDB_SIZE);
-		return TCM_INVALID_CDB_FIELD;
+		ret = TCM_INVALID_CDB_FIELD;
+		goto err;
 	}
 	/*
 	 * If the received CDB is larger than TCM_MAX_COMMAND_SIZE,
@@ -1435,10 +1439,10 @@ void transport_init_se_cmd(
 				" %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
 				scsi_command_size(cdb),
 				(unsigned long)sizeof(cmd->__t_task_cdb));
-			return TCM_OUT_OF_RESOURCES;
+			ret = TCM_OUT_OF_RESOURCES;
+			goto err;
 		}
-	} else
-		cmd->t_task_cdb = &cmd->__t_task_cdb[0];
+	}
 	/*
 	 * Copy the original CDB into cmd->
 	 */
@@ -1446,6 +1450,13 @@ void transport_init_se_cmd(
 
 	trace_target_sequencer_start(cmd);
 	return 0;
+
+err:
+	/* Copy the CDB here to allow trace_target_cmd_complete() to
+	 * print the cdb to the trace buffers.
+	 */
+	memcpy(cmd->t_task_cdb, cdb, min(scsi_command_size(cdb), (unsigned int)TCM_MAX_COMMAND_SIZE));
+	return ret;
 }
 EXPORT_SYMBOL(target_cmd_init_cdb);
 
@@ -1455,8 +1466,6 @@ void transport_init_se_cmd(
 	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret;
 
-	target_cmd_init_cdb(cmd, cdb);
-
 	ret = dev->transport->parse_cdb(cmd);
 	if (ret = TCM_UNSUPPORTED_SCSI_OPCODE)
 		pr_warn_ratelimited("%s/%s: Unsupported SCSI Opcode 0x%02x, sending CHECK_CONDITION.\n",
@@ -1598,6 +1607,13 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
 				data_length, data_dir, task_attr, sense);
 
+	se_cmd->orig_fe_lun = unpacked_lun;
+	rc = target_cmd_init_cdb(se_cmd, cdb);
+	if (rc) {
+		transport_send_check_condition_and_sense(se_cmd, rc, 0);
+		return 0;
+	}
+
 	if (flags & TARGET_SCF_USE_CPUID)
 		se_cmd->se_cmd_flags |= SCF_USE_CPUID;
 	else
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index bd3ed6ce7571..fdd8234906b6 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -526,6 +526,9 @@ static int target_xcopy_setup_pt_cmd(
 	}
 	cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 
+	if (target_cmd_init_cdb(cmd, cdb))
+		return -EINVAL;
+
 	cmd->tag = 0;
 	if (target_setup_cmd_from_cdb(cmd, cdb))
 		return -EINVAL;
-- 
1.8.3.1

[PATCH v3 2/3] target: fix NULL pointer dereference

[Sudhakar Panneerselvam]

NULL pointer dereference happens when the following conditions are met
1) A SCSI command is received for a non-existing LU or cdb
initialization fails in target_setup_cmd_from_cdb().
2) Tracing is enabled.

The following call sequences lead to NULL pointer dereference:

1) iscsit_setup_scsi_cmd
     transport_lookup_cmd_lun <-- lookup fails.
          or
     target_setup_cmd_from_cdb() <-- cdb initialization fails
   iscsit_process_scsi_cmd
     iscsit_sequence_cmd
       transport_send_check_condition_and_sense
         trace_target_cmd_complete <-- NULL dereference

2) target_submit_cmd_map_sgls
     transport_lookup_cmd_lun <-- lookup fails
          or
     target_setup_cmd_from_cdb() <-- cdb initialization fails
       transport_send_check_condition_and_sense
         trace_target_cmd_complete <-- NULL dereference

In the above sequence, cmd->t_task_cdb is uninitialized which when
referenced in trace_target_cmd_complete() causes NULL pointer dereference.

The fix is to use the helper, target_cmd_init_cdb() and call it after
transport_init_se_cmd() is called, so that cmd->t_task_cdb can
be initialized and hence can be referenced in trace_target_cmd_complete().

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/iscsi/iscsi_target.c    | 19 ++++++++++++-------
 drivers/target/target_core_transport.c | 28 ++++++++++++++++++++++------
 drivers/target/target_core_xcopy.c     |  3 +++
 3 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 59379d662626..1110ea507b83 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1167,6 +1167,17 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	target_get_sess_cmd(&cmd->se_cmd, true);
 
+	cmd->se_cmd.orig_fe_lun = scsilun_to_int(&hdr->lun);
+	cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb);
+	if (cmd->sense_reason) {
+		if (cmd->sense_reason == TCM_OUT_OF_RESOURCES) {
+			return iscsit_add_reject_cmd(cmd,
+				ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);
+		}
+
+		goto attach_cmd;
+	}
+
 	cmd->sense_reason = transport_lookup_cmd_lun(&cmd->se_cmd,
 						     scsilun_to_int(&hdr->lun));
 	if (cmd->sense_reason)
@@ -1175,14 +1186,8 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	/* only used for printks or comparing with ->ref_task_tag */
 	cmd->se_cmd.tag = (__force u32)cmd->init_task_tag;
 	cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb);
-	if (cmd->sense_reason) {
-		if (cmd->sense_reason == TCM_OUT_OF_RESOURCES) {
-			return iscsit_add_reject_cmd(cmd,
-					ISCSI_REASON_BOOKMARK_NO_RESOURCES, buf);
-		}
-
+	if (cmd->sense_reason)
 		goto attach_cmd;
-	}
 
 	if (iscsit_build_pdu_and_seq_lists(cmd, payload_length) < 0) {
 		return iscsit_add_reject_cmd(cmd,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index f2f7c5b818cc..4282fa98ff35 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1412,6 +1412,9 @@ void transport_init_se_cmd(
 sense_reason_t
 target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
 {
+	sense_reason_t ret;
+
+	cmd->t_task_cdb = &cmd->__t_task_cdb[0];
 	/*
 	 * Ensure that the received CDB is less than the max (252 + 8) bytes
 	 * for VARIABLE_LENGTH_CMD
@@ -1420,7 +1423,8 @@ void transport_init_se_cmd(
 		pr_err("Received SCSI CDB with command_size: %d that"
 			" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
 			scsi_command_size(cdb), SCSI_MAX_VARLEN_CDB_SIZE);
-		return TCM_INVALID_CDB_FIELD;
+		ret = TCM_INVALID_CDB_FIELD;
+		goto err;
 	}
 	/*
 	 * If the received CDB is larger than TCM_MAX_COMMAND_SIZE,
@@ -1435,10 +1439,10 @@ void transport_init_se_cmd(
 				" %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
 				scsi_command_size(cdb),
 				(unsigned long)sizeof(cmd->__t_task_cdb));
-			return TCM_OUT_OF_RESOURCES;
+			ret = TCM_OUT_OF_RESOURCES;
+			goto err;
 		}
-	} else
-		cmd->t_task_cdb = &cmd->__t_task_cdb[0];
+	}
 	/*
 	 * Copy the original CDB into cmd->
 	 */
@@ -1446,6 +1450,13 @@ void transport_init_se_cmd(
 
 	trace_target_sequencer_start(cmd);
 	return 0;
+
+err:
+	/* Copy the CDB here to allow trace_target_cmd_complete() to
+	 * print the cdb to the trace buffers.
+	 */
+	memcpy(cmd->t_task_cdb, cdb, min(scsi_command_size(cdb), (unsigned int)TCM_MAX_COMMAND_SIZE));
+	return ret;
 }
 EXPORT_SYMBOL(target_cmd_init_cdb);
 
@@ -1455,8 +1466,6 @@ void transport_init_se_cmd(
 	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret;
 
-	target_cmd_init_cdb(cmd, cdb);
-
 	ret = dev->transport->parse_cdb(cmd);
 	if (ret == TCM_UNSUPPORTED_SCSI_OPCODE)
 		pr_warn_ratelimited("%s/%s: Unsupported SCSI Opcode 0x%02x, sending CHECK_CONDITION.\n",
@@ -1598,6 +1607,13 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
 				data_length, data_dir, task_attr, sense);
 
+	se_cmd->orig_fe_lun = unpacked_lun;
+	rc = target_cmd_init_cdb(se_cmd, cdb);
+	if (rc) {
+		transport_send_check_condition_and_sense(se_cmd, rc, 0);
+		return 0;
+	}
+
 	if (flags & TARGET_SCF_USE_CPUID)
 		se_cmd->se_cmd_flags |= SCF_USE_CPUID;
 	else
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index bd3ed6ce7571..fdd8234906b6 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -526,6 +526,9 @@ static int target_xcopy_setup_pt_cmd(
 	}
 	cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 
+	if (target_cmd_init_cdb(cmd, cdb))
+		return -EINVAL;
+
 	cmd->tag = 0;
 	if (target_setup_cmd_from_cdb(cmd, cdb))
 		return -EINVAL;
-- 
1.8.3.1

[PATCH v3 3/3] target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()

[Sudhakar Panneerselvam]

This commit also removes the unused argument, cdb that was passed
to this function.

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/iscsi/iscsi_target.c    | 2 +-
 drivers/target/target_core_transport.c | 6 +++---
 drivers/target/target_core_xcopy.c     | 2 +-
 include/target/target_core_fabric.h    | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 1110ea507b83..c0966dd4937c 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1185,7 +1185,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	/* only used for printks or comparing with ->ref_task_tag */
 	cmd->se_cmd.tag = (__force u32)cmd->init_task_tag;
-	cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb);
+	cmd->sense_reason = target_cmd_parse_cdb(&cmd->se_cmd);
 	if (cmd->sense_reason)
 		goto attach_cmd;
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4282fa98ff35..a55a8aaae6b3 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1461,7 +1461,7 @@ void transport_init_se_cmd(
 EXPORT_SYMBOL(target_cmd_init_cdb);
 
 sense_reason_t
-target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
+target_cmd_parse_cdb(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret;
@@ -1483,7 +1483,7 @@ void transport_init_se_cmd(
 	atomic_long_inc(&cmd->se_lun->lun_stats.cmd_pdus);
 	return 0;
 }
-EXPORT_SYMBOL(target_setup_cmd_from_cdb);
+EXPORT_SYMBOL(target_cmd_parse_cdb);
 
 /*
  * Used by fabric module frontends to queue tasks directly.
@@ -1645,7 +1645,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 		return 0;
 	}
 
-	rc = target_setup_cmd_from_cdb(se_cmd, cdb);
+	rc = target_cmd_parse_cdb(se_cmd);
 	if (rc != 0) {
 		transport_generic_request_failure(se_cmd, rc);
 		return 0;
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index fdd8234906b6..842563ba10f6 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -530,7 +530,7 @@ static int target_xcopy_setup_pt_cmd(
 		return -EINVAL;
 
 	cmd->tag = 0;
-	if (target_setup_cmd_from_cdb(cmd, cdb))
+	if (target_cmd_parse_cdb(cmd))
 		return -EINVAL;
 
 	if (transport_generic_map_mem_to_cmd(cmd, xop->xop_data_sg,
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 6a2bfcca0c98..1e7ea57d0463 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -153,7 +153,7 @@ void	transport_init_se_cmd(struct se_cmd *,
 		struct se_session *, u32, int, int, unsigned char *);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u64);
 sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
-sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
+sense_reason_t target_cmd_parse_cdb(struct se_cmd *);
 int	target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *,
 		unsigned char *, unsigned char *, u64, u32, int, int, int,
 		struct scatterlist *, u32, struct scatterlist *, u32,
-- 
1.8.3.1

[PATCH v3 3/3] target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()

[Sudhakar Panneerselvam]

This commit also removes the unused argument, cdb that was passed
to this function.

Signed-off-by: Sudhakar Panneerselvam <sudhakar.panneerselvam@oracle.com>
---
 drivers/target/iscsi/iscsi_target.c    | 2 +-
 drivers/target/target_core_transport.c | 6 +++---
 drivers/target/target_core_xcopy.c     | 2 +-
 include/target/target_core_fabric.h    | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 1110ea507b83..c0966dd4937c 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1185,7 +1185,7 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	/* only used for printks or comparing with ->ref_task_tag */
 	cmd->se_cmd.tag = (__force u32)cmd->init_task_tag;
-	cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb);
+	cmd->sense_reason = target_cmd_parse_cdb(&cmd->se_cmd);
 	if (cmd->sense_reason)
 		goto attach_cmd;
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4282fa98ff35..a55a8aaae6b3 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1461,7 +1461,7 @@ void transport_init_se_cmd(
 EXPORT_SYMBOL(target_cmd_init_cdb);
 
 sense_reason_t
-target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
+target_cmd_parse_cdb(struct se_cmd *cmd)
 {
 	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret;
@@ -1483,7 +1483,7 @@ void transport_init_se_cmd(
 	atomic_long_inc(&cmd->se_lun->lun_stats.cmd_pdus);
 	return 0;
 }
-EXPORT_SYMBOL(target_setup_cmd_from_cdb);
+EXPORT_SYMBOL(target_cmd_parse_cdb);
 
 /*
  * Used by fabric module frontends to queue tasks directly.
@@ -1645,7 +1645,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 		return 0;
 	}
 
-	rc = target_setup_cmd_from_cdb(se_cmd, cdb);
+	rc = target_cmd_parse_cdb(se_cmd);
 	if (rc != 0) {
 		transport_generic_request_failure(se_cmd, rc);
 		return 0;
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index fdd8234906b6..842563ba10f6 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -530,7 +530,7 @@ static int target_xcopy_setup_pt_cmd(
 		return -EINVAL;
 
 	cmd->tag = 0;
-	if (target_setup_cmd_from_cdb(cmd, cdb))
+	if (target_cmd_parse_cdb(cmd))
 		return -EINVAL;
 
 	if (transport_generic_map_mem_to_cmd(cmd, xop->xop_data_sg,
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 6a2bfcca0c98..1e7ea57d0463 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -153,7 +153,7 @@ void	transport_init_se_cmd(struct se_cmd *,
 		struct se_session *, u32, int, int, unsigned char *);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *, u64);
 sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
-sense_reason_t target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
+sense_reason_t target_cmd_parse_cdb(struct se_cmd *);
 int	target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *,
 		unsigned char *, unsigned char *, u64, u32, int, int, int,
 		struct scatterlist *, u32, struct scatterlist *, u32,
-- 
1.8.3.1

Re: [PATCH v3 2/3] target: fix NULL pointer dereference

[Mike Christie]

On 6/2/20 1:33 PM, Sudhakar Panneerselvam wrote:
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index f2f7c5b818cc..4282fa98ff35 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1412,6 +1412,9 @@ void transport_init_se_cmd(
>   sense_reason_t
>   target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
>   {
> +	sense_reason_t ret;
> +
> +	cmd->t_task_cdb = &cmd->__t_task_cdb[0];
>   	/*
>   	 * Ensure that the received CDB is less than the max (252 + 8) bytes
>   	 * for VARIABLE_LENGTH_CMD
> @@ -1420,7 +1423,8 @@ void transport_init_se_cmd(
>   		pr_err("Received SCSI CDB with command_size: %d that"
>   			" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
>   			scsi_command_size(cdb), SCSI_MAX_VARLEN_CDB_SIZE);
> -		return TCM_INVALID_CDB_FIELD;
> +		ret = TCM_INVALID_CDB_FIELD;
> +		goto err;
>   	}
>   	/*
>   	 * If the received CDB is larger than TCM_MAX_COMMAND_SIZE,
> @@ -1435,10 +1439,10 @@ void transport_init_se_cmd(
>   				" %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
>   				scsi_command_size(cdb),
>   				(unsigned long)sizeof(cmd->__t_task_cdb));
> -			return TCM_OUT_OF_RESOURCES;
> +			ret = TCM_OUT_OF_RESOURCES;
> +			goto err;
>   		}
> -	} else
> -		cmd->t_task_cdb = &cmd->__t_task_cdb[0];
> +	}
>   	/*
>   	 * Copy the original CDB into cmd->
>   	 */
> @@ -1446,6 +1450,13 @@ void transport_init_se_cmd(
>   
>   	trace_target_sequencer_start(cmd);
>   	return 0;
> +
> +err:
> +	/* Copy the CDB here to allow trace_target_cmd_complete() to

You should follow the coding style in the rest of the code. Do "/*" then 
start your text or do it all on one line if it fits:

/*
  * Copy the CDB here to allow trace_target_cmd_complete() to


> +	 * print the cdb to the trace buffers.
> +	 */
> +	memcpy(cmd->t_task_cdb, cdb, min(scsi_command_size(cdb), (unsigned int)TCM_MAX_COMMAND_SIZE));

Use 80 char cols like you did in the rest of the patch and the other code.

> +	return ret;
>   }
>   EXPORT_SYMBOL(target_cmd_init_cdb);
>   
> @@ -1455,8 +1466,6 @@ void transport_init_se_cmd(
>   	struct se_device *dev = cmd->se_dev;
>   	sense_reason_t ret;
>   
> -	target_cmd_init_cdb(cmd, cdb);
> -
>   	ret = dev->transport->parse_cdb(cmd);
>   	if (ret = TCM_UNSUPPORTED_SCSI_OPCODE)
>   		pr_warn_ratelimited("%s/%s: Unsupported SCSI Opcode 0x%02x, sending CHECK_CONDITION.\n",
> @@ -1598,6 +1607,13 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
>   	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
>   				data_length, data_dir, task_attr, sense);
>   

This should maybe be in transport_init_se_cmd. It might be useful there 
for the tmr case, if we wanted to add a trace point there too.

At least a comment and some cleanup, because it's not obvious why we set 
it here then also set it again in transport_lookup_cmd_lun.


> +	se_cmd->orig_fe_lun = unpacked_lun; > +	rc = target_cmd_init_cdb(se_cmd, cdb);
> +	if (rc) {
> +		transport_send_check_condition_and_sense(se_cmd, rc, 0);

Can we do this before doing a get() on the cmd? If the fabric module is 
such that it does a put() on the cmd (the callers using SCF_ACK_KREF) in 
its cmd clean up path, then we would end up with unbalanced 
sess->cmd_count and cmd refcounts.

Maybe move this to after target_get_sess_cmd().


> +		return 0;
> +	}
> +
>   	if (flags & TARGET_SCF_USE_CPUID)
>   		se_cmd->se_cmd_flags |= SCF_USE_CPUID;
>   	else
> diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
> index bd3ed6ce7571..fdd8234906b6 100644
> --- a/drivers/target/target_core_xcopy.c
> +++ b/drivers/target/target_core_xcopy.c
> @@ -526,6 +526,9 @@ static int target_xcopy_setup_pt_cmd(
>   	}
>   	cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
>   
> +	if (target_cmd_init_cdb(cmd, cdb))
> +		return -EINVAL;
> +
>   	cmd->tag = 0;
>   	if (target_setup_cmd_from_cdb(cmd, cdb))
>   		return -EINVAL;
> 

Re: [PATCH v3 2/3] target: fix NULL pointer dereference

[Mike Christie]

On 6/2/20 1:33 PM, Sudhakar Panneerselvam wrote:
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index f2f7c5b818cc..4282fa98ff35 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1412,6 +1412,9 @@ void transport_init_se_cmd(
>   sense_reason_t
>   target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
>   {
> +	sense_reason_t ret;
> +
> +	cmd->t_task_cdb = &cmd->__t_task_cdb[0];
>   	/*
>   	 * Ensure that the received CDB is less than the max (252 + 8) bytes
>   	 * for VARIABLE_LENGTH_CMD
> @@ -1420,7 +1423,8 @@ void transport_init_se_cmd(
>   		pr_err("Received SCSI CDB with command_size: %d that"
>   			" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
>   			scsi_command_size(cdb), SCSI_MAX_VARLEN_CDB_SIZE);
> -		return TCM_INVALID_CDB_FIELD;
> +		ret = TCM_INVALID_CDB_FIELD;
> +		goto err;
>   	}
>   	/*
>   	 * If the received CDB is larger than TCM_MAX_COMMAND_SIZE,
> @@ -1435,10 +1439,10 @@ void transport_init_se_cmd(
>   				" %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
>   				scsi_command_size(cdb),
>   				(unsigned long)sizeof(cmd->__t_task_cdb));
> -			return TCM_OUT_OF_RESOURCES;
> +			ret = TCM_OUT_OF_RESOURCES;
> +			goto err;
>   		}
> -	} else
> -		cmd->t_task_cdb = &cmd->__t_task_cdb[0];
> +	}
>   	/*
>   	 * Copy the original CDB into cmd->
>   	 */
> @@ -1446,6 +1450,13 @@ void transport_init_se_cmd(
>   
>   	trace_target_sequencer_start(cmd);
>   	return 0;
> +
> +err:
> +	/* Copy the CDB here to allow trace_target_cmd_complete() to

You should follow the coding style in the rest of the code. Do "/*" then 
start your text or do it all on one line if it fits:

/*
  * Copy the CDB here to allow trace_target_cmd_complete() to


> +	 * print the cdb to the trace buffers.
> +	 */
> +	memcpy(cmd->t_task_cdb, cdb, min(scsi_command_size(cdb), (unsigned int)TCM_MAX_COMMAND_SIZE));

Use 80 char cols like you did in the rest of the patch and the other code.

> +	return ret;
>   }
>   EXPORT_SYMBOL(target_cmd_init_cdb);
>   
> @@ -1455,8 +1466,6 @@ void transport_init_se_cmd(
>   	struct se_device *dev = cmd->se_dev;
>   	sense_reason_t ret;
>   
> -	target_cmd_init_cdb(cmd, cdb);
> -
>   	ret = dev->transport->parse_cdb(cmd);
>   	if (ret == TCM_UNSUPPORTED_SCSI_OPCODE)
>   		pr_warn_ratelimited("%s/%s: Unsupported SCSI Opcode 0x%02x, sending CHECK_CONDITION.\n",
> @@ -1598,6 +1607,13 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
>   	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
>   				data_length, data_dir, task_attr, sense);
>   

This should maybe be in transport_init_se_cmd. It might be useful there 
for the tmr case, if we wanted to add a trace point there too.

At least a comment and some cleanup, because it's not obvious why we set 
it here then also set it again in transport_lookup_cmd_lun.


> +	se_cmd->orig_fe_lun = unpacked_lun; > +	rc = target_cmd_init_cdb(se_cmd, cdb);
> +	if (rc) {
> +		transport_send_check_condition_and_sense(se_cmd, rc, 0);

Can we do this before doing a get() on the cmd? If the fabric module is 
such that it does a put() on the cmd (the callers using SCF_ACK_KREF) in 
its cmd clean up path, then we would end up with unbalanced 
sess->cmd_count and cmd refcounts.

Maybe move this to after target_get_sess_cmd().


> +		return 0;
> +	}
> +
>   	if (flags & TARGET_SCF_USE_CPUID)
>   		se_cmd->se_cmd_flags |= SCF_USE_CPUID;
>   	else
> diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
> index bd3ed6ce7571..fdd8234906b6 100644
> --- a/drivers/target/target_core_xcopy.c
> +++ b/drivers/target/target_core_xcopy.c
> @@ -526,6 +526,9 @@ static int target_xcopy_setup_pt_cmd(
>   	}
>   	cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
>   
> +	if (target_cmd_init_cdb(cmd, cdb))
> +		return -EINVAL;
> +
>   	cmd->tag = 0;
>   	if (target_setup_cmd_from_cdb(cmd, cdb))
>   		return -EINVAL;
> 

RE: [PATCH v3 2/3] target: fix NULL pointer dereference

[Sudhakar Panneerselvam]

> 
> You should follow the coding style in the rest of the code. Do "/*" then
> start your text or do it all on one line if it fits:
> 
> /*
>   * Copy the CDB here to allow trace_target_cmd_complete() to

Thanks, I will fix this.

> 
> 
> > +	 * print the cdb to the trace buffers.
> > +	 */
> > +	memcpy(cmd->t_task_cdb, cdb, min(scsi_command_size(cdb), (unsigned
> int)TCM_MAX_COMMAND_SIZE));
> 
> Use 80 char cols like you did in the rest of the patch and the other code.

I recently noticed that 80 char limitation was relaxed from mainline by commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144. The new limit is 100 char. I was confused whether to stick to 80 or the new limit. Let me know.

> 
> > +	return ret;
> >   }
> >   EXPORT_SYMBOL(target_cmd_init_cdb);
> >
> > @@ -1455,8 +1466,6 @@ void transport_init_se_cmd(
> >   	struct se_device *dev = cmd->se_dev;
> >   	sense_reason_t ret;
> >
> > -	target_cmd_init_cdb(cmd, cdb);
> > -
> >   	ret = dev->transport->parse_cdb(cmd);
> >   	if (ret == TCM_UNSUPPORTED_SCSI_OPCODE)
> >   		pr_warn_ratelimited("%s/%s: Unsupported SCSI Opcode 0x%02x,
> sending CHECK_CONDITION.\n",
> > @@ -1598,6 +1607,13 @@ int target_submit_cmd_map_sgls(struct se_cmd
> *se_cmd, struct se_session *se_sess
> >   	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
> >   				data_length, data_dir, task_attr, sense);
> >
> 
> This should maybe be in transport_init_se_cmd. It might be useful there
> for the tmr case, if we wanted to add a trace point there too.
> 
> At least a comment and some cleanup, because it's not obvious why we set
> it here then also set it again in transport_lookup_cmd_lun.

Yes, I thought of initializing the cdb in transport_init_se_cmd() but realized later that TMR requests are transport level entities and hence they don't have an associated cdb with them. So, in future if we want to trace tmr request, then we may have to introduce new set of trace functions that do not reference cdb. What do you think?

> 
> 
> > +	se_cmd->orig_fe_lun = unpacked_lun; > +	rc =
> target_cmd_init_cdb(se_cmd, cdb);
> > +	if (rc) {
> > +		transport_send_check_condition_and_sense(se_cmd, rc, 0);
> 
> Can we do this before doing a get() on the cmd? If the fabric module is
> such that it does a put() on the cmd (the callers using SCF_ACK_KREF) in
> its cmd clean up path, then we would end up with unbalanced
> sess->cmd_count and cmd refcounts.
> 
> Maybe move this to after target_get_sess_cmd().

I moved it before target_get_sess_cmd() because if target_get_sess_cmd() fails then we have NULL pointer dereference issue again. For instance, the sequence
  vhost_scsi_submission_work
     target_submit_cmd_map_sgls
       target_get_sess_cmd() -- Suppose this fails
     transport_send_check_condition_and_sense
        trace_target_cmd_complete -- NULL ptr derefence.

Still thinking how to address both these issues together.

Thanks
Sudhakar

RE: [PATCH v3 2/3] target: fix NULL pointer dereference

[Sudhakar Panneerselvam]

> 
> You should follow the coding style in the rest of the code. Do "/*" then
> start your text or do it all on one line if it fits:
> 
> /*
>   * Copy the CDB here to allow trace_target_cmd_complete() to

Thanks, I will fix this.

> 
> 
> > +	 * print the cdb to the trace buffers.
> > +	 */
> > +	memcpy(cmd->t_task_cdb, cdb, min(scsi_command_size(cdb), (unsigned
> int)TCM_MAX_COMMAND_SIZE));
> 
> Use 80 char cols like you did in the rest of the patch and the other code.

I recently noticed that 80 char limitation was relaxed from mainline by commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144. The new limit is 100 char. I was confused whether to stick to 80 or the new limit. Let me know.

> 
> > +	return ret;
> >   }
> >   EXPORT_SYMBOL(target_cmd_init_cdb);
> >
> > @@ -1455,8 +1466,6 @@ void transport_init_se_cmd(
> >   	struct se_device *dev = cmd->se_dev;
> >   	sense_reason_t ret;
> >
> > -	target_cmd_init_cdb(cmd, cdb);
> > -
> >   	ret = dev->transport->parse_cdb(cmd);
> >   	if (ret == TCM_UNSUPPORTED_SCSI_OPCODE)
> >   		pr_warn_ratelimited("%s/%s: Unsupported SCSI Opcode 0x%02x,
> sending CHECK_CONDITION.\n",
> > @@ -1598,6 +1607,13 @@ int target_submit_cmd_map_sgls(struct se_cmd
> *se_cmd, struct se_session *se_sess
> >   	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
> >   				data_length, data_dir, task_attr, sense);
> >
> 
> This should maybe be in transport_init_se_cmd. It might be useful there
> for the tmr case, if we wanted to add a trace point there too.
> 
> At least a comment and some cleanup, because it's not obvious why we set
> it here then also set it again in transport_lookup_cmd_lun.

Yes, I thought of initializing the cdb in transport_init_se_cmd() but realized later that TMR requests are transport level entities and hence they don't have an associated cdb with them. So, in future if we want to trace tmr request, then we may have to introduce new set of trace functions that do not reference cdb. What do you think?

> 
> 
> > +	se_cmd->orig_fe_lun = unpacked_lun; > +	rc =
> target_cmd_init_cdb(se_cmd, cdb);
> > +	if (rc) {
> > +		transport_send_check_condition_and_sense(se_cmd, rc, 0);
> 
> Can we do this before doing a get() on the cmd? If the fabric module is
> such that it does a put() on the cmd (the callers using SCF_ACK_KREF) in
> its cmd clean up path, then we would end up with unbalanced
> sess->cmd_count and cmd refcounts.
> 
> Maybe move this to after target_get_sess_cmd().

I moved it before target_get_sess_cmd() because if target_get_sess_cmd() fails then we have NULL pointer dereference issue again. For instance, the sequence
  vhost_scsi_submission_work
     target_submit_cmd_map_sgls
       target_get_sess_cmd() -- Suppose this fails
     transport_send_check_condition_and_sense
        trace_target_cmd_complete -- NULL ptr derefence.

Still thinking how to address both these issues together.

Thanks
Sudhakar

RE: [PATCH v3 2/3] target: fix NULL pointer dereference

[Sudhakar Panneerselvam]

> > Maybe move this to after target_get_sess_cmd().
> 
> I moved it before target_get_sess_cmd() because if target_get_sess_cmd() fails
> then we have NULL pointer dereference issue again. For instance, the sequence
>   vhost_scsi_submission_work
>      target_submit_cmd_map_sgls
>        target_get_sess_cmd() -- Suppose this fails
>      transport_send_check_condition_and_sense
>         trace_target_cmd_complete -- NULL ptr derefence.
> 
> Still thinking how to address both these issues together.

Also, noticed that not all callers of target_get_sess_cmd() check for return value.( iscsit_setup_scsi_cmd() and iscsit_handle_task_mgt_cmd()). Could this cause problems?

-Sudhakar

RE: [PATCH v3 2/3] target: fix NULL pointer dereference

[Sudhakar Panneerselvam]

> > Maybe move this to after target_get_sess_cmd().
> 
> I moved it before target_get_sess_cmd() because if target_get_sess_cmd() fails
> then we have NULL pointer dereference issue again. For instance, the sequence
>   vhost_scsi_submission_work
>      target_submit_cmd_map_sgls
>        target_get_sess_cmd() -- Suppose this fails
>      transport_send_check_condition_and_sense
>         trace_target_cmd_complete -- NULL ptr derefence.
> 
> Still thinking how to address both these issues together.

Also, noticed that not all callers of target_get_sess_cmd() check for return value.( iscsit_setup_scsi_cmd() and iscsit_handle_task_mgt_cmd()). Could this cause problems?

-Sudhakar

Re: [PATCH v3 2/3] target: fix NULL pointer dereference

[Mike Christie]

On 6/2/20 5:37 PM, Sudhakar Panneerselvam wrote:
>>
>> You should follow the coding style in the rest of the code. Do "/*" then
>> start your text or do it all on one line if it fits:
>>
>> /*
>>    * Copy the CDB here to allow trace_target_cmd_complete() to
> 
> Thanks, I will fix this.
> 
>>
>>
>>> +	 * print the cdb to the trace buffers.
>>> +	 */
>>> +	memcpy(cmd->t_task_cdb, cdb, min(scsi_command_size(cdb), (unsigned
>> int)TCM_MAX_COMMAND_SIZE));
>>
>> Use 80 char cols like you did in the rest of the patch and the other code.
> 
> I recently noticed that 80 char limitation was relaxed from mainline by commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144. The new limit is 100 char. I was confused whether to stick to 80 or the new limit. Let me know.
> 

I would normally stick with what's in the existing code, because it 
still says that the preferred limit is 80. For cases where readbility is 
an issue then I would go up to 100.


>>
>>> +	return ret;
>>>    }
>>>    EXPORT_SYMBOL(target_cmd_init_cdb);
>>>
>>> @@ -1455,8 +1466,6 @@ void transport_init_se_cmd(
>>>    	struct se_device *dev = cmd->se_dev;
>>>    	sense_reason_t ret;
>>>
>>> -	target_cmd_init_cdb(cmd, cdb);
>>> -
>>>    	ret = dev->transport->parse_cdb(cmd);
>>>    	if (ret = TCM_UNSUPPORTED_SCSI_OPCODE)
>>>    		pr_warn_ratelimited("%s/%s: Unsupported SCSI Opcode 0x%02x,
>> sending CHECK_CONDITION.\n",
>>> @@ -1598,6 +1607,13 @@ int target_submit_cmd_map_sgls(struct se_cmd
>> *se_cmd, struct se_session *se_sess
>>>    	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
>>>    				data_length, data_dir, task_attr, sense);
>>>
>>
>> This should maybe be in transport_init_se_cmd. It might be useful there
>> for the tmr case, if we wanted to add a trace point there too.
>>
>> At least a comment and some cleanup, because it's not obvious why we set
>> it here then also set it again in transport_lookup_cmd_lun.
> 
> Yes, I thought of initializing the cdb in transport_init_se_cmd() but realized later that TMR requests are transport level entities and hence they don't have an associated cdb with them. So, in future if we want to trace tmr request, then we may have to introduce new set of trace functions that do not reference cdb. What do you think?

I'm just talking about the LUN value and not the cdb here. In my opinion 
it's just a matter of initializing fields in transport_init_se_cmd that 
we later reference instead of having the initializations scattered 
around in multiple places.

I'm not talking about having a common trace function for the tmr and non 
tmr paths.

Also, for the cdb case the init in the target_cmd_init_cdb seems nice to 
me, because it's clear that is where we are setting up the cdb related 
fields.


> 
>>
>>
>>> +	se_cmd->orig_fe_lun = unpacked_lun; > +	rc >> target_cmd_init_cdb(se_cmd, cdb);
>>> +	if (rc) {
>>> +		transport_send_check_condition_and_sense(se_cmd, rc, 0);
>>
>> Can we do this before doing a get() on the cmd? If the fabric module is
>> such that it does a put() on the cmd (the callers using SCF_ACK_KREF) in
>> its cmd clean up path, then we would end up with unbalanced
>> sess->cmd_count and cmd refcounts.
>>
>> Maybe move this to after target_get_sess_cmd().
> 
> I moved it before target_get_sess_cmd() because if target_get_sess_cmd() fails then we have NULL pointer dereference issue again. For instance, the sequence

Yeah, that's why I noticed the issue :) You didn't update the 
target_get_sess_cmd failure path to do 
transport_send_check_condition_and_sense even though you moved the cdb 
init before the get() call, so the code looked off.


>    vhost_scsi_submission_wor >       target_submit_cmd_map_sgls
>         target_get_sess_cmd() -- Suppose this fails
>       transport_send_check_condition_and_sense >          trace_target_cmd_complete -- NULL ptr derefence.
> 
> Still thinking how to address both these issues together.
> 

Maybe you need a new trace call for the case where we can't fully 
initialize the cmd. It could be used for cases like where 
transport_generic_new_cmd is used directly but fails, the 
transport_handle_queue_full case, and your case where we fail during the 
initial setup.

Re: [PATCH v3 2/3] target: fix NULL pointer dereference

[Mike Christie]

On 6/2/20 5:37 PM, Sudhakar Panneerselvam wrote:
>>
>> You should follow the coding style in the rest of the code. Do "/*" then
>> start your text or do it all on one line if it fits:
>>
>> /*
>>    * Copy the CDB here to allow trace_target_cmd_complete() to
> 
> Thanks, I will fix this.
> 
>>
>>
>>> +	 * print the cdb to the trace buffers.
>>> +	 */
>>> +	memcpy(cmd->t_task_cdb, cdb, min(scsi_command_size(cdb), (unsigned
>> int)TCM_MAX_COMMAND_SIZE));
>>
>> Use 80 char cols like you did in the rest of the patch and the other code.
> 
> I recently noticed that 80 char limitation was relaxed from mainline by commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144. The new limit is 100 char. I was confused whether to stick to 80 or the new limit. Let me know.
> 

I would normally stick with what's in the existing code, because it 
still says that the preferred limit is 80. For cases where readbility is 
an issue then I would go up to 100.


>>
>>> +	return ret;
>>>    }
>>>    EXPORT_SYMBOL(target_cmd_init_cdb);
>>>
>>> @@ -1455,8 +1466,6 @@ void transport_init_se_cmd(
>>>    	struct se_device *dev = cmd->se_dev;
>>>    	sense_reason_t ret;
>>>
>>> -	target_cmd_init_cdb(cmd, cdb);
>>> -
>>>    	ret = dev->transport->parse_cdb(cmd);
>>>    	if (ret == TCM_UNSUPPORTED_SCSI_OPCODE)
>>>    		pr_warn_ratelimited("%s/%s: Unsupported SCSI Opcode 0x%02x,
>> sending CHECK_CONDITION.\n",
>>> @@ -1598,6 +1607,13 @@ int target_submit_cmd_map_sgls(struct se_cmd
>> *se_cmd, struct se_session *se_sess
>>>    	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
>>>    				data_length, data_dir, task_attr, sense);
>>>
>>
>> This should maybe be in transport_init_se_cmd. It might be useful there
>> for the tmr case, if we wanted to add a trace point there too.
>>
>> At least a comment and some cleanup, because it's not obvious why we set
>> it here then also set it again in transport_lookup_cmd_lun.
> 
> Yes, I thought of initializing the cdb in transport_init_se_cmd() but realized later that TMR requests are transport level entities and hence they don't have an associated cdb with them. So, in future if we want to trace tmr request, then we may have to introduce new set of trace functions that do not reference cdb. What do you think?

I'm just talking about the LUN value and not the cdb here. In my opinion 
it's just a matter of initializing fields in transport_init_se_cmd that 
we later reference instead of having the initializations scattered 
around in multiple places.

I'm not talking about having a common trace function for the tmr and non 
tmr paths.

Also, for the cdb case the init in the target_cmd_init_cdb seems nice to 
me, because it's clear that is where we are setting up the cdb related 
fields.


> 
>>
>>
>>> +	se_cmd->orig_fe_lun = unpacked_lun; > +	rc =
>> target_cmd_init_cdb(se_cmd, cdb);
>>> +	if (rc) {
>>> +		transport_send_check_condition_and_sense(se_cmd, rc, 0);
>>
>> Can we do this before doing a get() on the cmd? If the fabric module is
>> such that it does a put() on the cmd (the callers using SCF_ACK_KREF) in
>> its cmd clean up path, then we would end up with unbalanced
>> sess->cmd_count and cmd refcounts.
>>
>> Maybe move this to after target_get_sess_cmd().
> 
> I moved it before target_get_sess_cmd() because if target_get_sess_cmd() fails then we have NULL pointer dereference issue again. For instance, the sequence

Yeah, that's why I noticed the issue :) You didn't update the 
target_get_sess_cmd failure path to do 
transport_send_check_condition_and_sense even though you moved the cdb 
init before the get() call, so the code looked off.


>    vhost_scsi_submission_wor >       target_submit_cmd_map_sgls
>         target_get_sess_cmd() -- Suppose this fails
>       transport_send_check_condition_and_sense >          trace_target_cmd_complete -- NULL ptr derefence.
> 
> Still thinking how to address both these issues together.
> 

Maybe you need a new trace call for the case where we can't fully 
initialize the cmd. It could be used for cases like where 
transport_generic_new_cmd is used directly but fails, the 
transport_handle_queue_full case, and your case where we fail during the 
initial setup.

Re: [PATCH v3 2/3] target: fix NULL pointer dereference

[Mike Christie]

On 6/2/20 6:01 PM, Sudhakar Panneerselvam wrote:
>>> Maybe move this to after target_get_sess_cmd().
>>
>> I moved it before target_get_sess_cmd() because if target_get_sess_cmd() fails
>> then we have NULL pointer dereference issue again. For instance, the sequence
>>    vhost_scsi_submission_work
>>       target_submit_cmd_map_sgls
>>         target_get_sess_cmd() -- Suppose this fails
>>       transport_send_check_condition_and_sense
>>          trace_target_cmd_complete -- NULL ptr derefence.
>>
>> Still thinking how to address both these issues together.
> 
> Also, noticed that not all callers of target_get_sess_cmd() check for return value.( iscsit_setup_scsi_cmd() and iscsit_handle_task_mgt_cmd()). Could this cause problems?
> 

I think it's ok. iscsi doesn't use target_sess_cmd_list_set_waiting so 
the only way it fails there is if there is a driver bug.

Re: [PATCH v3 2/3] target: fix NULL pointer dereference

[Mike Christie]

On 6/2/20 6:01 PM, Sudhakar Panneerselvam wrote:
>>> Maybe move this to after target_get_sess_cmd().
>>
>> I moved it before target_get_sess_cmd() because if target_get_sess_cmd() fails
>> then we have NULL pointer dereference issue again. For instance, the sequence
>>    vhost_scsi_submission_work
>>       target_submit_cmd_map_sgls
>>         target_get_sess_cmd() -- Suppose this fails
>>       transport_send_check_condition_and_sense
>>          trace_target_cmd_complete -- NULL ptr derefence.
>>
>> Still thinking how to address both these issues together.
> 
> Also, noticed that not all callers of target_get_sess_cmd() check for return value.( iscsit_setup_scsi_cmd() and iscsit_handle_task_mgt_cmd()). Could this cause problems?
> 

I think it's ok. iscsi doesn't use target_sess_cmd_list_set_waiting so 
the only way it fails there is if there is a driver bug.

Re: [PATCH v3 2/3] target: fix NULL pointer dereference

[Mike Christie]

On 6/2/20 6:26 PM, Mike Christie wrote:
>>
> 
> Maybe you need a new trace call for the case where we can't fully 
> initialize the cmd. It could be used for cases like where 
> transport_generic_new_cmd is used directly but fails, the 
> transport_handle_queue_full case, and your case where we fail during the 
> initial setup.

Ignore the transport_handle_queue_full case. I thought we had drivers 
using it when they initially read commands in, but that's not the case 
so the cmd is always setup in that function.

Re: [PATCH v3 2/3] target: fix NULL pointer dereference

[Mike Christie]

On 6/2/20 6:26 PM, Mike Christie wrote:
>>
> 
> Maybe you need a new trace call for the case where we can't fully 
> initialize the cmd. It could be used for cases like where 
> transport_generic_new_cmd is used directly but fails, the 
> transport_handle_queue_full case, and your case where we fail during the 
> initial setup.

Ignore the transport_handle_queue_full case. I thought we had drivers 
using it when they initially read commands in, but that's not the case 
so the cmd is always setup in that function.