# Проверяем открытый исходный код UEFI для Intel Galileo при помощи PVS\-Studio

Разработка прошивок, даже если она ведется не на ассемблере для экзотических архитектур, а на C для i386/amd64 — дело весьма непростое, да и цена ошибки может быть крайне высокой, вплоть до выхода целевой аппаратной платформы из строя, поэтому использование различных техник предотвращения ошибок на самых ранних этапах разработки — необходимость\.  

![0326_UEFI_for_Intel_Galileo_ru/image1.png](https://import.viva64.com/docx/blog/0326_UEFI_for_Intel_Galileo_ru/image1.png)

К сожалению, о формальной верификации или использовании MISRA C в случае UEFI\-прошивок остается только мечтать \(с другой стороны, мало кто хочет тратить на разработку прошивки пару лет и 50% бюджета проекта\), поэтому сегодня поговорим о статическом анализе, а точнее — о популярном на Хабре статическом анализаторе PVS\-Studio, которым попытаемся найти ошибки в открытом коде UEFI для Intel Galileo\.  



За результатами проверки покорнейше прошу под кат\.

## Настройка окружения

Как мне подсказывает капитан Очевидность, для анализа какого\-либо кода нам понадобится анализатор, сам код и сборочная среда для него\.

Анализатор лежит [на сайте его разработчика](https://pvs-studio.ru/ru/pvs-studio/download/), после установки пишем ему письмо с просьбой предоставить на короткое время ключ, позволяющий видеть не только предупреждения первого уровня \(а именно так сделано в ознакомительных версиях\), а все вообще — в нашем случае лучше перебдеть, чем недобдеть\.

Код прошивки является частью [Quark BSP](https://downloadcenter.intel.com/download/26418/Intel-Galileo-Board-Support-Package) и основан на EDK2010\.SR1, как и все современные реализации UEFI, за исключением продукции компании Apple\. 

EDK имеет собственную сборочную систему, поэтому для проверки собираемого ей кода воспользуемся PVS\-Studio Standalone\. Как подготовить пакет Quark\_EDKII к сборке — [читайте в этом документе](https://downloadmirror.intel.com/26418/eng/Quark_BSP_BuildandSWUserGuide_329687_007.pdf), подробнее останавливаться на ней здесь я не буду\.

## Запуск анализатора

Запускаем PVS\-Studio Standalone, нажимаем кнопку Analyze your files\.\.\., открывается окно Compiler Monitoring, в котором нажимаем единственную кнопку Start Monitoring\. Теперь открываем консоль в папке с Quark\_EDKII и выполняем команду _quarkbuild \-r32 S QuarkPlatform_ для сборки релизной версии прошивки\. Ждем окончания сборки, в окне Compiler Monitoring наблюдаем за ростом числа обнаруженных вызовов компилятора, после окончания сборки нажимаем кнопку Stop Monitoring и ждем окончания анализа\. 

## Результаты анализа

Для текущей версии Quark\_EDKII\_v1\.1\.0 анализатор показывает 96 предупреждений первого уровня, 100 — второго и 63\-третьего \(с настройками по умолчанию, т\.е\. при выборе только General Analysis\)\. Отсортируем их по номеру предупреждения и приступим к поиску ошибок\.

**Предупреждение**: V521 Such expressions using the ',' operator are dangerous\. Make sure the expression is correct\. 

**Файл**: quarkplatformpkg\\pci\\dxe\\pcihostbridge\\pcihostbridge\.c, 181, 272

**Код:**

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

**Комментарий**: неправильное использование оператора "запятая" в условии\. Напомню, что этот оператор имеет минимальный приоритет, вычисляет значения обоих операндов, но сам принимает значение правого\. В данном случае условие полностью аналогично IioResourceMapEntry < MaxIIO, а проверка TotalRootBridgeFound < HostBridge\-\>RootBridgeCount хоть и выполняется, но на продолжение или прерывание цикла не влияет\.

**Предлагаемый фикс:** заменить запятую на && в условии\.

**Предупреждение**: V524 It is odd that the body of 'AllocateRuntimePages' function is fully equivalent to the body of 'AllocatePages' function\. 

**Файл**: mdepkg\\library\\smmmemoryallocationlib\\memoryallocationlib\.c, 208 и далее 

**Код:**

```cpp
/** 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); 
}
```

**Комментарий**: код противоречит комментарию и выделяет память типа EfiRuntimeServicesData вместо подразумеваемого EfiBootServicesData\. Разница между ними в том, что память второго типа будет автоматически освобождена по окончанию фазы BDS, а память первого типа должна быть освобождена явным вызовом FreeMem до окончания вышеупомянутой фазы, иначе она останется недоступной для ОС навсегда\. В итоге мелкая вроде бы ошибка может приводить к совершенно непонятным утечкам памяти и фрагментации доступного ОС адресного пространства\.

**Предлагаемый фикс:** замена типа памяти на EfiBootServicesData во всех не\-Runtime функциях этого файла\.

**Предупреждение**: V524 It is odd that the body of 'OhciSetLsThreshold' function is fully equivalent to the body of 'OhciSetPeriodicStart' function\. 

**Файл**: quarksocpkg\\quarksouthcluster\\usb\\ohci\\pei\\ohcireg\.c, 1010, 1015 и quarksocpkg\\quarksouthcluster\\usb\\ohci\\dxe\\ohcireg\.c, 1010, 1040

**Код:**

```cpp
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; 
}
```

**Комментарий:** еще одна жертва копипасты, на этот раз вместо HC\_LS\_THREASHOLD устанавливается и проверяется бит HC\_PERIODIC\_START\.

**Предлагаемый фикс:** заменить бит на правильный\.

**Предупреждение**: V528 It is odd that pointer to 'char' type is compared with the '\\0' value\. Probably meant: \*MatchLang \!\= '\\0'\.  

**Файл**: quarkplatformpkg\\platform\\dxe\\smbiosmiscdxe\\miscnumberofinstallablelanguagesfunction\.c, 95

**Код:**

```cpp
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; 
}
```

**Комментарий**: в результате ошибки с проверкой неразыменованного указателя цикл стал бесконечным, и от зацикливания спасает только то, что внутри нашелся break\.  

**Предлагаемый фикс**: добавить недостающее разыменование\.

**Предупреждение**: V535 The variable 'Index' is being used for this loop and for the outer loop\.

**Файл**: mdemodulepkg\\core\\pismmcore\\dispatcher\.c, 1233, 1269, 1316

**Код**:

```cpp
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++) 
  { 
    .... 
  } 
}
```

**Комментарий**: пример кода, который работает лишь благодаря удачному стечению обстоятельств\. HandleCount во внешнем цикле почти всегда равен 1, в массиве mSmmFileTypes тоже на данный момент ровно один элемент, а AprioriEntryCount не меньше 1, поэтому внешний цикл завершается\. Ясно, что подразумевалось совсем иное поведение, но у копипаста свое мнение на это счет\.

**Предлагаемый фикс**: ввести независимые счетчики для каждого цикла\.

**Предупреждение**: V547 Expression '\(0\) \> \(1 — Dtr1\.field\.tCMD\)' is always false\. Unsigned type value is never < 0\.

**Файл**: quarksocpkg\\quarknorthcluster\\memoryinit\\pei\\meminit\.c, 483, 487

**Код**:

```cpp
#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); 
}
```

**Комментарий**: простейший макрос и автоматическое приведение типов наносят ответный удар\. Т\.к\. tCMD — это битовое поле типа uint32\_t, то в условии 0 \> 1 — tCMD обе части будут автоматически приведены к uint32\_t, отчего оно станет ложным при любом значении tCMD\.

**Предлагаемый фикс**:

```cpp
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; 
}
```

**Предупреждение**: V547 Expression 'PollCount \>\= \(\(1000 \* 1000\) / 25\)' is always false\. The value range of unsigned char type: \[0, 255\]\.

**Файл**: quarksocpkg\\quarksouthcluster\\i2c\\common\\i2ccommon\.c, 297

**Код**:

```cpp
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);
```

**Комментарий**: макрос MAX\_STOP\_DET\_POLL\_COUNT разворачивается в 40000, а PollCount не может быть больше 255, в результате возможен бесконечный цикл\.

**Предлагаемый фикс**: сменить тип PollCount на UINT32\.

**Предупреждение**: V560 A part of conditional expression is always true: \(0x00040000\)\.

**Файл**: quarksocpkg\\quarknorthcluster\\library\\intelqnclib\\pciexpress\.c, 370

**Код**:

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

**Комментарий**: вместо побитового AND в выражение закралось логическое, в результате проверка стала бесполезной\.

**Предлагаемый фикс**:

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

**Предупреждение**: V560 A part of conditional expression is always true: 0x0FFFFF000\.

**Файл**: quarksocpkg\\quarknorthcluster\\library\\intelqnclib\\intelqnclib\.c, 378

**Код**:

```cpp
return QNCPortRead(QUARK_NC_HOST_BRIDGE_SB_PORT_ID, 
  QUARK_NC_HOST_BRIDGE_HMBOUND_REG) && HMBOUND_MASK;
```

**Комментарий**: то же самое, что и в предыдущем случае, только еще хуже, в этот раз пострадало возвращаемое значение

**Предлагаемый фикс**:

```cpp
return QNCPortRead(QUARK_NC_HOST_BRIDGE_SB_PORT_ID, 
  QUARK_NC_HOST_BRIDGE_HMBOUND_REG) & HMBOUND_MASK;
```

**Предупреждение**: V560 A part of conditional expression is always true: 0x00400\.

**Файл**: quarksocpkg\\quarksouthcluster\\usb\\ohci\\pei\\ohcireg\.c, 1065 и   quarksocpkg\\quarksouthcluster\\usb\\ohci\\dxe\\ohcireg\.c, 1070

**Код:**

```cpp
if (Field & (RH_DEV_REMOVABLE || RH_PORT_PWR_CTRL_MASK)) 
{
  ....
}
```

**Комментарий**: на этот раз не повезло побитовому OR\.

**Предлагаемый фикс**:

```cpp
if (Field & (RH_DEV_REMOVABLE | RH_PORT_PWR_CTRL_MASK)) 
{
  ....
}
```

**Предупреждение**: 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\. 

**Файл**: s:\\quarkplatformpkg\\platform\\dxe\\smbiosmiscdxe\\miscsystemmanufacturerfunction\.c, 155

**Код:**

```cpp
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; 
}
```

**Комментарий**: снова подвел копипаст, будь он неладен\. Получаем одно значение, проверяем другое, имеем непонятное поведение функции\.

**Предлагаемый фикс:**

```cpp
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; 
}
```

## Заключение

Я старался отобрать только явно ошибочный код, не обращая внимания на проблемы вроде опасного использования операторов сдвига, повторного присваивания одной и той же переменной, преобразования литералов и переменных целого типа в указатели и т\.п\., говорящие скорее о низком качестве кода, чем о наличии ошибки в нем, но даже так список получился довольно внушительным\. Проекты для десктопных материнских плат в среднем больше этого в 4\-5 раз \(около 4000 вызовов компилятора по счетчику в окне Monitoring вместо 800 в нашем случае\), и там встречаются те же типичные ошибки\.



К сожалению, Intel до сих пор не выложила исходный код Quark\_EDKII на GitHub, поэтому pull request'ы для этого проекта я пока никому не отправил\. Возможно,  [izard](https://habr.com/ru/users/izard/) в курсе, кто именно в Intel отвечает за проект и в кого следует бросить ссылкой, чтобы баги когда\-нибудь исправили\.



Спасибо читателям за внимание, а разработчикам PVS\-Studio — за полезную программу и тестовый ключ\.



_**Примечание\.**_ [Оригинал статьи](https://habr.com/ru/post/258721/) на русском языке впервые был опубликован на сайте Хабрахабр\. Статья и её перевод публикуются на нашем сайте с согласия автора\.