From 0d7261400ac4bc59ea3a13d18562d414337625ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Klaus=20K=C3=A4mpf?= 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 * 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 * 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 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 #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 Improve buffer overflow checking in scsi_command_util::ModeSelect Signed-off-by: Klaus Kämpf --- cpp/devices/scsi_command_util.cpp | 23 ++++++++++++- cpp/test/mocks.h | 4 +++ cpp/test/scsi_command_util_test.cpp | 27 +++++++++++++-- cpp/test/scsihd_test.cpp | 52 +++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 4 deletions(-) diff --git a/cpp/devices/scsi_command_util.cpp b/cpp/devices/scsi_command_util.cpp index 20bb83f76b..940be2bdda 100644 --- a/cpp/devices/scsi_command_util.cpp +++ b/cpp/devices/scsi_command_util.cpp @@ -33,14 +33,23 @@ string scsi_command_util::ModeSelect(scsi_command cmd, cdb_t cdb, span 0) { @@ -62,12 +71,24 @@ string scsi_command_util::ModeSelect(scsi_command cmd, cdb_t cdb, span cmd = { 0x15, 0x10, 0x00, 0x00, 0x19, 0x00 }; + vector 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 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 cmd = { 0x15, 0x10, 0x00, 0x00, static_cast(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"; +}