Sorry for the delay
On Sun, 01 Apr 2012 21:24:16 +0200
Arne Redlich <arne.redlich@xxxxxxxxxxxxxx> wrote:
> 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);
Have you confirmed that iscsi_transportid() returns a valid id?
> + 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
>
>
--
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
