Используя таинственный инструмент XXX-Xxxxxx, мы вновь нашли кучу ошибок в Linux Kernel. Для желающих словить от этих ошибок Facepalm прошу пожаловать под кат.
Мне вот просто интересно, с какой теперь аргументацией будет бан интересного материала. :)
Опечатки
Самая распространённая категория ошибок связана с обычными опечатками и Copy-Paste. Если вы читали наши статьи раньше, я думаю, вы уже в этом убедились. Они появляются в любых проектах на любых операционных системах на любых языках. В таких ошибках и раскрывается потенциал статического анализатора: другими инструментами найти их значительно сложнее. Посмотрим, как с ними обстоят дела в ядре Linux:
int dbg_check_nondata_nodes_order(....)
{
....
sa = container_of(cur, struct ubifs_scan_node, list);
sb = container_of(cur->next, struct ubifs_scan_node, list);
if (sa->type != UBIFS_INO_NODE && sa->type != UBIFS_DENT_NODE &&
sa->type != UBIFS_XENT_NODE) {
ubifs_err(c, "bad node type %d", sa->type);
ubifs_dump_node(c, sa->node);
return -EINVAL;
}
if (sa->type != UBIFS_INO_NODE && sa->type != UBIFS_DENT_NODE &&
sa->type != UBIFS_XENT_NODE) {
ubifs_err(c, "bad node type %d", sb->type);
ubifs_dump_node(c, sb->node);
return -EINVAL;
}
....
}
Анализатор жалуется на два одинаковых условия подряд: видимо, во втором забыли поменять sa на sb. Ну и кто после этого скажет, что в крутых проектах не копипастят?
static ssize_t write_file_spec_scan_ctl(struct file *file,
const char __user *user_buf,
size_t count, loff_t *ppos)
{
struct ath10k *ar = file->private_data;
char buf[32];
ssize_t len;
int res;
len = min(count, sizeof(buf) - 1);
if (copy_from_user(buf, user_buf, len))
return -EFAULT;
buf[len] = '\0';
mutex_lock(&ar->conf_mutex);
if (strncmp("trigger", buf, 7) == 0) {
....
} else if (strncmp("background", buf, 9) == 0) {
res = ath10k_spectral_scan_config(ar, SPECTRAL_BACKGROUND);
} else if (strncmp("manual", buf, 6) == 0) {
res = ath10k_spectral_scan_config(ar, SPECTRAL_MANUAL);
} else if (strncmp("disable", buf, 7) == 0) {
res = ath10k_spectral_scan_config(ar, SPECTRAL_DISABLED);
} else {
res = -EINVAL;
}
mutex_unlock(&ar->conf_mutex);
if (res < 0)
return res;
return count;
}
Классический вид ошибки: в функцию нужно передать два аргумента: указатель на строку и её длину. Часто, когда аргументом служит литерал, длину считать ленятся и пишут просто число.
Смотрите, в коде есть несколько подряд strncmp. В каждый из них передают литерал. И в strncmp(«background», buf, 9) длину рассчитали неверно: слово «background» состоит из 10, а не из 9 символов.
static void adpt_inquiry(adpt_hba* pHba)
{
....
memset(pHba->detail, 0, sizeof(pHba->detail));
memcpy(&(pHba->detail), "Vendor: Adaptec ", 16);
memcpy(&(pHba->detail[16]), " Model: ", 8);
memcpy(&(pHba->detail[24]), (u8*) &buf[16], 16);
memcpy(&(pHba->detail[40]), " FW: ", 4); // <=
memcpy(&(pHba->detail[44]), (u8*) &buf[32], 4);
pHba->detail[48] = '\0'; /* precautionary */
....
}
Ещё один пример. Длина строки " FW: " равна 5, а вовсе не 4 символам.
Как избавиться от такой ошибки? В C можно использовать макрос наподобие:
#define str_len(S) (sizeof(S) / sizeof((S)[0]))
Но использование таких макросов само по себе опасно: лучше конечно добавить compiler-specific проверок на то, что переданный аргумент действительно массив.
Для читателей, пишущих на C++, могу порекомендовать std::string_view, который наконец-то появился в C++17. Лучше не передавать в функцию строки парой указатель-длина. Но если нужно вручную посчитать размер массива (например, чтобы передать его в функцию memcpy), то можно использовать std::size(array) или его аналог: для литералов размер будет посчитан в compile-time.
Избегайте повторения кода и не ленитесь использовать средства языка (будь то макросы или шаблоны) для compile time вычислений!
static ssize_t lp8788_show_eoc_time(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct lp8788_charger *pchg = dev_get_drvdata(dev);
char *stime[] = { "400ms", "5min", "10min", "15min",
"20min", "25min", "30min" "No timeout" };
....
}
Как известно, два записанных подряд литерала конкатенируются. Это позволяет их удобно использовать, например, в макросах. Опасность возникает, когда мы пишем массив из таких литералов: можно пропустить запятую и получить неожиданный результат.
В данном случае «слипнутся» два последних литерала и получится «30minNo timeout». Это двойная ошибка. Во-первых, текст неправильный, во-вторых в массиве будет не хватать одного элемента, что может привести к выходу за границу массива.
Советую использовать другой способ форматирования, в нём такая ошибка станет заметной:
char *stime[] = {
"400ms"
, "5min"
, "10min"
, "15min"
, "20min"
, "25min"
, "30min"
"No timeout"
};
Подробнее о таком способе табличного оформления кода предлагаю познакомиться в главой N13 из небольшой бесплатной книги.
const struct ahc_pci_identity *
ahc_find_pci_device(ahc_dev_softc_t pci)
{
....
if (ahc_get_pci_function(pci) > 0
&& ahc_9005_subdevinfo_valid(device, vendor, // <=
subdevice, subvendor)
&& SUBID_9005_MFUNCENB(subdevice) == 0)
return (NULL);
....
}
Иногда бывает сложно понять, на что ругается анализатор. Кстати, такое часто бывает: человек не понял, что ему написал анализатор, отправил нам отчёт с «ложным срабатыванием», а там на самом деле ошибка. Вот и мне здесь показалось, что это ложное срабатывание: функция определена немного выше по коду и там все параметры на своих местах. Вот как она выглядит:
static int
ahc_9005_subdevinfo_valid(uint16_t device, uint16_t vendor,
uint16_t subdevice, uint16_t subvendor)
{
....
}
В чём же дело? Оказывается, что ещё выше есть объявление этой функции и вот там-то эти аргументы и перепутаны. По факту ничего страшного в логике программы нет, но лучше всё-таки поправить, дабы никого не смущать и не сбивать с толку.
static int ahc_9005_subdevinfo_valid(uint16_t vendor, uint16_t device,
uint16_t subvendor, uint16_t subdevice);
Но что самое забавное, ошибка здесь уже была: параметры действительно были перепутаны, просто забыли поправить объявление. Хорошо, что анализатор тоже нашёл это место.
static int del_pmksa(struct wiphy *wiphy,
struct net_device *netdev,
struct cfg80211_pmksa *pmksa)
{
....
for (; i < (priv->pmkid_list.numpmkid - 1); i++) {
memcpy(priv->pmkid_list.pmkidlist[i].bssid,
priv->pmkid_list.pmkidlist[i + 1].bssid,
ETH_ALEN);
memcpy(priv->pmkid_list.pmkidlist[i].pmkid,
priv->pmkid_list.pmkidlist[i].pmkid,
PMKID_LEN);
}
....
}
В последнем memcpy совпадают указатели. Возможно, хотели написать по аналогии с предыдущим выражением:
memcpy(priv->pmkid_list.pmkidlist[i].pmkid,
priv->pmkid_list.pmkidlist[i + 1].pmkid,
PMKID_LEN);
Неиспользуемые переменные
static int mac_ioctl(struct net_device *ndev,
struct ifreq *req,
int cmd)
{
u8 *buff = NULL;
s8 rssi;
u32 size = 0, length = 0;
struct wilc_vif *vif;
s32 ret = 0;
struct wilc *wilc;
vif = netdev_priv(ndev);
wilc = vif->wilc;
if (!wilc->initialized)
return 0;
switch (cmd) {
case SIOCSIWPRIV:
{
struct iwreq *wrq = (struct iwreq *)req;
size = wrq->u.data.length;
if (size && wrq->u.data.pointer) {
buff = memdup_user(wrq->u.data.pointer,
wrq->u.data.length);
if (IS_ERR(buff))
return PTR_ERR(buff);
if (strncasecmp(buff, "RSSI", length) == 0) { // <=
....
}
}
}
....
}
done:
kfree(buff);
return ret;
}
В функцию strncasecmp в качестве аргумента длины передали 0. В коде нет места, где бы изменялась переменная length, поэтому её значение останется нулём. Наверное, нужно было использовать size.
static
unsigned short
SiS_GetModeID(int VGAEngine, unsigned int VBFlags,
int HDisplay, int VDisplay,
int Depth, bool FSTN,
int LCDwidth, int LCDheight)
{
unsigned short ModeIndex = 0;
switch(HDisplay)
{
case 320:
if(VDisplay == 200) ModeIndex = ModeIndex_320x200[Depth];
else if(VDisplay == 240) {
if((VBFlags & CRT2_LCD) && (FSTN))
ModeIndex = ModeIndex_320x240_FSTN[Depth];
else
ModeIndex = ModeIndex_320x240[Depth];
}
break;
case 400:
if((!(VBFlags & CRT1_LCDA)) ||
((LCDwidth >= 800) && (LCDwidth >= 600))) { // <=
if(VDisplay == 300) ModeIndex = ModeIndex_400x300[Depth];
}
break;
case 512:
if((!(VBFlags & CRT1_LCDA)) ||
((LCDwidth >= 1024) && (LCDwidth >= 768))) { // <=
if(VDisplay == 384) ModeIndex = ModeIndex_512x384[Depth];
}
break;
....
}
return ModeIndex;
}
Не всегда неиспользуемые в функции параметры - это ошибка. В достаточно старых API возникают ситуации, когда параметр стал не нужен и его перезаписывают или просто не используют. Но присмотритесь внимательнее к этому фрагменту: здесь забыли сравнить высоту. Вместо этого появились сравнения вида '(A > 5) && (A > 3)', которые сами по себе избыточны.
Путаница в приоритетах операций
static int nvme_pr_preempt(struct block_device *bdev,
u64 old, u64 new,
enum pr_type type, bool abort)
{
u32 cdw10 = nvme_pr_type(type) << 8 | abort ? 2 : 1;
return nvme_pr_command(bdev, cdw10, old, new,
nvme_cmd_resv_acquire);
}
Тернарный оператор в C - это очень опасный оператор. Дело в том, что у него низкий приоритет, и в сложных выражениях легко запутаться и получить совершенно иной порядок вычислений. Поэтому лучше, когда сомневаешься, использовать скобки.
Подозрительные проверки
bool rtl92ee_rx_query_desc(struct ieee80211_hw *hw,
struct rtl_stats *status,
struct ieee80211_rx_status *rx_status,
u8 *pdesc, struct sk_buff *skb)
{
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rx_fwinfo *p_drvinfo;
struct ieee80211_hdr *hdr;
u32 phystatus = GET_RX_DESC_PHYST(pdesc);
....
status->macid = GET_RX_DESC_MACID(pdesc);
if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
status->wake_match = BIT(2);
else if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
status->wake_match = BIT(1);
else if (GET_RX_STATUS_DESC_UNICAST_MATCH(pdesc))
status->wake_match = BIT(0);
else
status->wake_match = 0;
....
}
Со стороны сложно понять, что здесь не так. Два раза идёт одна и та же проверка макросом GET_RX_STATUS_DESC_MAGIC_MATCH. Если посмотрим его объявление, то увидим два других макроса:
#define GET_RX_STATUS_DESC_PATTERN_MATCH(__pdesc)
LE_BITS_TO_4BYTE(__pdesc+12, 29, 1)
#define GET_RX_STATUS_DESC_UNICAST_MATCH(__pdesc)
LE_BITS_TO_4BYTE(__pdesc+12, 30, 1)
#define GET_RX_STATUS_DESC_MAGIC_MATCH(__pdesc)
LE_BITS_TO_4BYTE(__pdesc+12, 31, 1)
Возможно хотели использовать отсутствующий в исходном фрагменте GET_RX_STATUS_DESC_PATTERN_MATCH. Иначе эта проверка просто бессмысленна.
int ext_sd_send_cmd_get_rsp(struct rtsx_chip *chip,
u8 cmd_idx, u32 arg, u8 rsp_type,
u8 *rsp, int rsp_len, bool special_check)
{
int retval;
int timeout = 100;
u16 reg_addr;
u8 *ptr;
....
if (cmd_idx == SELECT_CARD) {
if (rsp_type == SD_RSP_TYPE_R2) {
if ((ptr[3] & 0x1E) != 0x04) {
rtsx_trace(chip);
return STATUS_FAIL;
}
} else if (rsp_type == SD_RSP_TYPE_R0) {
if ((ptr[3] & 0x1E) != 0x03) { // <=
rtsx_trace(chip);
return STATUS_FAIL;
}
}
}
....
}
Ошибка связана с битовыми операциями. Результат побитовой конъюнкции с 0x1E из-за одного бита никогда не будет равен значению 0x03.
static int ks_wlan_set_power(struct net_device *dev,
struct iw_request_info *info,
struct iw_param *vwrq, char *extra)
{
struct ks_wlan_private *priv =
(struct ks_wlan_private *)netdev_priv(dev);
short enabled;
if (priv->sleep_mode == SLP_SLEEP) {
return -EPERM;
}
/* for SLEEP MODE */
enabled = vwrq->disabled ? 0 : 1;
if (enabled == 0) { /* 0 */
priv->reg.powermgt = POWMGT_ACTIVE_MODE;
} else if (enabled) { /* 1 */
if (priv->reg.operation_mode == MODE_INFRASTRUCTURE)
priv->reg.powermgt = POWMGT_SAVE1_MODE;
else
return -EINVAL;
} else if (enabled) { /* 2 */
if (priv->reg.operation_mode == MODE_INFRASTRUCTURE)
priv->reg.powermgt = POWMGT_SAVE2_MODE;
else
return -EINVAL;
} else
return -EINVAL;
hostif_sme_enqueue(priv, SME_POW_MNGMT_REQUEST);
return 0;
}
Сократим пример до:
enabled = vwrq->disabled ? 0 : 1;
if (enabled == 0) { /* 0 */
....
} else if (enabled) { /* 1 */
....
} else if (enabled) { /* 2 */
....
} else
....
Данный код выглядит очень странно. Вроде бы область значений чётко обговорена выражением выше: enabled равен 0 или 1. Но проверяется целых 4 значения. При этом комментарии только мешают: если цифры должны были обозначать возможное значение переменной, то сейчас они не соответствуют действительности: проверка на 1 и 2 записаны одинаковым образом.
static int odm_ARFBRefresh_8188E(
struct odm_dm_struct *dm_odm,
struct odm_ra_info *pRaInfo)
{ /* Wilson 2011/10/26 */
....
if (pRaInfo->HighestRate > 0x13)
pRaInfo->PTModeSS = 3;
else if (pRaInfo->HighestRate > 0x0b)
pRaInfo->PTModeSS = 2;
else if (pRaInfo->HighestRate > 0x0b)
pRaInfo->PTModeSS = 1;
else
pRaInfo->PTModeSS = 0;
....
return 0;
}
Ещё одно место, в котором идут подряд два условия. Обратите внимание, что тела при этом разные. Сложно сказать, есть ли здесь реальная ошибка, или это просто неиспользуемый код: это уже задача разработчиков проекта. Задача анализатора обратить внимание на подозрительное место.
static int sst_fill_widget_module_info(
struct snd_soc_dapm_widget *w,
struct snd_soc_platform *platform)
{
struct snd_kcontrol *kctl;
int index, ret = 0;
struct snd_card *card = platform->component.card->snd_card;
char *idx;
down_read(&card->controls_rwsem);
list_for_each_entry(kctl, &card->controls, list) {
....
} else if (strstr(kctl->id.name, "interleaver")) {
struct sst_enum *e = (void *)kctl->private_value;
e->w = w;
} else if (strstr(kctl->id.name, "deinterleaver")) {
struct sst_enum *e = (void *)kctl->private_value;
e->w = w;
}
....
}
up_read(&card->controls_rwsem);
return 0;
}
В этом фрагменте последовательно проверяют наличие нескольких подстрок в одной строке. Для наглядности я оставил только интересующие нас подстроки. Предположим, что мы не нашли interleaver - тогда нет смысла искать deinterleaver, ведь подстроки interleaver уже точно нет. Поэтому этот участок кода никогда не заработает, но, так как тела у if и else одинаковые, это не страшно. Это просто избыточный код.
void
nlmsvc_grant_reply(struct nlm_cookie *cookie, __be32 status)
{
struct nlm_block *block;
dprintk("grant_reply: looking for cookie %x, s=%d \n",
*(unsigned int *)(cookie->data), status);
if (!(block = nlmsvc_find_block(cookie)))
return;
if (block) {
if (status == nlm_lck_denied_grace_period) {
/* Try again in a couple of seconds */
nlmsvc_insert_block(block, 10 * HZ);
} else {
/* Lock is now held by client, or has been rejected.
* In both cases, the block should be removed. */
nlmsvc_unlink_block(block);
}
}
nlmsvc_release_block(block);
}
Этот пример демонстрирует, почему статическому анализатору недостаточно выполнять Pattern-based analysis, обходя AST. Важно уметь выполнять Control flow analysis и Data flow analysis. В момент, когда block == NULL происходит return, соответственно дальше по коду мы точно можем сказать, что указатель ненулевой. И когда мы встречаем проверку на NULL, мы точно понимаем, что что-то тут не так.
Похоже, что вторая проверка указателя здесь просто лишняя. Однако, вдруг здесь хотели проверить другую переменную? Кто знает... Этот код анализатору явно стоит предоставить разработчику для проверки.
Аналогичная ситуация:
bool menu_is_visible(struct menu *menu)
{
struct menu *child;
struct symbol *sym;
....
if (!sym || sym_get_tristate_value(menu->sym) == no) // <=
return false;
for (child = menu->list; child; child = child->next) {
if (menu_is_visible(child)) {
if (sym) // <=
sym->flags |= SYMBOL_DEF_USER;
return true;
}
}
return false;
}
Ошибка в макросе
#define CFS_FAIL_TIMEOUT(id, secs) \
cfs_fail_timeout_set(id, 0, secs * 1000, CFS_FAIL_LOC_NOSET)
#define OBD_FAIL_TIMEOUT(id, secs) \
CFS_FAIL_TIMEOUT(id, secs)
int ptl_send_rpc(struct ptlrpc_request *request, int noreply)
{
....
OBD_FAIL_TIMEOUT(OBD_FAIL_PTLRPC_DELAY_SEND,
request->rq_timeout + 5);
....
}
А вот такие ошибки очень редки. До этого я видел только одно срабатывание этой диагностики в реальном проекте: примечательно, что это был FreeBSD. Ошибку допустили в определении макроса: лучше всего все его параметры окружать скобками. Если этого не делать, то возможен такой случай: при подстановке 'x + 5' в 'secs * 1000' получается 'x + 5 * 1000', а это явно не то, что ожидал автор.
Бессмысленный memset
int amdgpu_atom_asic_init(struct atom_context *ctx)
{
int hwi = CU16(ctx->data_table + ATOM_DATA_FWI_PTR);
uint32_t ps[16];
int ret;
memset(ps, 0, 64);
ps[0] = cpu_to_le32(CU32(hwi + ATOM_FWI_DEFSCLK_PTR));
ps[1] = cpu_to_le32(CU32(hwi + ATOM_FWI_DEFMCLK_PTR));
if (!ps[0] || !ps[1])
return 1;
if (!CU16(ctx->cmd_table + 4 + 2 * ATOM_CMD_INIT))
return 1;
ret = amdgpu_atom_execute_table(ctx, ATOM_CMD_INIT, ps);
if (ret)
return ret;
memset(ps, 0, 64); // <=
return ret;
}
Нет смысла добавлять memset перед return: компилятор, увидев, что эта операция не меняет видимое состояние программы (массив всё равно выходит из области видимости), удалит её. Если нужно стереть какие-то важные данные, то для этого стоит использовать memset_s или написать свой аналог.
Это ошибка кстати фактически является уязвимостью. Не затираются данные, которые должны быть затёрты.
Опасное использование memcmp
static void power_control_timeout(unsigned long data)
{
....
u8 other = memcmp(requester->frame_rcvd.iaf.sas_addr,
iphy->frame_rcvd.iaf.sas_addr,
sizeof(requester->frame_rcvd.iaf.sas_addr));
if (other == 0) {
....
}
....
}
Если внимательно прочитать, что говорит документация о возвращаемом значении memcmp, то мы увидим, что гарантии о каком-либо конкретном диапазоне нет: функция может вернуть любое число в рамках своего типа. И это не всегда -1, 0 и 1. Поэтому нельзя сохранять его значение в переменной меньшего типа: при потере старших разрядов младшие могут составить ноль. Похожая ошибка привела к уязвимости в MySQL/MariaDB.
Перемещено tailgunner из kernel
