spc_pr_register_and_move() suffers from a number of problems:
* the offsets for the TRANSPORTID PARAMETER DATA LENGTH (addlen) and the
TransportID are wrong
* invalid input data (TransportID length restrictions imposed by the spec) is not
sufficiently checked
* TransportIDs are compared using strncmp() which does not work with transports
other than iSCSI
.
Signed-off-by: Arne Redlich <arne.redlich@xxxxxxxxxxxxxx>
---
Tomo, list,
This one is completely and utterly untested besides having the compiler's blessing, so please
review and test carefully.
It addresses the aforementioned issues within the framework of the current code, but I think in
the long run this (and related code) would greatly benefit from using types instead of magic constants
for the handling of SCSI commands.
Cheers,
Arne
usr/spc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
1 files changed, 44 insertions(+), 6 deletions(-)
diff --git a/usr/spc.c b/usr/spc.c
index 93aa062..2a71508 100644
--- a/usr/spc.c
+++ b/usr/spc.c
@@ -19,6 +19,7 @@
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
* 02110-1301 USA
*/
+#include <assert.h>
#include <errno.h>
#include <inttypes.h>
#include <stdio.h>
@@ -1360,16 +1361,17 @@ static int spc_pr_register_and_move(int host_no, struct scsi_cmd *cmd)
char *buf;
uint8_t unreg;
uint64_t res_key, sa_res_key;
- uint32_t addlen, idlen;
+ uint32_t addlen, idlen, outlen;
struct registration *reg, *dst;
- uint16_t len = 24;
+ const uint16_t len = 24; /* otherwise we can't get at addlen */
int (*id)(int, uint64_t, char *, int);
char tpid[300]; /* large enough? */
if (get_unaligned_be16(cmd->scb + 7) < len)
goto sense;
- if (scsi_get_out_length(cmd) < len)
+ outlen = scsi_get_out_length(cmd);
+ if (outlen < len)
goto sense;
buf = scsi_get_out_buffer(cmd);
@@ -1382,7 +1384,24 @@ static int spc_pr_register_and_move(int host_no, struct scsi_cmd *cmd)
res_key = get_unaligned_be64(buf);
sa_res_key = get_unaligned_be64(buf + 8);
- addlen = get_unaligned_be32(buf + 24);
+ addlen = get_unaligned_be32(buf + 20);
+
+ /*
+ * SPC-4r02, 6.12.4:
+ * "The TRANSPORTID DESCRIPTOR LENGTH field specifies the number of bytes of
+ * the TransportID that follows, shall be a minimum of 24 bytes, and shall
+ * be a multiple of 4. [...]
+ * The command shall be terminated with CHECK CONDITION status [...]
+ * a) If the value in the parameter list length field in the CDB does
+ * not include all of the parameter list bytes specified by the
+ * TRANSPORTID PARAMETER DATA LENGTH field; or
+ * b) If the value in the TRANSPORTID PARAMETER DATA LENGTH field
+ * results in the truncation of a TransportID."
+ *
+ * See below for (b).
+ */
+ if (addlen < 24 || addlen % 4 != 0 || outlen - len < addlen)
+ goto sense;
reg = lookup_registration_by_nexus(cmd->dev, cmd->it_nexus);
if (!reg) {
@@ -1413,9 +1432,28 @@ found:
if (id) {
memset(tpid, 0, sizeof(tpid));
idlen = id(cmd->dev->tgt->tid, dst->nexus_id, tpid, sizeof(tpid));
- if (addlen) {
- if (strncmp(tpid, buf + 28, strlen(tpid)))
+ if (idlen <= 0)
+ goto sense;
+
+ assert(idlen % 4 == 0);
+ assert(idlen >= 24);
+
+ if (addlen < idlen)
+ goto sense;
+ else {
+ size_t i;
+
+ if (memcmp(tpid, buf + len, idlen))
goto sense;
+ /*
+ * the transport ID could be padded with an arbitrary
+ * (as long as it's padded to a 4 byte boundary, but we
+ * checked for that already) amount of \0's, or is that
+ * ruled out by SPC-x?
+ */
+ for (i = idlen; i < addlen - idlen; i++)
+ if (buf[len + i] != '\0')
+ goto sense;
}
}
--
1.7.9.1
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
