From 534f63f47bf94665f5230346e5a453a3a7d01f96 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 9 Aug 2020 00:37:56 +0000 Subject: [PATCH 1/5] scst_pres: Use {get,put}_unaligned_be64() instead of {get,put}_unaligned_be() This patch does not change any functionality. git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@9114 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_pres.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/scst/src/scst_pres.c b/scst/src/scst_pres.c index 99c9e44db..0c1dc587e 100644 --- a/scst/src/scst_pres.c +++ b/scst/src/scst_pres.c @@ -795,7 +795,7 @@ static int scst_pr_do_load_device_file(struct scst_device *dev, tid = &buf[pos]; pos += scst_tid_size(tid); - key = get_unaligned((__be64 *)&buf[pos]); + key = get_unaligned_be64(&buf[pos]); pos += sizeof(key); rel_tgt_id = get_unaligned((uint16_t *)&buf[pos]); @@ -1295,7 +1295,7 @@ static int scst_pr_register_with_spec_i_pt(struct scst_cmd *cmd, scst_assert_pr_mutex_held(cmd->dev); - action_key = get_unaligned((__be64 *)&buffer[8]); + action_key = get_unaligned_be64(&buffer[8]); ext_size = get_unaligned_be32(&buffer[24]); if ((ext_size + 28) > buffer_size) { @@ -1506,7 +1506,7 @@ static int scst_pr_register_on_tgt_id(struct scst_cmd *cmd, __be64 action_key; struct scst_dev_registrant *reg; - action_key = get_unaligned((__be64 *)&buffer[8]); + action_key = get_unaligned_be64(&buffer[8]); reg = scst_pr_add_registrant(cmd->dev, cmd->sess->transport_id, rel_tgt_id, action_key, false); @@ -1681,8 +1681,8 @@ void scst_pr_register(struct scst_cmd *cmd, uint8_t *buffer, int buffer_size) aptpl = buffer[20] & 0x01; spec_i_pt = (buffer[20] >> 3) & 0x01; all_tg_pt = (buffer[20] >> 2) & 0x01; - key = get_unaligned((__be64 *)&buffer[0]); - action_key = get_unaligned((__be64 *)&buffer[8]); + key = get_unaligned_be64(&buffer[0]); + action_key = get_unaligned_be64(&buffer[8]); if (spec_i_pt == 0 && buffer_size != 24) { TRACE_PR("Invalid buffer size %d", buffer_size); @@ -1768,7 +1768,7 @@ void scst_pr_register_and_ignore(struct scst_cmd *cmd, uint8_t *buffer, aptpl = buffer[20] & 0x01; all_tg_pt = (buffer[20] >> 2) & 0x01; - action_key = get_unaligned((__be64 *)&buffer[8]); + action_key = get_unaligned_be64(&buffer[8]); if (buffer_size != 24) { TRACE_PR("Invalid buffer size %d", buffer_size); @@ -1839,8 +1839,8 @@ void scst_pr_register_and_move(struct scst_cmd *cmd, uint8_t *buffer, scst_assert_pr_mutex_held(cmd->dev); aptpl = buffer[17] & 0x01; - key = get_unaligned((__be64 *)&buffer[0]); - action_key = get_unaligned((__be64 *)&buffer[8]); + key = get_unaligned_be64(&buffer[0]); + action_key = get_unaligned_be64(&buffer[8]); unreg = (buffer[17] >> 1) & 0x01; tid_buffer_size = get_unaligned_be32(&buffer[20]); @@ -1975,7 +1975,7 @@ void scst_pr_reserve(struct scst_cmd *cmd, uint8_t *buffer, int buffer_size) scst_assert_pr_mutex_held(dev); - key = get_unaligned((__be64 *)&buffer[0]); + key = get_unaligned_be64(&buffer[0]); scope = cmd->cdb[2] >> 4; type = cmd->cdb[2] & 0x0f; @@ -2064,7 +2064,7 @@ void scst_pr_release(struct scst_cmd *cmd, uint8_t *buffer, int buffer_size) scst_assert_pr_mutex_held(dev); - key = get_unaligned((__be64 *)&buffer[0]); + key = get_unaligned_be64(&buffer[0]); scope = cmd->cdb[2] >> 4; type = cmd->cdb[2] & 0x0f; @@ -2141,7 +2141,7 @@ void scst_pr_clear(struct scst_cmd *cmd, uint8_t *buffer, int buffer_size) scst_assert_pr_mutex_held(dev); - key = get_unaligned((__be64 *)&buffer[0]); + key = get_unaligned_be64(&buffer[0]); if (buffer_size != 24) { TRACE_PR("Invalid buffer size %d", buffer_size); @@ -2202,8 +2202,8 @@ static void scst_pr_do_preempt(struct scst_cmd *cmd, uint8_t *buffer, goto out; } - key = get_unaligned((__be64 *)&buffer[0]); - action_key = get_unaligned((__be64 *)&buffer[8]); + key = get_unaligned_be64(&buffer[0]); + action_key = get_unaligned_be64(&buffer[8]); scope = cmd->cdb[2] >> 4; type = cmd->cdb[2] & 0x0f; @@ -2593,8 +2593,7 @@ void scst_pr_read_keys(struct scst_cmd *cmd, uint8_t *buffer, int buffer_size) WARN_ON(reg->key == 0); - put_unaligned(reg->key, - (__be64 *)&buffer[offset]); + put_unaligned_be64(reg->key, &buffer[offset]); offset += 8; } @@ -2651,7 +2650,7 @@ void scst_pr_read_reservation(struct scst_cmd *cmd, uint8_t *buffer, b[6] = 0; b[7] = 0x10; - put_unaligned(key, (__be64 *)&b[8]); + put_unaligned_be64(key, &b[8]); b[21] = dev->pr_scope << 4 | dev->pr_type; size = 24; @@ -2742,7 +2741,7 @@ void scst_pr_read_full_status(struct scst_cmd *cmd, uint8_t *buffer, if (size_max - size > rec_len) { memset(&buffer[offset], 0, rec_len); - put_unaligned(reg->key, (__be64 *)(&buffer[offset])); + put_unaligned_be64(reg->key, &buffer[offset]); if (dev->pr_is_set && scst_pr_is_holder(dev, reg)) { buffer[offset + 12] = 1; From 6138af9193c457a60b9c9c7bfb1289aca268334a Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 9 Aug 2020 00:40:40 +0000 Subject: [PATCH 2/5] scst_pres: Fix a potential buffer overflow in tid_secure() This was detected by Coverity. git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@9115 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_dlm.c | 4 ++-- scst/src/scst_pres.c | 50 +++++++++++++++++++++++--------------------- scst/src/scst_pres.h | 2 +- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/scst/src/scst_dlm.c b/scst/src/scst_dlm.c index 5e28e54c2..652306189 100644 --- a/scst/src/scst_dlm.c +++ b/scst/src/scst_dlm.c @@ -426,9 +426,9 @@ static void scst_copy_to_dlm(struct scst_device *dev, dlm_lockspace_t *ls) struct pr_lvb *lvb = (void *)pr_dlm->lvb; struct pr_reg_lvb *reg_lvb; struct scst_dev_registrant *reg; - int i, tid_size; + int i; char reg_name[32]; - uint32_t nr_registrants; + uint32_t nr_registrants, tid_size; lockdep_assert_held(&pr_dlm->ls_mutex); diff --git a/scst/src/scst_pres.c b/scst/src/scst_pres.c index 0c1dc587e..d4517bcf7 100644 --- a/scst/src/scst_pres.c +++ b/scst/src/scst_pres.c @@ -84,7 +84,7 @@ static inline void scst_assert_pr_mutex_held(struct scst_device *dev) } #endif -int scst_tid_size(const uint8_t *tid) +uint32_t scst_tid_size(const uint8_t *tid) { sBUG_ON(tid == NULL); @@ -94,16 +94,18 @@ int scst_tid_size(const uint8_t *tid) return TID_COMMON_SIZE; } -/* Secures tid by setting 0 in the last byte of NULL-terminated tid's */ -static inline void tid_secure(uint8_t *tid) +/* Secures transport ID by setting 0 in the last byte of ASCII transport IDs */ +static bool tid_secure(uint8_t *tid, unsigned int buf_size) { if ((tid[0] & 0x0f) == SCSI_TRANSPORTID_PROTOCOLID_ISCSI) { - int size = scst_tid_size(tid); + const uint32_t size = scst_tid_size(tid); + if (size > buf_size) + return false; tid[size - 1] = '\0'; } - return; + return true; } /* Returns false if tid's are not equal, true otherwise */ @@ -122,8 +124,8 @@ bool tid_equal(const uint8_t *tid_a, const uint8_t *tid_b) if ((tid_a[0] & 0x0f) == SCSI_TRANSPORTID_PROTOCOLID_ISCSI) { const uint8_t tid_a_fmt = tid_a[0] & 0xc0; const uint8_t tid_b_fmt = tid_b[0] & 0xc0; - int tid_a_len, tid_a_max = scst_tid_size(tid_a) - 4; - int tid_b_len, tid_b_max = scst_tid_size(tid_b) - 4; + uint32_t tid_a_len, tid_a_max = scst_tid_size(tid_a) - 4; + uint32_t tid_b_len, tid_b_max = scst_tid_size(tid_b) - 4; int i; tid_a += 4; @@ -140,7 +142,6 @@ bool tid_equal(const uint8_t *tid_a, const uint8_t *tid_b) tid_a_len = p - tid_a; sBUG_ON(tid_a_len > tid_a_max); - sBUG_ON(tid_a_len < 0); } else tid_a_len = strnlen(tid_a, tid_a_max); } else @@ -157,7 +158,6 @@ bool tid_equal(const uint8_t *tid_a, const uint8_t *tid_b) tid_b_len = p - tid_b; sBUG_ON(tid_b_len > tid_b_max); - sBUG_ON(tid_b_len < 0); } else tid_b_len = strnlen(tid_b, tid_b_max); } else @@ -953,7 +953,7 @@ void scst_pr_sync_device_file(struct scst_device *dev) list_for_each_entry(reg, &dev->dev_registrants_list, dev_registrants_list_entry) { uint8_t is_holder = 0; - int size; + uint32_t size; is_holder = (dev->pr_holder == reg); @@ -1291,7 +1291,6 @@ static int scst_pr_register_with_spec_i_pt(struct scst_cmd *cmd, __be64 action_key; struct scst_device *dev = cmd->dev; struct scst_dev_registrant *reg; - uint8_t *transport_id; scst_assert_pr_mutex_held(cmd->dev); @@ -1309,25 +1308,25 @@ static int scst_pr_register_with_spec_i_pt(struct scst_cmd *cmd, offset = 0; while (offset < ext_size) { - transport_id = &buffer[28 + offset]; + uint8_t *const transport_id = &buffer[28 + offset]; + const uint32_t tid_size = scst_tid_size(transport_id); - if ((offset + scst_tid_size(transport_id)) > ext_size) { + if (offset + tid_size > ext_size) { TRACE_PR("Invalid transport_id size %d (max %d)", - scst_tid_size(transport_id), ext_size - offset); + tid_size, ext_size - offset); scst_set_invalid_field_in_parm_list(cmd, 24, 0); res = -EINVAL; goto out; } - tid_secure(transport_id); - offset += scst_tid_size(transport_id); + WARN_ON_ONCE(!tid_secure(transport_id, tid_size)); + offset += tid_size; } offset = 0; while (offset < ext_size) { + uint8_t *transport_id = &buffer[28 + offset]; struct scst_tgt_dev *t; - transport_id = &buffer[28 + offset]; - TRACE_PR("rel_tgt_id %d, transport_id %s", rel_tgt_id, debug_transport_id_to_initiator_name(transport_id)); @@ -1908,7 +1907,13 @@ void scst_pr_register_and_move(struct scst_cmd *cmd, uint8_t *buffer, goto out; } - tid_secure(transport_id_move); + if (!tid_secure(transport_id_move, buffer_size - 24)) { + TRACE_PR("Transport ID length %d exceeds buffer size %d", + scst_tid_size(transport_id_move), buffer_size - 24); + scst_set_cmd_error(cmd, + SCST_LOAD_SENSE(scst_sense_invalid_field_in_parm_list)); + goto out; + } if (dev->pr_type == TYPE_WRITE_EXCLUSIVE_ALL_REG || dev->pr_type == TYPE_EXCLUSIVE_ACCESS_ALL_REG) { @@ -2732,11 +2737,8 @@ void scst_pr_read_full_status(struct scst_cmd *cmd, uint8_t *buffer, list_for_each_entry(reg, &dev->dev_registrants_list, dev_registrants_list_entry) { - int ts; - int rec_len; - - ts = scst_tid_size(reg->transport_id); - rec_len = 24 + ts; + const uint32_t ts = scst_tid_size(reg->transport_id); + const uint32_t rec_len = 24 + ts; if (size_max - size > rec_len) { memset(&buffer[offset], 0, rec_len); diff --git a/scst/src/scst_pres.h b/scst/src/scst_pres.h index 722fe4e15..1ab2f6ef1 100644 --- a/scst/src/scst_pres.h +++ b/scst/src/scst_pres.h @@ -130,7 +130,7 @@ void scst_pr_report_caps(struct scst_cmd *cmd, uint8_t *buffer, int buffer_size) void scst_pr_read_full_status(struct scst_cmd *cmd, uint8_t *buffer, int buffer_size); -int scst_tid_size(const uint8_t *tid); +uint32_t scst_tid_size(const uint8_t *tid); bool tid_equal(const uint8_t *tid_a, const uint8_t *tid_b); struct scst_dev_registrant *scst_pr_find_reg(struct scst_device *dev, From 31dbbf327476723ce0794590878ce06e30dc9fa9 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 9 Aug 2020 00:41:14 +0000 Subject: [PATCH 3/5] usr/fileio: Suppress a Coverity complaint about 'page_size' Suppress the following Coverity complaint: CID 361202: Error handling issues (NEGATIVE_RETURNS) "page_size" is passed to a parameter that cannot be negative. git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@9116 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- usr/fileio/fileio.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/usr/fileio/fileio.c b/usr/fileio/fileio.c index a6a0f904d..64f9247c5 100644 --- a/usr/fileio/fileio.c +++ b/usr/fileio/fileio.c @@ -200,10 +200,12 @@ out: static void *align_alloc(size_t size) { - static uint32_t page_size; + static long page_size; - if (page_size == 0) + if (page_size == 0) { page_size = sysconf(_SC_PAGESIZE); + assert(page_size > 0); + } TRACE_MEM("Request to alloc %zdKB", size / 1024); return memalign(page_size, size); From 41b4e25644dac4cbf88fe38cc14bc8ba3778621d Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 9 Aug 2020 00:41:51 +0000 Subject: [PATCH 4/5] qla2x00t-32gbit: Always check the return value of qla24xx_get_isp_stats() This patch fixes the following Coverity warning: CID 361199 (#1 of 1): Unchecked return value (CHECKED_RETURN) 3. check_return: Calling qla24xx_get_isp_stats without checking return value (as is done elsewhere 4 out of 5 times). git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@9117 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- qla2x00t-32gbit/qla_attr.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/qla2x00t-32gbit/qla_attr.c b/qla2x00t-32gbit/qla_attr.c index 5d93ccc73..469f0ee50 100644 --- a/qla2x00t-32gbit/qla_attr.c +++ b/qla2x00t-32gbit/qla_attr.c @@ -2816,6 +2816,8 @@ qla2x00_reset_host_stats(struct Scsi_Host *shost) vha->qla_stats.jiffies_at_last_reset = get_jiffies_64(); if (IS_FWI2_CAPABLE(ha)) { + int rval; + stats = dma_alloc_coherent(&ha->pdev->dev, sizeof(*stats), &stats_dma, GFP_KERNEL); if (!stats) { @@ -2825,7 +2827,8 @@ qla2x00_reset_host_stats(struct Scsi_Host *shost) } /* reset firmware statistics */ - qla24xx_get_isp_stats(base_vha, stats, stats_dma, BIT_0); + rval = qla24xx_get_isp_stats(base_vha, stats, stats_dma, BIT_0); + WARN_ONCE(rval != QLA_SUCCESS, "rval = %d\n", rval); dma_free_coherent(&ha->pdev->dev, sizeof(*stats), stats, stats_dma); From 6616eb6cf795596d181d5a2f8dfe97534cf7c0ed Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 9 Aug 2020 00:42:32 +0000 Subject: [PATCH 5/5] scst_copy_mgr: Suppress a Coverity taint complaint Suppress the following (false positive) Coverity complaint: CID 361193: Insecure data handling (TAINTED_SCALAR) Passing tainted variable "sense_len" to a tainted sink. git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@9118 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_copy_mgr.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/scst/src/scst_copy_mgr.c b/scst/src/scst_copy_mgr.c index e8808a465..640885158 100644 --- a/scst/src/scst_copy_mgr.c +++ b/scst/src/scst_copy_mgr.c @@ -578,7 +578,7 @@ static void scst_cm_advance_seg_descr(struct scst_cmd *ec_cmd) static void scst_cm_prepare_final_sense(struct scst_cmd *ec_cmd) { struct scst_cm_ec_cmd_priv *priv = ec_cmd->cmd_data_descriptors; - uint8_t *fsense = NULL; + uint8_t add_sense_len, *fsense = NULL; int d_sense = scst_get_cmd_dev_d_sense(ec_cmd); bool copy_sense = false; int sense_to_copy = ec_cmd->sense_valid_len; @@ -665,7 +665,7 @@ static void scst_cm_prepare_final_sense(struct scst_cmd *ec_cmd) /* Descriptor format */ fsense[0] = 0x72; fsense[1] = COPY_ABORTED; - fsense[7] = 12; /* additional Sense Length */ + add_sense_len = 12; fsense[8] = 1; /* Command specific descriptor */ fsense[9] = 0xA; @@ -676,7 +676,7 @@ static void scst_cm_prepare_final_sense(struct scst_cmd *ec_cmd) /* Fixed format */ fsense[0] = 0x70; fsense[2] = COPY_ABORTED; - fsense[7] = 0x0a; /* additional Sense Length */ + add_sense_len = 0x0a; put_unaligned_be16(priv->cm_cur_seg_descr, &fsense[10]); @@ -691,15 +691,17 @@ static void scst_cm_prepare_final_sense(struct scst_cmd *ec_cmd) if (copy_sense) { TRACE_DBG("Copying %db of old sense", sense_to_copy); - fsense[7] += 1 + sense_to_copy; + add_sense_len += 1 + sense_to_copy; fsense[17] = ec_cmd->status; memcpy(&fsense[18], ec_cmd->sense, sense_to_copy); } - sense_len = fsense[7] + 8; + sense_len = add_sense_len + 8; TRACE_DBG("New sense len %d", sense_len); } + fsense[7] = add_sense_len; /* additional Sense Length */ + ec_cmd->status = SAM_STAT_CHECK_CONDITION; if (ec_cmd->sense != NULL) { memcpy(ec_cmd->sense, fsense, sense_len);