From 4e4c5b205a143d8d396d0bcabdf1beab62228330 Mon Sep 17 00:00:00 2001 From: Uwe Seimet <48174652+uweseimet@users.noreply.github.com> Date: Mon, 10 Oct 2022 08:16:47 +0200 Subject: [PATCH] Bugfix: MODE SELECT for format page is incorrect (issue #818) (#899) * Fix issue with MODE SELECT (#818) * Replaced strncpy because it was causing a compilation issue --- .../controllers/scsi_controller.cpp | 45 ++++-------- src/raspberrypi/controllers/scsi_controller.h | 2 +- src/raspberrypi/devices/device_factory.cpp | 2 +- src/raspberrypi/devices/scsi_command_util.cpp | 62 +++++++++++------ src/raspberrypi/devices/scsi_command_util.h | 1 + .../test/scsi_command_util_test.cpp | 68 ++++++++++++++++--- 6 files changed, 119 insertions(+), 61 deletions(-) diff --git a/src/raspberrypi/controllers/scsi_controller.cpp b/src/raspberrypi/controllers/scsi_controller.cpp index df4667aa..783054cf 100644 --- a/src/raspberrypi/controllers/scsi_controller.cpp +++ b/src/raspberrypi/controllers/scsi_controller.cpp @@ -625,6 +625,7 @@ void ScsiController::Receive() } // If result FALSE, move to status phase + // TODO Check whether we can raise scsi_exception here in order to simplify the error handling if (!result) { Error(sense_key::ABORTED_COMMAND); return; @@ -648,7 +649,8 @@ void ScsiController::Receive() break; case BUS::phase_t::dataout: - FlushUnit(); + // Block-oriented data have been handled above + DataOutNonBlockOriented(); Status(); break; @@ -769,47 +771,30 @@ bool ScsiController::XferOut(bool cont) return false; } -void ScsiController::FlushUnit() +void ScsiController::DataOutNonBlockOriented() { assert(IsDataOut()); - auto disk = dynamic_pointer_cast(GetDeviceForLun(GetEffectiveLun())); - if (disk == nullptr) { - return; - } - - // WRITE system only switch (GetOpcode()) { - case scsi_command::eCmdWrite6: - case scsi_command::eCmdWrite10: - case scsi_command::eCmdWrite16: - case scsi_command::eCmdWriteLong10: - case scsi_command::eCmdWriteLong16: - case scsi_command::eCmdVerify10: - case scsi_command::eCmdVerify16: - break; - case scsi_command::eCmdModeSelect6: - case scsi_command::eCmdModeSelect10: - // TODO What is this special handling of ModeSelect good for? - // Without it we would not need this method at all. - // ModeSelect is already handled in XferOutBlockOriented(). Why would it have to be handled once more? - try { - disk->ModeSelect(ctrl.cmd, GetBuffer(), GetOffset()); + case scsi_command::eCmdModeSelect10: { + // TODO Try to get rid of this cast + if (auto device = dynamic_pointer_cast(GetDeviceForLun(GetEffectiveLun())); + device != nullptr) { + device->ModeSelect(ctrl.cmd, GetBuffer(), GetOffset()); + } + else { + throw scsi_exception(sense_key::ILLEGAL_REQUEST, asc::INVALID_COMMAND_OPERATION_CODE); + } } - catch(const scsi_exception& e) { - LOGWARN("Error occured while processing Mode Select command %02X\n", (int)GetOpcode()) - Error(e.get_sense_key(), e.get_asc(), e.get_status()); - return; - } - break; + break; case scsi_command::eCmdSetMcastAddr: // TODO: Eventually, we should store off the multicast address configuration data here... break; default: - LOGWARN("Received an unexpected flush command $%02X\n", (int)GetOpcode()) + LOGWARN("Unexpected Data Out phase for command $%02X", (int)GetOpcode()) break; } } diff --git a/src/raspberrypi/controllers/scsi_controller.h b/src/raspberrypi/controllers/scsi_controller.h index d12bd98a..52fdd5ce 100644 --- a/src/raspberrypi/controllers/scsi_controller.h +++ b/src/raspberrypi/controllers/scsi_controller.h @@ -103,7 +103,7 @@ private: void ReceiveBytes(); void Execute(); - void FlushUnit(); + void DataOutNonBlockOriented(); void Receive(); void ProcessCommand(); diff --git a/src/raspberrypi/devices/device_factory.cpp b/src/raspberrypi/devices/device_factory.cpp index 490ca133..5a3ab52a 100644 --- a/src/raspberrypi/devices/device_factory.cpp +++ b/src/raspberrypi/devices/device_factory.cpp @@ -220,7 +220,7 @@ list DeviceFactory::GetNetworkInterfaces() const const int fd = socket(PF_INET, SOCK_DGRAM, IPPROTO_IP); ifreq ifr = {}; - strncpy(ifr.ifr_name, tmp->ifa_name, IFNAMSIZ); //NOSONAR Using strncpy is safe here + strcpy(ifr.ifr_name, tmp->ifa_name); //NOSONAR Using strcpy is safe here // Only list interfaces that are up if (!ioctl(fd, SIOCGIFFLAGS, &ifr) && (ifr.ifr_flags & IFF_UP)) { network_interfaces.emplace_back(tmp->ifa_name); diff --git a/src/raspberrypi/devices/scsi_command_util.cpp b/src/raspberrypi/devices/scsi_command_util.cpp index b2f3849f..4f1788a1 100644 --- a/src/raspberrypi/devices/scsi_command_util.cpp +++ b/src/raspberrypi/devices/scsi_command_util.cpp @@ -23,32 +23,30 @@ void scsi_command_util::ModeSelect(const vector& cdb, const vector& b throw scsi_exception(sense_key::ILLEGAL_REQUEST, asc::INVALID_FIELD_IN_PARAMETER_LIST); } + // Skip block descriptors + int offset; + if ((scsi_command)cdb[0] == scsi_command::eCmdModeSelect10) { + offset = 8 + GetInt16(buf, 6); + } + else { + offset = 4 + buf[3]; + } + length -= offset; + bool has_valid_page_code = false; - // Mode Parameter header - int offset = 0; - if (length >= 12) { - // Check the block length - if (buf[9] != (BYTE)(sector_size >> 16) || buf[10] != (BYTE)(sector_size >> 8) || - buf[11] != (BYTE)sector_size) { - // See below for details - LOGWARN("In order to change the sector size use the -b option when launching rascsi") - throw scsi_exception(sense_key::ILLEGAL_REQUEST, asc::INVALID_FIELD_IN_PARAMETER_LIST); - } - - offset += 12; - length -= 12; - } - - // Parsing the page - // TODO The length handling is wrong in case of length < size + // Parse the pages while (length > 0) { // Format device page if (int page = buf[offset]; page == 0x03) { + if (length < 14) { + throw scsi_exception(sense_key::ILLEGAL_REQUEST, asc::INVALID_FIELD_IN_PARAMETER_LIST); + } + // With this page the sector size for a subsequent FORMAT can be selected, but only very few // drives support this, e.g FUJITSU M2624S // We are fine as long as the current sector size remains unchanged - if (buf[offset + 0xc] != (BYTE)(sector_size >> 8) || buf[offset + 0xd] != (BYTE)sector_size) { + if (GetInt16(buf, offset + 12) != sector_size) { // With rascsi it is not possible to permanently (by formatting) change the sector size, // because the size is an externally configurable setting only LOGWARN("In order to change the sector size use the -b option when launching rascsi") @@ -63,6 +61,7 @@ void scsi_command_util::ModeSelect(const vector& cdb, const vector& b // Advance to the next page int size = buf[offset + 1] + 2; + length -= size; offset += size; } @@ -92,30 +91,45 @@ void scsi_command_util::AddAppleVendorModePage(map>& pages, bo // No changeable area if (!changeable) { const char APPLE_DATA[] = "APPLE COMPUTER, INC "; - memcpy(&buf[2], APPLE_DATA, sizeof(APPLE_DATA)); + memcpy(&buf.data()[2], APPLE_DATA, sizeof(APPLE_DATA)); } pages[48] = buf; } +int scsi_command_util::GetInt16(const vector& buf, int offset) +{ + assert(buf.size() > (size_t)offset + 1); + + return ((int)buf[offset] << 8) | buf[offset + 1]; +} + int scsi_command_util::GetInt16(const vector& buf, int offset) { + assert(buf.size() > (size_t)offset + 1); + return (buf[offset] << 8) | buf[offset + 1]; } int scsi_command_util::GetInt24(const vector& buf, int offset) { + assert(buf.size() > (size_t)offset + 2); + return (buf[offset] << 16) | (buf[offset + 1] << 8) | buf[offset + 2]; } uint32_t scsi_command_util::GetInt32(const vector& buf, int offset) { + assert(buf.size() > (size_t)offset + 3); + return ((uint32_t)buf[offset] << 24) | ((uint32_t)buf[offset + 1] << 16) | ((uint32_t)buf[offset + 2] << 8) | (uint32_t)buf[offset + 3]; } uint64_t scsi_command_util::GetInt64(const vector& buf, int offset) { + assert(buf.size() > (size_t)offset + 7); + return ((uint64_t)buf[offset] << 56) | ((uint64_t)buf[offset + 1] << 48) | ((uint64_t)buf[offset + 2] << 40) | ((uint64_t)buf[offset + 3] << 32) | ((uint64_t)buf[offset + 4] << 24) | ((uint64_t)buf[offset + 5] << 16) | @@ -124,12 +138,16 @@ uint64_t scsi_command_util::GetInt64(const vector& buf, int offset) void scsi_command_util::SetInt16(vector& buf, int offset, int value) { + assert(buf.size() > (size_t)offset + 1); + buf[offset] = (byte)(value >> 8); buf[offset + 1] = (byte)value; } void scsi_command_util::SetInt32(vector& buf, int offset, uint32_t value) { + assert(buf.size() > (size_t)offset + 3); + buf[offset] = (byte)(value >> 24); buf[offset + 1] = (byte)(value >> 16); buf[offset + 2] = (byte)(value >> 8); @@ -138,12 +156,16 @@ void scsi_command_util::SetInt32(vector& buf, int offset, uint32_t value) void scsi_command_util::SetInt16(vector& buf, int offset, int value) { + assert(buf.size() > (size_t)offset + 1); + buf[offset] = (BYTE)(value >> 8); buf[offset + 1] = (BYTE)value; } void scsi_command_util::SetInt32(vector& buf, int offset, uint32_t value) { + assert(buf.size() > (size_t)offset + 3); + buf[offset] = (BYTE)(value >> 24); buf[offset + 1] = (BYTE)(value >> 16); buf[offset + 2] = (BYTE)(value >> 8); @@ -152,6 +174,8 @@ void scsi_command_util::SetInt32(vector& buf, int offset, uint32_t value) void scsi_command_util::SetInt64(vector& buf, int offset, uint64_t value) { + assert(buf.size() > (size_t)offset + 7); + buf[offset] = (BYTE)(value >> 56); buf[offset + 1] = (BYTE)(value >> 48); buf[offset + 2] = (BYTE)(value >> 40); diff --git a/src/raspberrypi/devices/scsi_command_util.h b/src/raspberrypi/devices/scsi_command_util.h index 658f7d0a..5f95a8d2 100644 --- a/src/raspberrypi/devices/scsi_command_util.h +++ b/src/raspberrypi/devices/scsi_command_util.h @@ -22,6 +22,7 @@ namespace scsi_command_util void EnrichFormatPage(map>&, bool, int); void AddAppleVendorModePage(map>&, bool); + int GetInt16(const vector&, int); int GetInt16(const vector&, int); int GetInt24(const vector&, int); uint32_t GetInt32(const vector&, int); diff --git a/src/raspberrypi/test/scsi_command_util_test.cpp b/src/raspberrypi/test/scsi_command_util_test.cpp index dc9f7203..bfe65c5a 100644 --- a/src/raspberrypi/test/scsi_command_util_test.cpp +++ b/src/raspberrypi/test/scsi_command_util_test.cpp @@ -13,18 +13,20 @@ using namespace scsi_command_util; -TEST(ScsiCommandUtilTest, ModeSelect) +TEST(ScsiCommandUtilTest, ModeSelect6) { - const int LENGTH = 12; + const int LENGTH = 26; - vector cdb(16); - vector buf(255); + vector cdb(6); + vector buf(LENGTH); // PF (vendor-specific parameter format) cdb[1] = 0x00; EXPECT_THROW(ModeSelect(cdb, buf, LENGTH, 0), scsi_exception) << "Vendor-specific parameters are not supported"; + cdb[0] = (int)scsi_command::eCmdModeSelect6; + cdb[0] = 0x15; // PF (standard parameter format) cdb[1] = 0x10; // Request 512 bytes per sector @@ -35,18 +37,61 @@ TEST(ScsiCommandUtilTest, ModeSelect) << "Requested sector size does not match current sector size"; // Page 0 - buf[LENGTH] = 0x00; - EXPECT_THROW(ModeSelect(cdb, buf, LENGTH + 2, 512), scsi_exception) + buf[12] = 0x00; + EXPECT_THROW(ModeSelect(cdb, buf, LENGTH, 512), scsi_exception) << "Unsupported page 0 was not rejected"; // Page 3 (Format Device Page) - buf[LENGTH] = 0x03; - EXPECT_THROW(ModeSelect(cdb, buf, LENGTH + 2, 512), scsi_exception) + buf[12] = 0x03; + EXPECT_THROW(ModeSelect(cdb, buf, LENGTH, 512), scsi_exception) << "Requested sector size does not match current sector size"; // Match the requested to the current sector size - buf[LENGTH + 12] = 0x02; - ModeSelect(cdb, buf, LENGTH + 2, 512); + buf[24] = 0x02; + EXPECT_THROW(ModeSelect(cdb, buf, LENGTH - 1, 512), scsi_exception) + << "Not enough command parameters"; + + ModeSelect(cdb, buf, LENGTH, 512); +} + +TEST(ScsiCommandUtilTest, ModeSelect10) +{ + const int LENGTH = 30; + + vector cdb(10); + vector buf(LENGTH); + + // PF (vendor-specific parameter format) + cdb[1] = 0x00; + EXPECT_THROW(ModeSelect(cdb, buf, LENGTH, 0), scsi_exception) + << "Vendor-specific parameters are not supported"; + + cdb[0] = (int)scsi_command::eCmdModeSelect10; + // PF (standard parameter format) + cdb[1] = 0x10; + // Request 512 bytes per sector + buf[13] = 0x00; + buf[14] = 0x02; + buf[15] = 0x00; + EXPECT_THROW(ModeSelect(cdb, buf, LENGTH, 256), scsi_exception) + << "Requested sector size does not match current sector size"; + + // Page 0 + buf[16] = 0x00; + EXPECT_THROW(ModeSelect(cdb, buf, LENGTH, 512), scsi_exception) + << "Unsupported page 0 was not rejected"; + + // Page 3 (Format Device Page) + buf[16] = 0x03; + EXPECT_THROW(ModeSelect(cdb, buf, LENGTH, 512), scsi_exception) + << "Requested sector size does not match current sector size"; + + // Match the requested to the current sector size + buf[28] = 0x02; + EXPECT_THROW(ModeSelect(cdb, buf, LENGTH - 1, 512), scsi_exception) + << "Not enough command parameters"; + + ModeSelect(cdb, buf, LENGTH, 512); } TEST(ScsiCommandUtilTest, EnrichFormatPage) @@ -85,6 +130,9 @@ TEST(ScsiCommandUtilTest, AddAppleVendorModePage) TEST(ScsiCommandUtilTest, GetInt16) { + vector b = { 0xfe, 0xdc }; + EXPECT_EQ(0xfedc, GetInt16(b, 0)); + vector v = { 0x12, 0x34 }; EXPECT_EQ(0x1234, GetInt16(v, 0)); }