to the top
close form
Для получения триального ключа
заполните форму ниже
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Мне интересно попробовать плагин на:
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
check circle
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Проверяем открытый исходный код UEFI дл…

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

25 Май 2015

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

0326_UEFI_for_Intel_Galileo_ru/image1.png

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

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

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

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

Анализатор лежит на сайте его разработчика, после установки пишем ему письмо с просьбой предоставить на короткое время ключ, позволяющий видеть не только предупреждения первого уровня (а именно так сделано в ознакомительных версиях), а все вообще — в нашем случае лучше перебдеть, чем недобдеть.

Код прошивки является частью Quark BSP и основан на EDK2010.SR1, как и все современные реализации UEFI, за исключением продукции компании Apple.

EDK имеет собственную сборочную систему, поэтому для проверки собираемого ей кода воспользуемся PVS-Studio Standalone. Как подготовить пакет Quark_EDKII к сборке — читайте в этом документе, подробнее останавливаться на ней здесь я не буду.

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

Запускаем 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

Код:

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 и далее

Код:

/** 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

Код:

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

Код:

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

Код:

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

Код:

#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.

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

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

Код:

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

Код:

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

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

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

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

Код:

return QNCPortRead(QUARK_NC_HOST_BRIDGE_SB_PORT_ID, 
  QUARK_NC_HOST_BRIDGE_HMBOUND_REG) && HMBOUND_MASK;

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

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

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

Код:

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

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

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

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

Код:

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

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

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

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 в курсе, кто именно в Intel отвечает за проект и в кого следует бросить ссылкой, чтобы баги когда-нибудь исправили.

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

Примечание. Оригинал статьи на русском языке впервые был опубликован на сайте Хабрахабр. Статья и её перевод публикуются на нашем сайте с согласия автора.

Популярные статьи по теме
Под капотом SAST: как инструменты анализа кода ищут дефекты безопасности

Дата: 26 Янв 2023

Автор: Сергей Васильев

Сегодня речь о том, как SAST-решения ищут дефекты безопасности. Расскажу, как разные подходы к поиску потенциальных уязвимостей дополняют друг друга, зачем нужен каждый из них и как теория ложится на…
Ложные представления программистов о неопределённом поведении

Дата: 17 Янв 2023

Автор: Гость

Неопределённое поведение (UB) – непростая концепция в языках программирования и компиляторах. Я слышал много заблуждений в том, что гарантирует компилятор при наличии UB. Это печально, но неудивитель…
Топ-10 ошибок в C++ проектах за 2022 год

Дата: 29 Дек 2022

Автор: Владислав Столяров

Дело идёт к Новому году, а значит, самое время традиционно вспомнить десять самых интересных срабатываний, которые нашёл PVS-Studio в 2022 году.
PVS-Studio и RPCS3: лучшие предупреждения в один клик

Дата: 12 Дек 2022

Автор: Александр Куренев

Best Warnings — режим анализатора, оставляющий в окне вывода 10 лучших предупреждений. Мы предлагаем вам ознакомиться с обновлённым режимом Best Warnings на примере проверки проекта RPCS3.
Holy C++

Дата: 23 Ноя 2022

Автор: Гость

В этой статье постараюсь затронуть все вещи, которые можно без зазрения совести выкинуть из С++, не потеряв ничего (кроме боли), уменьшить стандарт, нагрузку на создателей компиляторов, студентов, из…

Комментарии (0)

Следующие комментарии next comments
close comment form
Unicorn with delicious cookie
Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо