Skip to content

Commit

Permalink
Multiple fixes for ModeSelect (PiSCSI#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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

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]>

Improve buffer overflow checking in scsi_command_util::ModeSelect

Signed-off-by: Klaus Kämpf <[email protected]>
  • Loading branch information
@kkaempf
kkaempf committed Aug 9, 2024
1 parent 657d22a commit 0d72614
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 4 deletions.
23 changes: 22 additions & 1 deletion cpp/devices/scsi_command_util.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,23 @@ string scsi_command_util::ModeSelect(scsi_command cmd, cdb_t cdb, span<const uin
// Skip block descriptors
int offset;
if (cmd == scsi_command::eCmdModeSelect10) {
if (length < 7) {
spdlog::warn("Incomplete Mode parameter header(10)");
throw scsi_exception(sense_key::illegal_request, asc::invalid_field_in_parameter_list);
}
offset = 8 + GetInt16(buf, 6);
}
else {
if (length < 4) {
spdlog::warn("Incomplete Mode parameter header(6)");
throw scsi_exception(sense_key::illegal_request, asc::invalid_field_in_parameter_list);
}
offset = 4 + buf[3];
}
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,12 +71,24 @@ 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 if ((page == 0x00) && (length == 1)) {
has_valid_page_code = true;
break; // page 0 is valid if last in list, might have no size byte following
}
else {
stringstream s;
s << "Unknown MODE SELECT page code: $" << setfill('0') << setw(2) << hex << page;
result = s.str();
}

if (length < 2) { // ensure there's a size byte before accessing it
spdlog::warn("Current MODE SELECT page has no size");
throw scsi_exception(sense_key::illegal_request, asc::invalid_field_in_parameter_list);
}
// Advance to the next page
const int size = buf[offset + 1] + 2;

Expand Down
4 changes: 4 additions & 0 deletions cpp/test/mocks.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,8 +360,12 @@ class MockSCSIHD : public SCSIHD //NOSONAR Ignore inheritance hierarchy depth in
FRIEND_TEST(ScsiHdTest, FinalizeSetup);
FRIEND_TEST(ScsiHdTest, GetProductData);
FRIEND_TEST(ScsiHdTest, SetUpModePages);
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:
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
52 changes: 52 additions & 0 deletions cpp/test/scsihd_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,3 +125,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";
}

0 comments on commit 0d72614

Please sign in to comment.