From ad5eae93e7f721817c95a01285babdb97ed940b8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Klaus=20K=C3=A4mpf?= <kkaempf@suse.de>
Date: Sun, 7 Jan 2024 07:06:35 +0100
Subject: [PATCH] Multiple fixes for ModeSelect (#1405)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Allow 'empty' ModeSelect6

tl;dr

Treat a computed length of 0 as `has_valid_page_code`.

Details:

The SRM console (aka 'BIOS') of DEC Alpha sends an empty
ModeSelect6 with the following data:
~~~
ModeSelect6, CDB $151000000c00
~~~

That makes 12 byte(s) as follows
~~~
  0  1  2  3   4   5  6  7   8   9 10 11
 00 00 00 08  00  00 00 00  00  00 02 00
~~~

decoding it (accoring to [1], Section 8.3.3, Table 94) gives us

Mode Data Length 0
Medium Type      0
Device-specific  0
Block desc len   8

Density Code     0
Number of blks   0
Reserved         0
Block length     512

`scsi_command_util::ModeSelect` computes
~~~
offset = 4 + buf[3];
~~~

giving 12 and

~~~
length -= offset;
~~~

giving 0.

Thus it never enters the `while` loop and `has_valid_page_code` stays
`false`, raising an error.

[1] [Small Computer System Interface - 2 rev 10L.pdf](https://dn790004.ca.archive.org/0/items/SCSISpecificationDocumentsSCSIDocuments/Small%20Computer%20System%20Interface%20-%202%20rev%2010L.pdf)

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>

* Allow ModeSelect with page code 1

OpenVMS Alpha (the operating system, not the SRM BIOS) uses
ModeSelect6 with a page code of 1.

The semantics are unknown, just accepting it works for me.

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>

* Fix page length computation in ModeSelect

tl;dr

The 'skip to next ModeSelect page' computation was off-by-one, either not
taking the page code itself into account or missing the fact that the
page length is given as `n - 1`.

Fix:

Add 1 to the computed length.

Details:

OpenVMS Alpha sends a ModeSelect6 as follows
~~~
command:

ModeSelect6, CDB $151000001900

payload:

 0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24
00 00 00 08 00 00 00 00 00 00 02 00 01 0a 24 00 00 00 00 00 00 00 00 00 00
~~~

This translates to (accoring to [1], Section 8.3.3)

~~~
Mode Data Length 0
Medium Type      0
Device-specific  0
Block desc len   8
~~~

with the following offset / length computation _before_ the `while` loop
~~~
offset = 12
length = 13
~~~

The first payload section is
~~~
 4  5  6  7  8  9 10 11
00 00 00 00 00 00 02 00
~~~

translating to

~~~
Density Code     0
Number of blks   0
Reserved         0
Block length   0x200 512
~~~

Then follows a pagecode 1 as
~~~
12 13 14 15 16 17 18 19 20 21 22 23 24
01 0a 24 00 00 00 00 00 00 00 00 00 00
~~~

translating to
~~~~
Page code        1
Page length -1  10
Mode parameters 24 00 00 00 00 00 00 00 00 00 00
~~~

computing (inside the `while` loop, as `// Advance to the next page`)

~~~
size =  10 + 2 = 12
~~~

followed by new `offset` and `length` values

~~~
offset = 25
length = 1
~~~

So it stays in the `while` loop (and has a larger-than-buffer `offset`
value)

Signed-off-by: Klaus Kämpf <kkaempf@gmail.com>
---
 cpp/devices/scsi_command_util.cpp   |  9 +++++++--
 cpp/test/scsi_command_util_test.cpp | 27 ++++++++++++++++++++++++---
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/cpp/devices/scsi_command_util.cpp b/cpp/devices/scsi_command_util.cpp
index 20bb83f7..5c6068a7 100644
--- a/cpp/devices/scsi_command_util.cpp
+++ b/cpp/devices/scsi_command_util.cpp
@@ -40,7 +40,8 @@ string scsi_command_util::ModeSelect(scsi_command cmd, cdb_t cdb, span<const uin
 	}
 	length -= offset;
 
-	bool has_valid_page_code = false;
+	// treat zero length as valid
+	bool has_valid_page_code = (length == 0);
 
 	// Parse the pages
 	while (length > 0) {
@@ -62,6 +63,10 @@ string scsi_command_util::ModeSelect(scsi_command cmd, cdb_t cdb, span<const uin
 
 			has_valid_page_code = true;
 		}
+		else if (page == 0x01) {
+			// OpenVMS Alpha 7.3 uses this
+			has_valid_page_code = true;
+		}
 		else {
 			stringstream s;
 			s << "Unknown MODE SELECT page code: $" << setfill('0') << setw(2) << hex << page;
@@ -71,7 +76,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;
+		length -= size + 1;
 		offset += size;
 	}
 
diff --git a/cpp/test/scsi_command_util_test.cpp b/cpp/test/scsi_command_util_test.cpp
index 20afa389..7bf81550 100644
--- a/cpp/test/scsi_command_util_test.cpp
+++ b/cpp/test/scsi_command_util_test.cpp
@@ -46,6 +46,11 @@ TEST(ScsiCommandUtilTest, ModeSelect6)
 			Property(&scsi_exception::get_asc, asc::invalid_field_in_parameter_list))))
 		<< "Unsupported page 0 was not rejected";
 
+	// Page 1
+	buf[12] = 0x01;
+	EXPECT_NO_THROW(ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, LENGTH, 512))
+		<< "Page 1 is supported";
+
 	// Page 3 (Format Device Page)
 	buf[12] = 0x03;
 	EXPECT_THAT([&] { ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, LENGTH, 512); },
@@ -62,7 +67,25 @@ TEST(ScsiCommandUtilTest, ModeSelect6)
 			Property(&scsi_exception::get_asc, asc::invalid_field_in_parameter_list))))
 		<< "Not enough command parameters";
 
-	EXPECT_FALSE(ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, LENGTH, 512).empty());
+	// check length computation
+	buf[3] = 8;
+	buf[10] = 2;
+	buf[12] = 1;
+	buf[13] = 10;
+	buf[14] = 0x24;
+	buf[24] = 0;
+	EXPECT_NO_THROW(ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, LENGTH, 512))
+		<< "Multi-page length computation";
+
+	// check length computation
+	buf[3] = 8;
+	buf[10] = 12;
+	buf[12] = 0;
+	buf[13] = 0;
+	buf[14] = 0;
+	buf[24] = 0;
+	EXPECT_NO_THROW(ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, 12, 512))
+		<< "Empty ModeSelect6";
 }
 
 TEST(ScsiCommandUtilTest, ModeSelect10)
@@ -111,8 +134,6 @@ TEST(ScsiCommandUtilTest, ModeSelect10)
 			Property(&scsi_exception::get_sense_key, sense_key::illegal_request),
 			Property(&scsi_exception::get_asc, asc::invalid_field_in_parameter_list))))
 		<< "Not enough command parameters";
-
-	EXPECT_FALSE(ModeSelect(scsi_command::eCmdModeSelect10, cdb, buf, LENGTH, 512).empty());
 }
 
 TEST(ScsiCommandUtilTest, EnrichFormatPage)