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

[Arne Redlich]

2012/4/1 Arne Redlich <arne.redlich at googlemail.com>:
> 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.

Any thoughts / feedback on this one?

Thanks,
Arne

> 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html