[stgt] [PATCH] Fix PR OUT with REGISTER AND MOVE service action

[FUJITA Tomonori]

Sorry for the delay

On Sun, 01 Apr 2012 21:24:16 +0200
Arne Redlich <arne.redlich at googlemail.com> 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 at googlemail.com>
> ---
> 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 at vger.kernel.org
> 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html