Checking open source UEFI for Intel Galileo with PVS-Studio

The development of firmware, even if it is not being done in assembler for exotic architectures, but in C for i386 / amd64 is quite a difficult task, and the cost of an error can be extremely high, even until the target hardware platform fails, so the use of various error prevention techniques in the very early stages of development - the need.

Unfortunately, formal verification or the use of MISRA C in the case of UEFI firmware can only be dreamed of (on the other hand, few people want to spend a couple of years and 50% of the project’s budget on firmware development), so today we’ll talk about static analysis, or rather the PVS-Studio static analyzer popular on Habré, which we will try to find errors in the open source UEFI code for Intel Galileo.

For the results of the check, I ask for kat.

Setting up the environment

As Captain Obviously tells me, to analyze any code, we need the analyzer, the code itself and the build environment for it.
')
The analyzer lies on the site of its developer , after installation we write him a letter asking for a short time to provide the key, which allows seeing not only the first level warnings (namely, this is done in the trial versions), but everything in general - in our case it is better to perebdet than not to finish.

The firmware code is part of Quark BSP and is based on EDK 2010.SR1, like all current implementations of UEFI, with the exception of Apple products.

EDK has its own build system, so we will use PVS-Studio Standalone to check the code collected by it. How to prepare Quark_EDKII package for assembly - read in this document , I will not dwell on it here in more detail.

Run analyzer

We launch PVS-Studio Standalone, click the Analyze your files ... button, the Compiler Monitoring window opens, in which we press the single Start Monitoring button. Now open the console in the folder with Quark_EDKII and execute the quarkbuild -r32 S QuarkPlatform command to build the release version of the firmware. We are waiting for the end of the assembly, in the Compiler Monitoring window we observe the increase in the number of detected calls to the compiler, after the end of the assembly we press the Stop Monitoring button and wait for the analysis to finish.

Analysis results

For the current version of Quark_EDKII_v1.1.0, the analyzer displays 96 warnings of the first level, 100 - of the second and 63-third (with default settings, i.e., only General Analysis is selected). Sort them by the number of warnings and proceed to search for errors.

Warning : V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct.
File : quarkplatformpkg \ pci \ dxe \ pcihostbridge \ pcihostbridge.c, 181, 272
Code :

