Nikolaj Schlej

May 25 2015

Tags:

#Cpp

Analyzing the Source Code of UEFI for Intel Galileo by PVS-Studio

May 25 2015

Author: Nikolaj Schlej

Environment setup
Running the analyzer
Analysis results
Conclusion

Firmware development, even when done not in assembler for exotic architectures but in plain C for i386/amd64, is a pretty tough job, where a single bug can cost too much - up to a major failure of the target hardware platform. So it is just vitally necessary to use various techniques to avoid errors at the earliest firmware development stages.

Unfortunately, we can only dream of formal verification or application of MISRA C in case of UEFI firmwares (on the other hand, no one feels like spending a couple of years and 50% of the project budget on firmware development), so today we will talk about static analysis - to be more exact, about the PVS-Studio static analyzer quite popular at Habrahabr. With its help, we will try to find whatever bugs we can in the open code of UEFI for Intel Galileo.

Welcome to read further to find out the analysis results.

Environment setup

As Captain Obvious reminds me, in order to carry out an analysis of some code, we'll need an analyzer, the code itself, and an appropriate build environment.

The analyzer can be downloaded from the developer's site. Once you've done it, email to the authors with a request for a temporary registration key to enable you to switch on and examine not only first-level warnings (it's the only level available in the demo version) but the other two as well. In our case, we are really better be safe than sorry.

The firmware's code is a part of Quark BSP and is based on EDK2010.SR1 just like all the other modern UEFI implementations except for Apple's products.

EDK has its own build system, so we'll use PVS-Studio Standalone version to check the code built there. To find out how to prepare the Quark_EDKII package for building, please see this document; I won't discuss the details of it here.

Running the analyzer

Run PVS-Studio Standalone and click on the Analyze your files... button. The Compiler Monitoring window will open where you need to click on the single button Start Monitoring. Now open the console in the Quark_EDKII folder and run the command quarkbuild -r32 S QuarkPlatform to build the release version of the firmware. Wait until the building process is over, watching the number of detected compiler calls growing in the Compiler Monitoring window. Once it is finished, click on the Stop Monitoring button and wait for the analysis process to finish.

Analysis results

For the current version Quark_EDKII_v1.1.0 , the analyzer outputs 96 first-level warnings, 100 second-level ones, and 63 third-level ones (under the default settings, i.e. with only the General Analysis rule-set enabled). Let's sort them by the warning number and start investigating the bugs.

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: The "comma" operator is used incorrectly in the condition. As you remember, this operator has the lowest precedence and calculates both of its operands but only takes itself the value of the right one. In this case, the condition is absolutely identical to IioResourceMapEntry < MaxIIO, while the check TotalRootBridgeFound < HostBridge->RootBridgeCount, despite being executed, doesn't in any way affect the loop continuation or termination.

Suggested fix: replace the comma in the condition with &&.

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 and further 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 memory of the EfiRuntimeServicesData type instead of the intended type EfiBootServicesData. The difference between the two is that in the latter case, memory will be automatically freed once the BDS phase is over, while in the former case, memory must be freed by explicitly calling FreeMem before the end of the BDS phase - otherwise it will forever remain inaccessible for the operating system. What we have as a result is a tiny bug which, though, may cause strange memory leaks and fragmentation of the address space available to the operating system.

Suggested fix: replace the used memory type with EfiBootServicesData in all the 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 victim of the copy-paste technique. This time, the HC_PERIODIC_START bit is set and checked instead of HC_LS_THREASHOLD.

Suggested fix: replace the inappropriate bit with the right 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: the bug with a check for a non-dereferenced pointer renders the loop infinite, and the only thing that saves the code from infinite looping is the presence of break inside it.

Suggested fix: add the missing pointer dereferencing operation.

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: this is an example of code that works well only thanks to a lucky coincidence. HandleCount in the external loop almost always equals 1; in the mSmmFileTypes array, there is also exactly one item at the moment; and AprioriEntryCount is not less than 1. Thanks to this, the external loop can terminate successfully. But it's obvious, of course, that the programmer intended quite a different behavior. Well, the copy-paste has a mind of its own.

Suggested fix: implement independent counters for each loop.

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: a simplest macro and automatic type conversion strike back. Since tCMD is a bit field of the uint32_t type, then in the 0 > 1 — tCMD condition, both parts will be automatically cast to uint32_t, which will make it evaluate to false regardless of tCMD's 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 MAX_STOP_DET_POLL_COUNT macro is expanded into 40000, while PollCount can't be larger than 255. The result is a potential infinite loop.

Suggested fix: replace the PollCount type with 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 a bitwise AND, a logical AND has slipped into the expression, rendering the check meaningless.

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 issue is the same as in the previous case, but it's even worse this time because it is the return value that has been 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, it was a 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: again this nasty copy-paste... Getting one value, checking another - the result is an odd function behavior.

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 was trying to pick only obviously incorrect code fragments, ignoring such issues as dangerous use of shift operations, value reassignment to one and the same variable, converting literals and integer variables to pointers, and so on, which usually indicate a poor code quality rather than the presence of bugs in it. But even that way, my list has turned out pretty lengthy. On average, projects for desktop motherboards are 4-5 times larger than that (about 4000 compiler calls, as opposed to 800 in our case, according to the counter in the Monitoring window), and there are the same typical bugs to be found there, too.

Unfortunately, Intel still hasn't uploaded Quark_EDKII's source code to GitHub, so I haven't sent the pull requests for this project to anyone yet. Perhaps izard knows who exactly at Intel is responsible for the project and whom to throw the link at to get the bugs finally fixed.

Thank you for reading, and thanks to PVS-Studio's developers for their wonderful program and test registration key they have granted to us.

Note. The article was originally published in Russian at the Habrahabr site. Translated and republished at our site by the author's permission.

#Cpp