Checking open source UEFI for Intel Galileo using PVS-Studio

Firmware development, even if it is conducted not in assembler for exotic architectures, but in C for i386 / amd64, is not a simple matter, and the cost of an error can be extremely high, even if the target hardware platform fails, so use various error prevention techniques in the very early stages of development - a necessity.

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

I humbly ask for the results of the check under cat.

Environment setting

As Captain Evidence tells me, to analyze any code, we need an analyzer, the code itself and the assembly environment for it.

The analyzer is on the site of its developer , after installation, we write a letter to him asking him to provide a key for a short time, allowing him to see not only first-level warnings (which is exactly what was done in the trial versions), but everything in general — in our case, it’s better to overdo than not overlook.

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

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

Analyzer start

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

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 63th of the third (with default settings, that is, when only General Analysis is selected). Sort them by warning number 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 : misuse of the comma operator in a condition. Let me remind you that this operator has the lowest priority, 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 it is fulfilled, does not affect the continuation or interruption of the cycle.
Suggested fix : replace comma with && in the condition.

Warning : V524 It is odd that the body of 'AllocateRuntimePages' function is fully equivalent to the body of 'AllocatePages' function.
File : mdepkg \ library \ smmmemoryallocationlib \ memoryallocationlib.c, 208 onwards
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 memory like EfiRuntimeServicesData instead of the implied EfiBootServicesData. The difference between the two is that the memory of the second type will be automatically freed at the end of the BDS phase , and the memory of the first type must be freed by an explicit call to FreeMem before the end of the above phase, otherwise it will remain inaccessible to the OS forever. As a result, a seemingly small error can lead to completely incomprehensible memory leaks and fragmentation of the address space available to the OS.
Suggested fix : replacing the memory type with EfiBootServicesData in all non-Runtime functions of this file.

Warning: V524 It is odd that the body of 'OhciSetLsThreshold' function is fully equivalent to the body of '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 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 an unreferenced pointer, the cycle became infinite, and only that inside was found to save from looping.
Suggested fix : add missing dereferencing.

Warning : V535 The variable 'Index' is being used for this loop and for the outer 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 : an example of code that works only thanks to a successful combination of circumstances. 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 implied, but copy-paste has its own opinion on this matter.
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 strike back. Because Since tCMD is a bit field of the uint32_t type, in the condition 0> 1 - tCMD both parts will be automatically cast to uint32_t, which will make it false for any tCMD value.
Suggested 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 macro MAX_STOP_DET_POLL_COUNT expands at 40,000, and PollCount cannot be greater than 255, as a result of which an infinite loop is possible.
Suggested fix : change 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 bitwise AND, a logical crept into the expression, as a result, the check became useless.
Suggested 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 was affected.
Suggested 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 no luck bitwise OR.
Suggested 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 second 'if' 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 : the copy-paste failed again, be it wrong. We get one value, check another, we have incomprehensible behavior of the function.
Suggested 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, re-assigning the same variable, converting literals and integer variables to pointers, etc., talking more about low quality of the code than about there is an error in it, but even so the list turned out to be pretty impressive. Projects for desktop motherboards are on average 4-5 times larger than this (about 4000 compiler calls by counter in the Monitoring window instead of 800 in our case), and the same typical errors are encountered there.

Unfortunately, Intel still has not posted the source code of Quark_EDKII on GitHub, so I have not sent pull requests for this project to anyone yet. Maybe izardI’m aware who exactly at Intel is responsible for the project and who should throw a link to, so that bugs will be fixed sometime.

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