[Scst-devel] [PATCH 3/5] scst: fix parsing of CDB control byte

[Tony B. <to...@cy...>]

SCST_GET_CDB_LEN() returns 0 for vendor-specific opcodes such as:

0xD1 READ DYN RUNTIME ATT
0xD2 WRITE DYN RUNTIME ATTR
0xE7 INIT ELEMENT STATUS WRANGE

This causes scst_set_cmd_from_cdb_info() to check cdb[-1] for the
control byte, causing an out-of-bounds array read.

- Move the parsing of the control byte after get_cdb_info() since that
  may set the CDB length to a known value.

- If the CDB length is still unknown, then assume the control byte is
  0 without accessing the CDB.

- Check for variable-length CDBs in scst_set_cmd_from_cdb_info() rather
  than using the wrong control byte and then overriding it in
  get_cdb_info_var_len().  This is necessary because the override would
  no longer work after the change above.

Also, the following code doesn't work:

    #define CONTROL_BYTE_NACA_BIT 0x04
    unsigned int cmd_naca:1;
    cmd_naca = (control & CONTROL_BYTE_NACA_BIT);

The result will always be 0.  Use this instead:

    cmd_naca = !!(control & CONTROL_BYTE_NACA_BIT);

(cmd_linked happened to work because CONTROL_BYTE_LINK_BIT is 0x01, but
apply the same fix there also for consistency).

Signed-off-by: Tony Battersby <to...@cy...>
---
 scst/src/scst_lib.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c
index 7eda3aa1e..6b480301a 100644
--- a/scst/src/scst_lib.c
+++ b/scst/src/scst_lib.c
@@ -11941,8 +11941,6 @@ static int scst_set_cmd_from_cdb_info(struct scst_cmd *cmd,
 	int res;
 
 	cmd->cdb_len = SCST_GET_CDB_LEN(cmd->cdb[0]);
-	cmd->cmd_naca = (cmd->cdb[cmd->cdb_len - 1] & CONTROL_BYTE_NACA_BIT);
-	cmd->cmd_linked = (cmd->cdb[cmd->cdb_len - 1] & CONTROL_BYTE_LINK_BIT);
 	cmd->op_name = ptr->info_op_name;
 	cmd->data_direction = ptr->info_data_direction;
 	cmd->op_flags = ptr->info_op_flags | SCST_INFO_VALID;
@@ -11952,6 +11950,15 @@ static int scst_set_cmd_from_cdb_info(struct scst_cmd *cmd,
 	cmd->len_len = ptr->info_len_len;
 	cmd->log2_max_buf_len = ptr->log2_max_buf_len;
 	res = (*ptr->get_cdb_info)(cmd, ptr);
+
+	/* get_cdb_info() may override cdb_len, so do this after */
+	if (likely(cmd->cdb_len != 0)) {
+		unsigned int control = (cmd->cdb[0] == 0x7F) ? cmd->cdb[1] :
+			cmd->cdb[cmd->cdb_len - 1];
+		cmd->cmd_naca = !!(control & CONTROL_BYTE_NACA_BIT);
+		cmd->cmd_linked = !!(control & CONTROL_BYTE_LINK_BIT);
+	}
+
 	if (!cmd->log2_max_buf_len ||
 	    cmd->bufflen <= (1U << cmd->log2_max_buf_len))
 		return res;
@@ -12047,9 +12054,6 @@ static int get_cdb_info_var_len(struct scst_cmd *cmd,
 
 	res = scst_set_cmd_from_cdb_info(cmd, ptr);
 
-	cmd->cmd_naca = (cmd->cdb[1] & CONTROL_BYTE_NACA_BIT);
-	cmd->cmd_linked = (cmd->cdb[1] & CONTROL_BYTE_LINK_BIT);
-
 out:
 	TRACE_EXIT_RES(res);
 	return res;
-- 
2.43.0