From 73917bce171e5f59897cd9397435cb6cc9fdefc3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Klaus=20K=C3=A4mpf?= <kkaempf@gmail.com>
Date: Mon, 1 Apr 2024 15:47:55 +0200
Subject: [PATCH] Fix length calculation in scsi_command_util::ModeSelect
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

OpenVMS Alpha sends a strange ModeSelect payload, apparently one byte
too large. This was 'fixed' by a (wrong) length calculation in #1405, breaking #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 #1402 pass
- adds a testcase for #1402
- adds a testcase for #1427

Fixes issue #1427

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>
---
 cpp/devices/scsi_command_util.cpp |  5 +--
 cpp/test/mocks.h                  |  3 ++
 cpp/test/scsihd_test.cpp          | 52 +++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/cpp/devices/scsi_command_util.cpp b/cpp/devices/scsi_command_util.cpp
index 5c6068a7..f7b31d22 100644
--- 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
index dd3c8268..047d73ad 100644
--- 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
index 81efae30..693d960a 100644
--- 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";
+}