Merge pull request #267 from ddiss/prin_readkeys_truncate

fix handling of and test truncated PR-in read-keys responses

lib/scsi-lowlevel.c

@@ -962,7 +962,9 @@ scsi_receivecopyresults_datain_unmarshall(struct scsi_task *task)
 	}
 }
 
-
+#ifndef MIN /* instead of including all of iscsi-private.h */
+#define MIN(a, b) (((a) < (b)) ? (a) : (b))
+#endif
 static void *
 scsi_persistentreservein_datain_unmarshall(struct scsi_task *task)
 {
@@ -972,21 +974,44 @@ scsi_persistentreservein_datain_unmarshall(struct scsi_task *task)
 	int i;
 
 	switch (scsi_persistentreservein_sa(task)) {
-	case SCSI_PERSISTENT_RESERVE_READ_KEYS:
-		i = task_get_uint32(task, 4);
+	case SCSI_PERSISTENT_RESERVE_READ_KEYS: {
+		uint32_t cdb_keys_len;
+		uint32_t data_keys_len;
+		uint32_t keys_len;
 
-		rk = scsi_malloc(task, offsetof(struct scsi_persistent_reserve_in_read_keys, keys) + i);
+		if (task->datain.size < 8) {
+			return NULL;
+		}
+
+		/*
+		 * SPC5r17: 6.16.2 READ KEYS service action
+		 * The ADDITIONAL LENGTH field indicates the number of bytes in
+		 * the Reservation key list. The contents of the ADDITIONAL
+		 * LENGTH field are not altered based on the allocation length.
+		 */
+		cdb_keys_len = task_get_uint32(task, 4);
+		data_keys_len = task->datain.size - 8;
+		/*
+		 * Only process as many keys as permitted by the given
+		 * ADDITIONAL LENGTH and data-in size limits.
+		 */
+		keys_len = MIN(cdb_keys_len, data_keys_len);
+
+		rk = scsi_malloc(task,
+			offsetof(struct scsi_persistent_reserve_in_read_keys,
+				 keys) + keys_len);
 		if (rk == NULL) {
 			return NULL;
 		}
 		rk->prgeneration      = task_get_uint32(task, 0);
-		rk->additional_length = task_get_uint32(task, 4);
+		rk->additional_length = cdb_keys_len;
 
-		rk->num_keys = rk->additional_length / 8;
+		rk->num_keys = keys_len / 8;
 		for (i = 0; i < (int)rk->num_keys; i++) {
 			rk->keys[i] = task_get_uint64(task, 8 + i * 8);
 		}
 		return rk;
+	}
 	case SCSI_PERSISTENT_RESERVE_READ_RESERVATION: {
 		size_t	alloc_sz;
 

test-tool/Makefile.am

@@ -70,6 +70,7 @@ iscsi_test_cu_SOURCES = iscsi-test-cu.c \
 	test_preventallow_lun_reset.c \
 	test_preventallow_2_itnexuses.c \
 	test_prin_read_keys_simple.c \
+	test_prin_read_keys_truncate.c \
 	test_prin_serviceaction_range.c \
 	test_prin_report_caps.c \
 	test_prout_register_simple.c \

test-tool/iscsi-support.c

@@ -667,17 +667,18 @@ prin_task(struct scsi_device *sdev, int service_action,
 }
 
 int
-prin_read_keys(struct scsi_device *sdev, struct scsi_task **tp,
-        struct scsi_persistent_reserve_in_read_keys **rkp)
+prin_read_keys(struct scsi_device *sdev,
+               struct scsi_task **tp,
+               struct scsi_persistent_reserve_in_read_keys **rkp,
+               uint16_t allocation_len)
 {
-        const int buf_sz = 16384;
         struct scsi_persistent_reserve_in_read_keys *rk = NULL;
 
 
         logging(LOG_VERBOSE, "Send PRIN/READ_KEYS");
 
         *tp = scsi_cdb_persistent_reserve_in(SCSI_PERSISTENT_RESERVE_READ_KEYS,
-            buf_sz);
+                                             allocation_len);
         assert(*tp != NULL);
 
         *tp = send_scsi_command(sdev, *tp, NULL);

test-tool/iscsi-support.h

@@ -822,7 +822,8 @@ int all_zero(const unsigned char *buf, unsigned size);
 int prin_task(struct scsi_device *sdev, int service_action,
     int success_expected);
 int prin_read_keys(struct scsi_device *sdev, struct scsi_task **tp,
-    struct scsi_persistent_reserve_in_read_keys **rkp);
+                   struct scsi_persistent_reserve_in_read_keys **rkp,
+                   uint16_t allocation_len);
 int prout_register_and_ignore(struct scsi_device *sdev,
     unsigned long long key);
 int prout_register_key(struct scsi_device *sdev,

test-tool/iscsi-test-cu.c

@@ -158,6 +158,7 @@ static CU_TestInfo tests_preventallow[] = {
 
 static CU_TestInfo tests_prin_read_keys[] = {
         { (char *)"Simple", test_prin_read_keys_simple },
+        { (char *)"Truncate", test_prin_read_keys_truncate },
         CU_TEST_INFO_NULL
 };
 
@@ -1069,7 +1070,7 @@ static int clear_pr(struct scsi_device *sdev)
         struct scsi_persistent_reserve_in_read_keys *rk;
 
         res = 0;
-        if (prin_read_keys(sdev, &pr_task, &rk) != 0)
+        if (prin_read_keys(sdev, &pr_task, &rk, 16384) != 0)
                 goto out;
 
         res = -1;

test-tool/iscsi-test-cu.h

@@ -115,6 +115,7 @@ void test_preventallow_lun_reset(void);
 void test_preventallow_2_itnexuses(void);
 
 void test_prin_read_keys_simple(void);
+void test_prin_read_keys_truncate(void);
 void test_prin_serviceaction_range(void);
 void test_prin_report_caps_simple(void);
 

test-tool/test_prin_read_keys_simple.c

@@ -37,9 +37,8 @@ test_prin_read_keys_simple(void)
         logging(LOG_VERBOSE, LOG_BLANK_LINE);
         logging(LOG_VERBOSE, "Test Persistent Reserve IN READ_KEYS works.");
 
-        ret = prin_read_keys(sd, &task, NULL);
+        ret = prin_read_keys(sd, &task, NULL, 16384);
         if (ret == -2) {
-                logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE IN is not implemented.");
                 CU_PASS("PERSISTENT RESERVE IN is not implemented.");
                 return;
         }        

test-tool/test_prin_read_keys_truncate.c

@@ -0,0 +1,74 @@
+/* -*-  mode:c; tab-width:8; c-basic-offset:8; indent-tabs-mode:nil;  -*- */
+/*
+   Copyright (C) 2013 by Lee Duncan <[email protected]>
+   Copyright (C) 2018 David Disseldorp
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <stdio.h>
+#include <arpa/inet.h>
+
+#include <CUnit/CUnit.h>
+
+#include "iscsi.h"
+#include "scsi-lowlevel.h"
+#include "iscsi-support.h"
+#include "iscsi-test-cu.h"
+
+
+void
+test_prin_read_keys_truncate(void)
+{
+        const unsigned long long key = rand_key();
+        struct scsi_persistent_reserve_in_read_keys *rk = NULL;
+        int ret;
+
+        logging(LOG_VERBOSE, LOG_BLANK_LINE);
+        logging(LOG_VERBOSE, "Test Persistent Reserve IN READ_KEYS works when "
+                "truncated.");
+
+        ret = prout_register_and_ignore(sd, key);
+        if (ret == -2) {
+                CU_PASS("PERSISTENT RESERVE OUT is not implemented.");
+                return;
+        }
+        CU_ASSERT_EQUAL(ret, 0);
+
+        /*
+         * alloc_len=8 restricts the response buffer to only accepting the
+         * PRGENERATION and ADDITIONAL LENGTH fields.
+         */
+        ret = prin_read_keys(sd, &task, &rk, 8);
+        if (ret == -2) {
+                CU_PASS("PERSISTENT RESERVE IN is not implemented.");
+                prout_register_key(sd, 0, key);
+                return;
+        }
+        CU_ASSERT_EQUAL(ret, 0);
+
+        /*
+         * SPC5r17: 6.16.2 READ KEYS service action
+         * The ADDITIONAL LENGTH field indicates the number of bytes in
+         * the Reservation key list. The contents of the ADDITIONAL
+         * LENGTH field are not altered based on the allocation length.
+         */
+        CU_ASSERT_NOT_EQUAL(rk->additional_length, 0);
+        /* key array should have been truncated in the response */
+        CU_ASSERT_EQUAL(rk->num_keys, 0);
+
+        /* remove our key from the target */
+        ret = prout_register_key(sd, 0, key);
+        CU_ASSERT_EQUAL(0, ret);
+}

test-tool/test_prin_report_caps.c

@@ -63,7 +63,6 @@ test_prin_report_caps_simple(void)
         /* register our reservation key with the target */
         ret = prout_register_and_ignore(sd, key);
         if (ret == -2) {
-                logging(LOG_NORMAL, "[SKIPPED] PERSISTENT RESERVE OUT is not implemented.");
                 CU_PASS("PERSISTENT RESERVE OUT is not implemented.");
                 return;
         }

test-tool/test_prin_serviceaction_range.c

@@ -38,12 +38,11 @@ test_prin_serviceaction_range(void)
         logging(LOG_VERBOSE, "Test Persistent Reserve IN Serviceaction range.");
 
         /* verify PRIN/READ_KEYS works -- XXX redundant -- remove this? */
-        ret = prin_read_keys(sd, &task, NULL);
+        ret = prin_read_keys(sd, &task, NULL, 16384);
         if (ret == -2) {
-                logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE IN is not implemented.");
                 CU_PASS("PERSISTENT RESERVE IN is not implemented.");
                 return;
-        }        
+        }
         CU_ASSERT_EQUAL(ret, 0);
 
         /* verify that PRIN/SA={0,1,2,3} works ... */

test-tool/test_prout_clear_simple.c

@@ -43,13 +43,12 @@ test_prout_clear_simple(void)
         /* register our reservation key with the target */
         ret = prout_register_and_ignore(sd, key);
         if (ret == -2) {
-                logging(LOG_NORMAL, "[SKIPPED] PERSISTENT RESERVE OUT is not implemented.");
                 CU_PASS("PERSISTENT RESERVE OUT is not implemented.");
                 return;
-        }        
+        }
         CU_ASSERT_EQUAL(ret, 0);
 
-        ret = prin_read_keys(sd, &tsk, &rk);
+        ret = prin_read_keys(sd, &tsk, &rk, 16384);
         CU_ASSERT_EQUAL(ret, 0);
         CU_ASSERT_NOT_EQUAL(rk, NULL);
         if (!rk)
@@ -79,7 +78,7 @@ test_prout_clear_simple(void)
         ret = prin_verify_not_reserved(sd);
         CU_ASSERT_EQUAL(ret, 0);
 
-        ret = prin_read_keys(sd, &tsk, &rk);
+        ret = prin_read_keys(sd, &tsk, &rk, 16384);
         CU_ASSERT_EQUAL(ret, 0);
         CU_ASSERT_NOT_EQUAL(rk, NULL);
         if (!rk)

test-tool/test_prout_preempt.c

@@ -54,7 +54,6 @@ test_prout_preempt_rm_reg(void)
 
         ret = prout_register_and_ignore(sd, k1);
         if (ret == -2) {
-                logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE OUT is not implemented.");
                 CU_PASS("PERSISTENT RESERVE OUT is not implemented.");
                 return;
         }
@@ -79,7 +78,7 @@ test_prout_preempt_rm_reg(void)
         CU_ASSERT_EQUAL(ret, 0);
 
         /* confirm that k1 and k2 are registered */
-        ret = prin_read_keys(sd, &tsk, &rk);
+        ret = prin_read_keys(sd, &tsk, &rk, 16384);
         CU_ASSERT_EQUAL_FATAL(ret, 0);
 
         CU_ASSERT_EQUAL(rk->num_keys, 2);
@@ -100,7 +99,7 @@ test_prout_preempt_rm_reg(void)
         ret = test_iscsi_tur_until_good(sd2, &num_uas);
         CU_ASSERT_EQUAL(ret, 0);
 
-        ret = prin_read_keys(sd, &tsk, &rk);
+        ret = prin_read_keys(sd, &tsk, &rk, 16384);
         CU_ASSERT_EQUAL_FATAL(ret, 0);
 
         CU_ASSERT_EQUAL(rk->num_keys, 1);

test-tool/test_prout_register_simple.c

@@ -40,7 +40,6 @@ test_prout_register_simple(void)
         /* register our reservation key with the target */
         ret = prout_register_and_ignore(sd, key);
         if (ret == -2) {
-                logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE OUT is not implemented.");
                 CU_PASS("PERSISTENT RESERVE OUT is not implemented.");
                 return;
         }        

test-tool/test_prout_reserve_access.c

@@ -50,7 +50,6 @@ verify_persistent_reserve_access(struct scsi_device *sd1, struct scsi_device *sd
         /* register our reservation key with the target */
         ret = prout_register_and_ignore(sd1, key);
         if (ret == -2) {
-                logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE OUT is not implemented.");
                 CU_PASS("PERSISTENT RESERVE OUT is not implemented.");
                 return;
         }        

test-tool/test_prout_reserve_ownership.c

@@ -47,7 +47,6 @@ verify_persistent_reserve_ownership(struct scsi_device *sd1, struct scsi_device
         /* register our reservation key with the target */
         ret = prout_register_and_ignore(sd1, key1);
         if (ret == -2) {
-                logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE OUT is not implemented.");
                 CU_PASS("PERSISTENT RESERVE OUT is not implemented.");
                 return;
         }        

test-tool/test_prout_reserve_simple.c

@@ -55,7 +55,6 @@ test_prout_reserve_simple(void)
         /* register our reservation key with the target */
         ret = prout_register_and_ignore(sd, key);
         if (ret == -2) {
-                logging(LOG_NORMAL, "[SKIPPED] PERSISTEN RESERVE OUT is not implemented.");
                 CU_PASS("PERSISTENT RESERVE OUT is not implemented.");
                 return;
         }