From f310938684d5544ab103e4c33f32eaf17ca5acd2 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Fri, 25 Sep 2026 00:13:48 +0000 Subject: [PATCH] Message: Validate PHY register batches --- CMakeLists.txt | 1 + communication/packet/ethphyregpacket.cpp | 27 ++++- device/device.cpp | 7 +- .../communication/message/ethphymessage.h | 12 +- .../communication/packet/ethphyregpacket.h | 2 +- test/unit/ethphyregistertest.cpp | 103 ++++++++++++++++++ 6 files changed, 141 insertions(+), 11 deletions(-) create mode 100644 test/unit/ethphyregistertest.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 3b40ab1..f949dd0 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -574,6 +574,7 @@ if(LIBICSNEO_BUILD_UNIT_TESTS) test/unit/ringbuffertest.cpp test/unit/apperrordecodertest.cpp test/unit/icsneoc2.cpp + test/unit/ethphyregistertest.cpp test/unit/windowsstrings.cpp test/unit/periodictest.cpp ) diff --git a/communication/packet/ethphyregpacket.cpp b/communication/packet/ethphyregpacket.cpp index 33b8bfb..7015ab1 100644 --- a/communication/packet/ethphyregpacket.cpp +++ b/communication/packet/ethphyregpacket.cpp @@ -20,7 +20,7 @@ std::shared_ptr HardwareEthernetPhyRegisterPacket::DecodeToMessag if( (PhyPacketVersion == pHeader->version) && (sizeof(PhyRegisterPacket_t) == pHeader->entryBytes) && - (numEntries <= MaxPhyEntries) && + (numEntries > 0 && numEntries <= MaxPhyEntries) && ((bytestream.size() - sizeof(PhyRegisterHeader_t)) == (sizeof(PhyRegisterPacket_t) * numEntries)) ) @@ -30,6 +30,10 @@ std::shared_ptr HardwareEthernetPhyRegisterPacket::DecodeToMessag for(size_t entryIdx{0}; entryIdx < numEntries; ++entryIdx) { const PhyRegisterPacket_t* pEntry = (pFirstEntry + entryIdx); + if(pEntry->version != PhyPacketVersion) { + report(APIEvent::Type::ParameterOutOfRange, APIEvent::Severity::Error); + return nullptr; + } auto phyMessage = std::make_shared(); phyMessage->Enabled = (pEntry->Enabled != 0u); phyMessage->WriteEnable = (pEntry->WriteEnable != 0u); @@ -43,6 +47,10 @@ std::shared_ptr HardwareEthernetPhyRegisterPacket::DecodeToMessag msg->messages.push_back(phyMessage); } } + else { + report(APIEvent::Type::ParameterOutOfRange, APIEvent::Severity::Error); + return nullptr; + } return msg; } @@ -59,7 +67,20 @@ bool HardwareEthernetPhyRegisterPacket::EncodeFromMessage(const EthPhyMessage& m report(APIEvent::Type::MessageMaxLengthExceeded, APIEvent::Severity::Error); return false; } - auto byteSize = (messageCount * sizeof(PhyRegisterPacket_t)) + sizeof(PhyRegisterHeader_t); + // Validate every entry before touching the caller's output or submitting a command. + for(const auto& entry : message.messages) { + if(!entry) { + report(APIEvent::Type::RequiredParameterNull, APIEvent::Severity::Error); + return false; + } + if(entry->BusIndex > 15 || entry->Version != PhyPacketVersion || + (entry->Clause45Enable ? (entry->Clause45.port > FiveBits || entry->Clause45.device > FiveBits) : + (entry->Clause22.phyAddr > FiveBits || entry->Clause22.regAddr > FiveBits))) { + report(APIEvent::Type::ParameterOutOfRange, APIEvent::Severity::Error); + return false; + } + } + auto byteSize = bytestream.size() + (messageCount * sizeof(PhyRegisterPacket_t)) + sizeof(PhyRegisterHeader_t); bytestream.reserve(byteSize); bytestream.push_back(static_cast(messageCount & 0xFF)); bytestream.push_back(static_cast((messageCount >> 8) & 0xFF)); @@ -67,7 +88,7 @@ bool HardwareEthernetPhyRegisterPacket::EncodeFromMessage(const EthPhyMessage& m bytestream.push_back(static_cast(sizeof(PhyRegisterPacket_t))); for(auto& phyMessage : message.messages) { - PhyRegisterPacket_t tempPacket; + PhyRegisterPacket_t tempPacket{}; tempPacket.Enabled = phyMessage->Enabled ? 0x1u : 0x0u; tempPacket.WriteEnable = phyMessage->WriteEnable ? 0x1u : 0x0u; tempPacket.BusIndex = (phyMessage->BusIndex & 0xF); diff --git a/device/device.cpp b/device/device.cpp index c087b8d..4818e75 100644 --- a/device/device.cpp +++ b/device/device.cpp @@ -2053,7 +2053,12 @@ std::optional Device::sendEthPhyMsg(const EthPhyMessage& message, } std::vector bytes; - HardwareEthernetPhyRegisterPacket::EncodeFromMessage(message, bytes, report); + if(timeout.count() <= 0) { + report(APIEvent::Type::ParameterOutOfRange, APIEvent::Severity::Error); + return std::nullopt; + } + if(!HardwareEthernetPhyRegisterPacket::EncodeFromMessage(message, bytes, report)) + return std::nullopt; std::shared_ptr response = com->waitForMessageSync( [this, bytes](){ return com->sendCommand(Command::PHYControlRegisters, bytes); }, std::make_shared(Message::Type::EthernetPhyRegister), timeout); diff --git a/include/icsneo/communication/message/ethphymessage.h b/include/icsneo/communication/message/ethphymessage.h index 9220d98..18de5e1 100644 --- a/include/icsneo/communication/message/ethphymessage.h +++ b/include/icsneo/communication/message/ethphymessage.h @@ -17,13 +17,13 @@ namespace icsneo { #endif struct PhyMessage { - bool Enabled; - bool WriteEnable; - bool Clause45Enable; - uint8_t BusIndex; - uint8_t Version; + bool Enabled = false; + bool WriteEnable = false; + bool Clause45Enable = false; + uint8_t BusIndex = 0; + uint8_t Version = PhyPacketVersion; union { - Clause22Message Clause22; + Clause22Message Clause22{}; Clause45Message Clause45; }; }; diff --git a/include/icsneo/communication/packet/ethphyregpacket.h b/include/icsneo/communication/packet/ethphyregpacket.h index 7c2bfcb..acbf7cf 100644 --- a/include/icsneo/communication/packet/ethphyregpacket.h +++ b/include/icsneo/communication/packet/ethphyregpacket.h @@ -65,7 +65,7 @@ struct PhyRegisterPacket_t { #pragma pack(pop) static constexpr size_t MaxPhyEntries = 128u; -static constexpr size_t MaxBytesPhyEntries = MaxPhyEntries * sizeof(PhyRegisterHeader_t); +static constexpr size_t MaxBytesPhyEntries = MaxPhyEntries * sizeof(PhyRegisterPacket_t); static constexpr uint8_t PhyPacketVersion = 1u; static constexpr uint8_t FiveBits = 0x1Fu; diff --git a/test/unit/ethphyregistertest.cpp b/test/unit/ethphyregistertest.cpp new file mode 100644 index 0000000..92b540a --- /dev/null +++ b/test/unit/ethphyregistertest.cpp @@ -0,0 +1,103 @@ +#include "icsneo/communication/message/ethphymessage.h" +#include "icsneo/communication/packet/ethphyregpacket.h" +#include "gtest/gtest.h" +#include "icsneo/device/tree/radjupiter/radjupiter.h" +using namespace icsneo; +namespace { +const device_eventhandler_t ignore = [](APIEvent::Type, APIEvent::Severity) {}; +EthPhyMessage request(bool clause45 = false, bool write = false) { + EthPhyMessage msg; + msg.appendPhyMessage(write, clause45, 31, clause45 ? 31 : 255, clause45 ? 65535 : 31, 0x1234); + msg.messages[0]->BusIndex = 15; + return msg; +} +std::vector encode(const EthPhyMessage& msg) { + std::vector bytes; + EXPECT_TRUE(HardwareEthernetPhyRegisterPacket::EncodeFromMessage(msg, bytes, ignore)); + return bytes; +} +} +TEST(EthPhyRegister, KnownWireBytes) { + for(bool clause45 : {false, true}) { + auto msg = request(clause45, true); + auto bytes = encode(msg); + EXPECT_EQ(bytes, (std::vector{1, 0, 1, 8, static_cast(clause45 ? 7 : 3), 0x1f, + 31, static_cast(clause45 ? 31 : 255), static_cast(clause45 ? 255 : 31), + static_cast(clause45 ? 255 : 0), 0x34, 0x12})); + } +} +TEST(EthPhyRegister, InvalidRequestsLeaveOutputUntouched) { + for(int invalid = 0; invalid < 8; ++invalid) { + auto msg = request(); + switch(invalid) { + case 0: msg.messages.clear(); break; + case 1: msg.messages.resize(129, msg.messages[0]); break; + case 2: msg.messages.push_back(nullptr); break; + case 3: msg.messages[0]->BusIndex = 16; break; + case 4: msg.messages[0]->Version = 0; break; + case 5: msg.messages[0]->Clause22.phyAddr = 32; break; + case 6: msg.messages[0]->Clause22.regAddr = 32; break; + case 7: msg.messages = request(true).messages; msg.messages[0]->Clause45.device = 32; break; + } + std::vector bytes{0xaa}; + EXPECT_FALSE(HardwareEthernetPhyRegisterPacket::EncodeFromMessage(msg, bytes, ignore)); + EXPECT_EQ(bytes, (std::vector{0xaa})); + } + auto msg = request(); msg.messages.resize(128, msg.messages[0]); + EXPECT_EQ(encode(msg).size(), 1028u); +} +TEST(EthPhyRegister, RejectMalformedResponses) { + const auto valid = encode(request()); + for(size_t size = 0; size < valid.size(); ++size) + EXPECT_EQ(HardwareEthernetPhyRegisterPacket::DecodeToMessage({valid.begin(), valid.begin() + size}, ignore), nullptr); + for(size_t index : {0u, 2u, 3u, 5u}) { + auto bytes = valid; bytes[index] = 0; + EXPECT_EQ(HardwareEthernetPhyRegisterPacket::DecodeToMessage(bytes, ignore), nullptr); + } + auto extra = valid; extra.push_back(0); + EXPECT_EQ(HardwareEthernetPhyRegisterPacket::DecodeToMessage(extra, ignore), nullptr); +} + +namespace { +class PhyTestDriver : public Driver { +public: + explicit PhyTestDriver(const device_eventhandler_t& report) : Driver(report) {} + bool open() override { return false; } + bool isOpen() override { return opened; } + bool opened = true; + bool close() override { opened = false; return true; } + driver_finder_t getFinder() override { return [](std::vector&) {}; } + int writes = 0; +protected: + bool writeInternal(const std::vector&) override { ++writes; return false; } +}; +class PhyTestJupiter : public RADJupiter { +public: + explicit PhyTestJupiter(const FoundDevice& found) : RADJupiter(found) {} + ~PhyTestJupiter() override { com->driver->close(); } + bool isOpen() const override { return true; } // Exercise the native send path, without hardware. +}; +} +TEST(EthPhyRegister, DeviceDoesNotSendInvalidRequests) { + PhyTestDriver* driver = nullptr; + FoundDevice found; + found.makeDriver = [&driver](device_eventhandler_t report, neodevice_t&) { + auto result = std::make_unique(report); + driver = result.get(); + return result; + }; + PhyTestJupiter device(found); + auto msg = request(); + msg.messages.push_back(nullptr); + EXPECT_FALSE(device.sendEthPhyMsg(msg).has_value()); + EXPECT_EQ(driver->writes, 0); + msg.messages = request().messages; msg.messages[0]->BusIndex = 16; + EXPECT_FALSE(device.sendEthPhyMsg(msg).has_value()); + EXPECT_EQ(driver->writes, 0); + msg.messages = request().messages; + EXPECT_FALSE(device.sendEthPhyMsg(msg, std::chrono::milliseconds(0)).has_value()); + EXPECT_EQ(driver->writes, 0); + // The valid request reaches the fake driver's explicit send failure. + EXPECT_FALSE(device.sendEthPhyMsg(msg).has_value()); + EXPECT_EQ(driver->writes, 1); +}