From be9b8033343974e219f4e5778167d6532075a668 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sat, 26 Sep 2015 20:51:49 -0700 Subject: [PATCH] test-tool: Avoid that using receive_copy_results() triggers a use-after-free Move the scsi_free_scsi_task() call from receive_copy_results() to the callers of this function to avoid that accessing the unmarshalled data triggers a use-after-free. Signed-off-by: Bart Van Assche --- test-tool/iscsi-support.c | 26 ++++++++++--------- test-tool/iscsi-support.h | 5 +++- test-tool/test_extendedcopy_descr_limits.c | 12 ++++++--- .../test_receive_copy_results_copy_status.c | 14 +++++++--- .../test_receive_copy_results_op_params.c | 8 ++++-- 5 files changed, 42 insertions(+), 23 deletions(-) diff --git a/test-tool/iscsi-support.c b/test-tool/iscsi-support.c index 3c7a89d..5c485a0 100644 --- a/test-tool/iscsi-support.c +++ b/test-tool/iscsi-support.c @@ -2956,22 +2956,27 @@ void populate_param_header(unsigned char *buf, int list_id, int str, int list_id buf[15] = inline_data_len & 0xFF; } -int receive_copy_results(struct scsi_device *sdev, enum scsi_copy_results_sa sa, int list_id, void **datap, int status, enum scsi_sense_key key, int *ascq, int num_ascq) +int receive_copy_results(struct scsi_task **task, struct scsi_device *sdev, + enum scsi_copy_results_sa sa, int list_id, + void **datap, int status, enum scsi_sense_key key, + int *ascq, int num_ascq) { int ret; - struct scsi_task *task; logging(LOG_VERBOSE, "Send RECEIVE COPY RESULTS"); - task = scsi_cdb_receive_copy_results(sa, list_id, 1024); + *task = scsi_cdb_receive_copy_results(sa, list_id, 1024); assert(task != NULL); - task = send_scsi_command(sdev, task, NULL); + *task = send_scsi_command(sdev, *task, NULL); - ret = check_result("RECEIVECOPYRESULT", sdev, task, status, key, ascq, num_ascq); + ret = check_result("RECEIVECOPYRESULT", sdev, *task, status, key, ascq, + num_ascq); + if (ret < 0) + return ret; - if (task->status == SCSI_STATUS_GOOD && datap != NULL) { - *datap = scsi_datain_unmarshall(task); + if ((*task)->status == SCSI_STATUS_GOOD && datap != NULL) { + *datap = scsi_datain_unmarshall(*task); if (*datap == NULL) { logging(LOG_NORMAL, "[FAIL] failed to unmarshall RECEIVE COPY RESULTS data. %s", @@ -2980,11 +2985,8 @@ int receive_copy_results(struct scsi_device *sdev, enum scsi_copy_results_sa sa, } } - ret = check_result("RECEIVECOPYRESULT", sdev, task, status, key, ascq, num_ascq); - if (task) - scsi_free_scsi_task(task); - - return ret; + return check_result("RECEIVECOPYRESULT", sdev, *task, status, key, ascq, + num_ascq); } #define TEST_ISCSI_TUR_MAX_RETRIES 5 diff --git a/test-tool/iscsi-support.h b/test-tool/iscsi-support.h index d00ba0c..d406e40 100644 --- a/test-tool/iscsi-support.h +++ b/test-tool/iscsi-support.h @@ -337,6 +337,9 @@ int populate_tgt_desc(unsigned char *desc, enum ec_descr_type_code desc_type, in int populate_seg_desc_hdr(unsigned char *hdr, enum ec_descr_type_code desc_type, int dc, int cat, int src_index, int dst_index); int populate_seg_desc_b2b(unsigned char *desc, int dc, int cat, int src_index, int dst_index, int num_blks, uint64_t src_lba, uint64_t dst_lba); void populate_param_header(unsigned char *buf, int list_id, int str, int list_id_usage, int prio, int tgt_desc_len, int seg_desc_len, int inline_data_len); -int receive_copy_results(struct scsi_device *sdev, enum scsi_copy_results_sa sa, int list_id, void **datap, int status, enum scsi_sense_key key, int *ascq, int num_ascq); +int receive_copy_results(struct scsi_task **task, struct scsi_device *sdev, + enum scsi_copy_results_sa sa, int list_id, + void **datap, int status, enum scsi_sense_key key, + int *ascq, int num_ascq); int test_iscsi_tur_until_good(struct scsi_device *iscsi_sd, int *num_uas); #endif /* _ISCSI_SUPPORT_H_ */ diff --git a/test-tool/test_extendedcopy_descr_limits.c b/test-tool/test_extendedcopy_descr_limits.c index 24e9cbf..f3f277c 100644 --- a/test-tool/test_extendedcopy_descr_limits.c +++ b/test-tool/test_extendedcopy_descr_limits.c @@ -52,6 +52,7 @@ void test_extendedcopy_descr_limits(void) { int ret; + struct scsi_task *edl_task; struct iscsi_data data; unsigned char *xcopybuf; struct scsi_copy_results_op_params *opp; @@ -64,12 +65,12 @@ test_extendedcopy_descr_limits(void) CHECK_FOR_DATALOSS; logging(LOG_VERBOSE, "Issue RECEIVE COPY RESULTS (OPERATING PARAMS)"); - ret = receive_copy_results(sd, SCSI_COPY_RESULTS_OP_PARAMS, 0, - (void **)&opp, EXPECT_STATUS_GOOD); + ret = receive_copy_results(&edl_task, sd, SCSI_COPY_RESULTS_OP_PARAMS, 0, + (void **)&opp, EXPECT_STATUS_GOOD); if (ret < 0) { CU_PASS("[SKIPPED] Target does not support " "RECEIVE_COPY_RESULTS. Skipping test"); - return; + goto out; } CU_ASSERT_EQUAL(ret, 0); @@ -95,7 +96,7 @@ test_extendedcopy_descr_limits(void) if (ret == -2) { CU_PASS("[SKIPPED] Target does not support " "EXTENDED_COPY. Skipping test"); - return; + goto out; } CU_ASSERT_EQUAL(ret, 0); @@ -123,4 +124,7 @@ test_extendedcopy_descr_limits(void) ret = extendedcopy(sd, &data, EXPECT_PARAM_LIST_LEN_ERR); CU_ASSERT_EQUAL(ret, 0); } + +out: + scsi_free_scsi_task(edl_task); } diff --git a/test-tool/test_receive_copy_results_copy_status.c b/test-tool/test_receive_copy_results_copy_status.c index 05404b9..2ad296a 100644 --- a/test-tool/test_receive_copy_results_copy_status.c +++ b/test-tool/test_receive_copy_results_copy_status.c @@ -30,6 +30,7 @@ void test_receive_copy_results_copy_status(void) { int ret; + struct scsi_task *cs_task; struct scsi_copy_results_copy_status *csp; int tgt_desc_len = 0, seg_desc_len = 0; int offset = XCOPY_DESC_OFFSET, list_id = 1; @@ -40,12 +41,14 @@ test_receive_copy_results_copy_status(void) logging(LOG_VERBOSE, "Test RECEIVE COPY RESULTS, COPY STATUS"); logging(LOG_VERBOSE, "No copy in progress"); - ret = receive_copy_results(sd, SCSI_COPY_RESULTS_COPY_STATUS, + ret = receive_copy_results(&cs_task, sd, SCSI_COPY_RESULTS_COPY_STATUS, list_id, NULL, EXPECT_INVALID_FIELD_IN_CDB); + scsi_free_scsi_task(cs_task); + cs_task = NULL; if (ret == -2) { CU_PASS("[SKIPPED] Target does not support " "RECEIVE_COPY_STATUS. Skipping test"); - return; + goto out; } CU_ASSERT_EQUAL(ret, 0); @@ -77,13 +80,16 @@ test_receive_copy_results_copy_status(void) if (ret == -2) { CU_PASS("[SKIPPED] Target does not support " "EXTENDED_COPY. Skipping test"); - return; + goto out; } CU_ASSERT_EQUAL(ret, 0); logging(LOG_VERBOSE, "Copy Status for the above Extended Copy command"); - ret = receive_copy_results(sd, SCSI_COPY_RESULTS_COPY_STATUS, + ret = receive_copy_results(&cs_task, sd, SCSI_COPY_RESULTS_COPY_STATUS, list_id, (void **)&csp, EXPECT_STATUS_GOOD); CU_ASSERT_EQUAL(ret, 0); + +out: + scsi_free_scsi_task(cs_task); } diff --git a/test-tool/test_receive_copy_results_op_params.c b/test-tool/test_receive_copy_results_op_params.c index ff9b6f7..44fea11 100644 --- a/test-tool/test_receive_copy_results_op_params.c +++ b/test-tool/test_receive_copy_results_op_params.c @@ -30,16 +30,17 @@ void test_receive_copy_results_op_params(void) { int ret; + struct scsi_task *op_task; struct scsi_copy_results_op_params *opp; logging(LOG_VERBOSE, LOG_BLANK_LINE); logging(LOG_VERBOSE, "Test RECEIVE COPY RESULTS, OPERATING PARAMS"); - ret = receive_copy_results(sd, SCSI_COPY_RESULTS_OP_PARAMS, 0, + ret = receive_copy_results(&op_task, sd, SCSI_COPY_RESULTS_OP_PARAMS, 0, (void **)&opp, EXPECT_STATUS_GOOD); if (ret == -2) { CU_PASS("[SKIPPED] RECEIVE_COPY_RESULT is not implemented."); - return; + goto out; } CU_ASSERT_EQUAL(ret, 0); @@ -47,4 +48,7 @@ test_receive_copy_results_op_params(void) "max_target_desc=%d, max_seg_desc=%d", opp->max_target_desc_count, opp->max_segment_desc_count); + +out: + scsi_free_scsi_task(op_task); }