for (TotalRootBridgeFound = 0, IioResourceMapEntry = 0; TotalRootBridgeFound < HostBridge->RootBridgeCount, IioResourceMapEntry < MaxIIO; IioResourceMapEntry++) { 

Comment : improper use of the comma operator in the condition. Let me remind you that this operator has the lowest priority, it calculates the values ​​of both operands, but itself takes the value of the right one. In this case, the condition is completely analogous to IioResourceMapEntry <MaxIIO, and the TotalRootBridgeFound <HostBridge-> RootBridgeCount check, although fulfilled, does not affect the continuation or interruption of the cycle.
The proposed fix : replace the comma with && in the condition.

Warning : V524 It is the odd that the body of the AllocateRuntimePages function is fully equivalent to the body of the AllocatePages.
File : mdepkg \ library \ smmmemoryallocationlib \ memoryallocationlib.c, 208 and on
Code :

 /** Allocates one or more 4KB pages of type EfiBootServicesData. Allocates the number of 4KB pages of type EfiBootServicesData and returns a pointer to the allocated buffer. The buffer returned is aligned on a 4KB boundary. If Pages is 0, then NULL is returned. If there is not enough memory remaining to satisfy the request, then NULL is returned. @param Pages The number of 4 KB pages to allocate. @return A pointer to the allocated buffer or NULL if allocation fails. **/ VOID * EFIAPI AllocatePages ( IN UINTN Pages ) { return InternalAllocatePages (EfiRuntimeServicesData, Pages); } 

Comment : The code contradicts the comment and allocates EfiRuntimeServicesData memory instead of the implied EfiBootServicesData. The difference between them is that the memory of the second type will be automatically released at the end of the BDS phase, and the memory of the first type must be freed by explicitly calling FreeMem before the end of the aforementioned phase, otherwise it will remain inaccessible for the OS forever. As a result, a seemingly small error can lead to completely incomprehensible memory leaks and the fragmentation of the OS accessible address space.
The proposed fix : replacing the memory type with EfiBootServicesData in all non-Runtime functions of this file.

Warning : It is the odd that the body of the OhciSetLsThreshold 'function is fully equivalent to the body of the OhciSetPeriodicStart' function.
File : quarksocpkg \ quarksouthcluster \ usb \ ohci \ pei \ ohcireg.c, 1010, 1015 and quarksocpkg \ quarksouthcluster \ usb \ ohci \ dxe \ ohcireg.c, 1010, 1040
Code :

 EFI_STATUS OhciSetLsThreshold ( IN USB_OHCI_HC_DEV *Ohc, IN UINT32 Value ) { EFI_STATUS Status; Status = OhciSetOperationalReg (Ohc->PciIo, HC_PERIODIC_START, &Value); return Status; } 

Comment : another copy-paste victim, this time the HC_PERIODIC_START bit is set and checked instead of HC_LS_THREASHOLD.
Suggested fix : replace the bit with the correct one.

Warning : V528 It is odd that the pointer to 'char' type is compared with the '\ 0' value. Probably meant: * MatchLang! = '\ 0'.
File : quarkplatformpkg \ platform \ dxe \ smbiosmiscdxe \ miscnumberofinstallablelanguagesfunction.c, 95
Code :

 for (MatchLang = Languages, (*Offset) = 0; MatchLang != '\0'; (*Offset)++) { // // Seek to the end of current match language. // for (EndMatchLang = MatchLang; *EndMatchLang != '\0' && *EndMatchLang != ';'; EndMatchLang++); if ((EndMatchLang == MatchLang + CompareLength) && AsciiStrnCmp(MatchLang, BestLanguage, CompareLength) == 0) { // // Find the current best Language in the supported languages // break; } // // best language match be in the supported language. // ASSERT (*EndMatchLang == ';'); MatchLang = EndMatchLang + 1; } 

Comment : as a result of an error with checking the un-denoted pointer, the cycle became infinite, and only the fact that there was a break saves from looping.
Proposed fix : add missing dereference.

Warning : V535 The variable 'Index' is being used for this loop.
File : mdemodulepkg \ core \ pismmcore \ dispatcher.c, 1233, 1269, 1316
Code :

 for (Index = 0; Index < HandleCount; Index++) { FvHandle = HandleBuffer[Index]; ... for (Index = 0; Index < sizeof (mSmmFileTypes)/sizeof (EFI_FV_FILETYPE); Index++) { ... } ... for (Index = 0; Index < AprioriEntryCount; Index++) { ... } } 

Comment : sample code that works only due to lucky coincidence. HandleCount in the outer loop is almost always equal to 1, in the mSmmFileTypes array, there is also exactly one element at the moment, and AprioriEntryCount is not less than 1, so the outer loop ends. It is clear that a completely different behavior was meant, but copy-paste has its own opinion on this.
Suggested fix : introduce independent counters for each cycle.

Warning : V547 Expression '(0)> (1 - Dtr1.field.tCMD)' is always false. Unsigned type value is never <0.
File : quarksocpkg \ quarknorthcluster \ memoryinit \ pei \ meminit.c, 483, 487
Code :

 #define MMAX(a,b) ((a)>(b)?(a):(b)) ... #pragma pack(1) typedef union { uint32_t raw; struct { ... uint32_t tCMD :2; /**< bit [5:4] Command transport duration */ ... } field; } RegDTR1; /**< DRAM Timing Register 1 */ #pragma pack() ... if (mrc_params->ddr_speed == DDRFREQ_800) { Dtr3.field.tXP = MMAX(0, 1 - Dtr1.field.tCMD); } else { Dtr3.field.tXP = MMAX(0, 2 - Dtr1.field.tCMD); } 

Comment : the simplest macro and automatic type casting hit back. Since tCMD is a bit field of the uint32_t type, then in condition 0> 1 - tCMD both parts will be automatically converted to uint32_t, which will make it false for any tCMD value.
The proposed fix :

 if (mrc_params->ddr_speed == DDRFREQ_800) { Dtr3.field.tXP = Dtr1.field.tCMD > 0 ? 0 : 1 ; } else { Dtr3.field.tXP = Dtr1.field.tCMD > 1 ? 0 : 2 - Dtr1.field.tCMD; } 

Warning : V547 Expression 'PollCount> = ((1000 * 1000) / 25)' is always false. The value range of unsigned char type: [0, 255].
File : quarksocpkg \ quarksouthcluster \ i2c \ common \ i2ccommon.c, 297
Code :

 UINT8 PollCount; ... do { Data = *((volatile UINT32 *) (UINTN)(Addr)); if ((Data & I2C_REG_RAW_INTR_STAT_TX_ABRT) != 0) { Status = EFI_ABORTED; break; } if ((Data & I2C_REG_RAW_INTR_STAT_TX_OVER) != 0) { Status = EFI_DEVICE_ERROR; break; } if ((Data & I2C_REG_RAW_INTR_STAT_RX_OVER) != 0) { Status = EFI_DEVICE_ERROR; break; } if ((Data & I2C_REG_RAW_INTR_STAT_STOP_DET) != 0) { Status = EFI_SUCCESS; break; } MicroSecondDelay(TI2C_POLL); PollCount++; if (PollCount >= MAX_STOP_DET_POLL_COUNT) { Status = EFI_TIMEOUT; break; } } while (TRUE); 

Comment : The MAX_STOP_DET_POLL_COUNT macro expands to 40,000, and PollCount cannot be greater than 255, resulting in an infinite loop.
Suggested fix : change the PollCount type to UINT32.

Warning : V560 A part of conditional expression is always true: (0x00040000).
File : quarksocpkg \ quarknorthcluster \ library \ intelqnclib \ pciexpress.c, 370
Code :

 if ((QNCMmPci32 (0, Bus, Device, Function, (CapOffset + PCIE_LINK_CAP_OFFSET)) && B_QNC_PCIE_LCAP_CPM) != B_QNC_PCIE_LCAP_CPM) { return; } 

Comment : instead of the bitwise AND, the logical expression crept into the expression, as a result, the check became useless.
The proposed fix :

 if ((QNCMmPci32 (0, Bus, Device, Function, (CapOffset + PCIE_LINK_CAP_OFFSET)) & B_QNC_PCIE_LCAP_CPM) != B_QNC_PCIE_LCAP_CPM) { return; } 

Warning : V560 A part of conditional expression is always true: 0x0FFFFF000.
File : quarksocpkg \ quarknorthcluster \ library \ intelqnclib \ intelqnclib.c, 378
Code :

 return QNCPortRead(QUARK_NC_HOST_BRIDGE_SB_PORT_ID, QUARK_NC_HOST_BRIDGE_HMBOUND_REG) && HMBOUND_MASK; 

Comment : the same as in the previous case, only worse, this time the return value suffered
The proposed fix :

 return QNCPortRead(QUARK_NC_HOST_BRIDGE_SB_PORT_ID, QUARK_NC_HOST_BRIDGE_HMBOUND_REG) & HMBOUND_MASK; 

Warning : V560 A part of conditional expression is always true: 0x00400.
File : quarksocpkg \ quarksouthcluster \ usb \ ohci \ pei \ ohcireg.c, 1065 and quarksocpkg \ quarksouthcluster \ usb \ ohci \ dxe \ ohcireg.c, 1070
Code :

 if (Field & (RH_DEV_REMOVABLE || RH_PORT_PWR_CTRL_MASK)) { 

Comment : this time there was no luck with a beat OR.
The proposed fix :

 if (Field & (RH_DEV_REMOVABLE | RH_PORT_PWR_CTRL_MASK)) { 

Warning : V649 There are two "if" statements with identical conditional expressions. The first 'if' statement contains function return. This means that the statement is senseless.
File : s: \ quarkplatformpkg \ platform \ dxe \ smbiosmiscdxe \ miscsystemmanufacturerfunction.c, 155
Code :

  SerialNumStrLen = StrLen(SerialNumberPtr); if (SerialNumStrLen > SMBIOS_STRING_MAX_LENGTH) { return EFI_UNSUPPORTED; } ... SKUNumStrLen = StrLen(SKUNumberPtr); if (SerialNumStrLen > SMBIOS_STRING_MAX_LENGTH) { return EFI_UNSUPPORTED; } ... FamilyStrLen = StrLen(FamilyPtr); if (SerialNumStrLen > SMBIOS_STRING_MAX_LENGTH) { return EFI_UNSUPPORTED; } 

Comment : copy-paste failed again, damn it. We receive one value, we check another, we have not clear behavior of function.
The proposed fix :

  SerialNumStrLen = StrLen(SerialNumberPtr); if (SerialNumStrLen > SMBIOS_STRING_MAX_LENGTH) { return EFI_UNSUPPORTED; } ... SKUNumStrLen = StrLen(SKUNumberPtr); if (SKUNumStrLen > SMBIOS_STRING_MAX_LENGTH) { return EFI_UNSUPPORTED; } ... FamilyStrLen = StrLen(FamilyPtr); if (FamilyStrLen > SMBIOS_STRING_MAX_LENGTH) { return EFI_UNSUPPORTED; } 

Conclusion

I tried to select only obviously erroneous code, not paying attention to problems like the dangerous use of shift operators, reassignment of the same variable, conversion of literals and integer type variables into pointers, etc., that speak about low code quality rather than there is an error in it, but even so the list was quite impressive. Projects for desktop motherboards are on average 4-5 times larger (about 4000 compiler calls on the counter in the Monitoring window instead of 800 in our case), and there are the same typical errors.

Unfortunately, Intel has not yet posted the source code Quark_EDKII on GitHub, so I haven’t sent pull requests for this project to anyone yet. Perhaps izard is aware of who exactly is responsible for the project in Intel and who should be sent a link in order for the bugs to be fixed.

Thanks to the readers for their attention, and to the developers of PVS-Studio for the useful program and the test key.