К сожалению, о формальной верификации или использовании 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 — за полезную программу и тестовый ключ.
Комментарии (22)
xvilka
26.05.2015 04:59Сравнить бы с такими же результатами для coreboot.
Вот данные из Coverity (PVS Studio у меня нет):
Lines of code analyzed: 2,140,306
Defects density: 0.18
Total defects: 1070
Outstanding: 392
P.S. Пару лет назад прогонял код edk2 через Klocwork. Результатов было почти на 50 страниц. К сожалению тот отчет не сохранился.CodeRush Автор
26.05.2015 20:27+1И сравню, только сборку под Windows заведу, ключ у меня до конца недели.
Andrey2008
26.05.2015 10:38+1Николай, большое спасибо за статью. Для читателей хочу отметить, что статья написана не на заказ, а исключительно по доброй воле. И нам было очень приятно и неожиданно увидеть её на Хабре. Ещё раз спасибо.
Milfgard
26.05.2015 10:57+1Аккуратнее, вы только что явно обозначили, что всё остальное у вас — хардкорная заказуха.
Andrey2008
26.05.2015 11:11+7Ну так практически все остальные наши и есть. :) Только мы и пишем. Написанных не нами статей про PVS-Studio на Хабре, к сожалению, всего несколько штук. Вот поэтому и захотел подчеркнуть, что это не наша статья. Иначе многие этого просто не заметят на общем фоне. :)
vt4a2h
26.05.2015 11:54+4Если я не ошибаюсь, то большая часть статей о PVS написана разработчиками PVS. И их интересно читать, в отличие от многих других рекламных статей. Так что тут всё совсем прозрачно и видно кто и что написал.
dmitrmax
26.05.2015 12:39Справедливости ради замечу, что признаваться не в чем: habrahabr.ru/exchange/order/7 — всё открыто.
CodeRush Автор
26.05.2015 20:28+1Пожалуйста, спасибо вам за тестовый ключ и отличную программу, получилось отловить несколько неприятных багов в своих проектах.
izard
26.05.2015 12:43+2Спасибо, перешлю ссылку на google translate статьи человеку, который должен знать разработчиков quark EDK2
Andrey2008
26.05.2015 12:49Спасибо. Кстати, чуть позже мы сделаем перевод для нашего сайта.
Andrey2008
29.05.2015 00:15+2А вот и перевод: Analyzing the Source Code of UEFI for Intel Galileo by PVS-Studio.
xvilka
26.05.2015 13:29Еще поною: им бы уже, наконец, перейти на C99-совместимый код, благо его поддержка в компиляторе от VS2015 существенно улучшена. Ну и хотя бы некоторые из правил MISRA C не повредит соблюдать.
CodeRush Автор
26.05.2015 20:30Если только в EDK3, и то я не уверен, нет спроса пока ни на C99, ни на более качественный код, зато новых фич в UEFI 2.5 и PI 1.4 снова базилион.
dmitrmax
Сколько по времени проводился анализ? И какой примерно объем кода у этого проекта? На выходных пробовал демо-версию данного анализатора, и на довольно простом проекте он работал около 3 часов и при этом по таймауту зафейлил анализ нескольких файлов (правда делалось это на виртуалке с одним ядром).
CodeRush Автор
Проект небольшой, всего 80 Мб кода, на анализ ушло ~5 минут работы анализатора (VmWare 10, 4 ядра, 8 гб памяти) плюс около часа я потратил на разгребание предупреждений и поиск ошибок.
На работе анализировал проекты на 400 — 600 Мб кода, на анализ тратится 10-15 минут, 4 ядра, 8 гб памяти в реальной ОС.
dmitrmax
Очень странно. Может быть, конечно, дело в том, что в моём случае были плюсы, которые тянули boost.
Andrey2008
Да, boost может весьма замедлять скорость анализа.
P.S. Советы по повышению скорости работы PVS-Studio.
dmitrmax
Но boost бы тоже не мешало проверить )
Andrey2008
PVS-Studio наконец то добрался до Boost.
Andrey2008
И вообще для желающих помедитировать над чужими ошибками: Обновляемый список статей, в которых мы рассказываем об ошибках, найденных с помощью PVS-Studio в открытых проектах.