Skip to content

Commit

Permalink
Multiple fixes for ModeSelect (#1405)
Browse files Browse the repository at this point in the history 
* 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>
  • Loading branch information
@kkaempf
kkaempf authored Jan 7, 2024
1 parent 2566509 commit ad5eae9
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 5 deletions.
9 changes: 7 additions & 2 deletions cpp/devices/scsi_command_util.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
Expand All @@ -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;
}

Expand Down
27 changes: 24 additions & 3 deletions cpp/test/scsi_command_util_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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); },
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit ad5eae9

Please sign in to comment.