* [PATCH v2 0/3] target: fix NULL pointer dereference
@ 2020-05-21 22:13 Sudhakar Panneerselvam
2020-05-21 22:13 ` [PATCH v2 1/3] target: factor out a new helper, target_cmd_init_cdb() Sudhakar Panneerselvam
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Sudhakar Panneerselvam @ 2020-05-21 22:13 UTC (permalink / raw)
To: martin.petersen, mchristi, target-devel, linux-scsi; +Cc: shirley.ma
The following set of commits address a NULL pointer dereference and some
refactoring around this issue.
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 | 27 +++++++++++++++++++++------
drivers/target/target_core_xcopy.c | 5 ++++-
include/target/target_core_fabric.h | 3 ++-
4 files changed, 40 insertions(+), 16 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2 1/3] target: factor out a new helper, target_cmd_init_cdb()
2020-05-21 22:13 [PATCH v2 0/3] target: fix NULL pointer dereference Sudhakar Panneerselvam
@ 2020-05-21 22:13 ` Sudhakar Panneerselvam
2020-05-21 22:13 ` [PATCH v2 2/3] target: fix NULL pointer dereference Sudhakar Panneerselvam
2020-05-21 22:13 ` [PATCH v2 3/3] target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb() Sudhakar Panneerselvam
2 siblings, 0 replies; 4+ messages in thread
From: Sudhakar Panneerselvam @ 2020-05-21 22:13 UTC (permalink / raw)
To: martin.petersen, mchristi, target-devel, linux-scsi; +Cc: shirley.ma
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2 2/3] target: fix NULL pointer dereference
2020-05-21 22:13 [PATCH v2 0/3] target: fix NULL pointer dereference Sudhakar Panneerselvam
2020-05-21 22:13 ` [PATCH v2 1/3] target: factor out a new helper, target_cmd_init_cdb() Sudhakar Panneerselvam
@ 2020-05-21 22:13 ` Sudhakar Panneerselvam
2020-05-21 22:13 ` [PATCH v2 3/3] target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb() Sudhakar Panneerselvam
2 siblings, 0 replies; 4+ messages in thread
From: Sudhakar Panneerselvam @ 2020-05-21 22:13 UTC (permalink / raw)
To: martin.petersen, mchristi, target-devel, linux-scsi; +Cc: shirley.ma
NULL pointer dereference happens when a SCSI command is received for a
non-existing LU and tracing is enabled. The following call sequences
lead to NULL pointer dereference:
1) iscsit_setup_scsi_cmd
transport_lookup_cmd_lun <-- lookup 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
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
before transport_lookup_cmd_lun() 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 | 11 +++++++++--
drivers/target/target_core_xcopy.c | 3 +++
3 files changed, 24 insertions(+), 9 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..67bfac1081c1 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1455,8 +1455,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",
@@ -1619,6 +1617,15 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
*/
if (flags & TARGET_SCF_BIDI_OP)
se_cmd->se_cmd_flags |= SCF_BIDI;
+
+ 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);
+ target_put_sess_cmd(se_cmd);
+ return 0;
+ }
+
/*
* Locate se_lun pointer and attach it to struct se_cmd
*/
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2 3/3] target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb()
2020-05-21 22:13 [PATCH v2 0/3] target: fix NULL pointer dereference Sudhakar Panneerselvam
2020-05-21 22:13 ` [PATCH v2 1/3] target: factor out a new helper, target_cmd_init_cdb() Sudhakar Panneerselvam
2020-05-21 22:13 ` [PATCH v2 2/3] target: fix NULL pointer dereference Sudhakar Panneerselvam
@ 2020-05-21 22:13 ` Sudhakar Panneerselvam
2 siblings, 0 replies; 4+ messages in thread
From: Sudhakar Panneerselvam @ 2020-05-21 22:13 UTC (permalink / raw)
To: martin.petersen, mchristi, target-devel, linux-scsi; +Cc: shirley.ma
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..d89983b2e8d6 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, hdr->cdb);
if (cmd->sense_reason)
goto attach_cmd;
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 67bfac1081c1..74ef89c1e7fb 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1450,7 +1450,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, unsigned char *cdb)
{
struct se_device *dev = cmd->se_dev;
sense_reason_t ret;
@@ -1472,7 +1472,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.
@@ -1636,7 +1636,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, cdb);
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..ed43086bf965 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, cdb))
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..65496b2a7ad6 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 *, unsigned char *);
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-21 22:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-21 22:13 [PATCH v2 0/3] target: fix NULL pointer dereference Sudhakar Panneerselvam
2020-05-21 22:13 ` [PATCH v2 1/3] target: factor out a new helper, target_cmd_init_cdb() Sudhakar Panneerselvam
2020-05-21 22:13 ` [PATCH v2 2/3] target: fix NULL pointer dereference Sudhakar Panneerselvam
2020-05-21 22:13 ` [PATCH v2 3/3] target: rename target_setup_cmd_from_cdb() to target_cmd_parse_cdb() Sudhakar Panneerselvam
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).