Fix length calculation in scsi_command_util::ModeSelect

OpenVMS Alpha sends a strange ModeSelect payload, apparently one byte too large. This was 'fixed' by a (wrong) length calculation in PiSCSI#1405, breaking PiSCSI#1427. This PR - fixes the wrong length calculation - improves the loop test in scsi_command_util::ModeSelect to prevent a buffer overflow. (Remaining length was checked for > 0, but buffer access is at offset and offset + 1, effectively requiring 2 bytes.) - the loop test fix makes PiSCSI#1402 pass - adds a testcase for PiSCSI#1402 - adds a testcase for PiSCSI#1427 Fixes issue PiSCSI#1427 Signed-off-by: Klaus Kämpf <[email protected]>
kkaempf · Apr 1, 2024 · 73917bc · 73917bc
1 parent b1a3ffb
commit 73917bc
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 2 deletions.
diff --git a/cpp/devices/scsi_command_util.cpp b/cpp/devices/scsi_command_util.cpp
@@ -44,7 +44,8 @@ string scsi_command_util::ModeSelect(scsi_command cmd, cdb_t cdb, span<const uin
 	bool has_valid_page_code = (length == 0);
 
 	// Parse the pages
-	while (length > 0) {
+        // expect (remaining) length > 1 because we access buf[offset+1] below
+	while (length > 1) {
 		// Format device page
 		if (const int page = buf[offset]; page == 0x03) {
 			if (length < 14) {
@@ -76,7 +77,7 @@ string scsi_command_util::ModeSelect(scsi_command cmd, cdb_t cdb, span<const uin
 		// Advance to the next page
 		const int size = buf[offset + 1] + 2;
 
-		length -= size + 1;
+		length -= size;
 		offset += size;
 	}
 

diff --git a/cpp/test/mocks.h b/cpp/test/mocks.h
@@ -363,6 +363,9 @@ class MockSCSIHD : public SCSIHD //NOSONAR Ignore inheritance hierarchy depth in
 	FRIEND_TEST(ScsiHdTest, DECSpecialFunctionControlPage);
 	FRIEND_TEST(ScsiHdTest, GetSectorSizes);
 	FRIEND_TEST(ScsiHdTest, ModeSelect);
+	FRIEND_TEST(ScsiHdTest, PageCode1);
+	FRIEND_TEST(ScsiHdTest, MultiplePages);
+
 	FRIEND_TEST(PiscsiExecutorTest, SetSectorSize);
 
 public:

diff --git a/cpp/test/scsihd_test.cpp b/cpp/test/scsihd_test.cpp
@@ -140,3 +140,55 @@ TEST(ScsiHdTest, ModeSelect)
 	buf[20] = 0x02;
 	EXPECT_NO_THROW(hd.ModeSelect(scsi_command::eCmdModeSelect10, cmd, buf, 255)) << "MODE SELECT(10) is supported";
 }
+
+// Test for the ModeSelect6, page code 1 (issued by Alpha VMS)
+TEST(ScsiHdTest, PageCode1)
+{
+	MockSCSIHD hd(0, false);
+	vector<int> cmd = { 0x15, 0x10, 0x00, 0x00, 0x19, 0x00 };
+	vector<uint8_t> buf = { 0x00, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00
+		, 0x01, 0x0a, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+        };
+
+	EXPECT_NO_THROW(hd.ModeSelect(scsi_command::eCmdModeSelect6, cmd, buf, buf.size())) << "Page code 1 is supported";
+}
+
+// Test for the ModeSelect6, multiple pages
+TEST(ScsiHdTest, MultiplePages)
+{
+	MockSCSIHD hd(0, false);
+	vector<uint8_t> buf = { 0x00, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00
+                , 0x02, 0x01, 0x00  // 12
+		, 0x02, 0x01, 0x00  // 15
+		, 0x02, 0x01, 0x00  // 18
+		, 0x02, 0x01, 0x00  // 21
+		, 0x02, 0x01, 0x00  // 24
+		, 0x02, 0x01, 0x00  // 27
+		, 0x02, 0x01, 0x00  // 30
+		, 0x02, 0x01, 0x00  // 33
+		, 0x02, 0x01, 0x00  // 36
+		, 0x02, 0x01, 0x00  // 39
+		, 0x02, 0x01, 0x00  // 42
+		, 0x02, 0x01, 0x00  // 45
+		, 0x02, 0x01, 0x00  // 48
+		, 0x02, 0x01, 0x00  // 51
+		, 0x02, 0x01, 0x00  // 54
+		, 0x02, 0x01, 0x00  // 57
+		, 0x02, 0x01, 0x00  // 60
+		, 0x02, 0x01, 0x00  // 63
+		, 0x02, 0x01, 0x00  // 66
+		, 0x02, 0x01, 0x00  // 69
+		, 0x02, 0x01, 0x00  // 72
+		, 0x02, 0x01, 0x00  // 75
+		, 0x02, 0x01, 0x00  // 78
+		, 0x02, 0x01, 0x00  // 81
+		, 0x02, 0x01, 0x00  // 84
+		, 0x02, 0x01, 0x00  // 87
+		, 0x02, 0x01, 0x00  // 90
+		, 0x03, 0x16, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x19, 0x08, 0x00, 0x00, 0x01, 0x00, 0x0b, 0x00, 0x14, 0x00, 0x00, 0x00, 0x00
+        };
+	vector<int> cmd = { 0x15, 0x10, 0x00, 0x00, static_cast<int>(buf.size()), 0x00 };
+
+	hd.SetSectorSizeInBytes(2048); // pass the "page 3" sector_size test in scsi_command_util::ModeSelect
+	EXPECT_NO_THROW(hd.ModeSelect(scsi_command::eCmdModeSelect6, cmd, buf, buf.size())) << "Multiple pages are supported";
+}