Как найти 56 потенциальных уязвимостей в коде FreeBSD за один вечер06.04.2017 11:33

Пришло время вновь проверить проект FreeBSD и продемонстрировать, что даже в таких серьезных и качественных проектах анализатор PVS-Studio легко находит ошибки. В этот раз я решил взглянуть на поиск ошибок с точки зрения обнаружения потенциальных уязвимостей. Анализатор PVS-Studio всегда умел выявлять дефекты, которые потенциально можно использовать для атаки. Но мы никогда не акцентировали на этом внимание и описывали ошибки как опечатки, последствия неудачного Copy-Paste и так далее, и не классифицировали их, например, согласно CWE. Сейчас очень популярно говорить и о безопасности, и об уязвимостях, поэтому попробую немного расширить ваше восприятие нашего анализатора. PVS-Studio — это не только поиск багов, но ещё и инструмент, повышающий безопасность кода.

О проверке

Отчет о нашей проверке FreeBSD в 2016 году можно посмотреть здесь.

Как следует из названия, то, что будет описано в статье, я нашел за один вечер. Т.е. на поиск потенциальных уязвимостей я потратил всего 2–3 часа. Это говорит о всей мощи статического анализатора PVS-Studio. Я рекомендую использовать наш анализатор всем, кого заботит качество кода, а тем более его надёжность и устойчивость к возможным атакам.

Выписал код с ошибками я быстро, а вот найти время оформить свои изыскания в виде этой статьи я не мог три недели. За это время мы даже поправили некоторые из ошибок, которые будут описаны в статье, в рамках нового направления «Дефекты безопасности, которые устранила команда PVS-Studio на этой неделе»: выпуск N2, выпуск N3.

Правили мы то, что попроще и где понятно, как поправить, не вдаваясь в суть алгоритмов. Поэтому авторам FreeBSD не стоит ограничиваться нашими правками и даже этой статьей, а имеет смысл провести анализ кода самостоятельно и более подробно. Я готов выдать им временный ключ для полноценной проверки, а также помочь в борьбе с ложными срабатываниями, которые будут им мешать. Кстати, о ложных срабатываниях…

Ложные срабатывания

Проверив проект с помощью PVS-Studio, можно получить очень большой разброс по количеству ложных срабатываний. Например, мы недавно проверяли проект FAR, и количество ложных срабатываний составило 50%. Это отличный результат, означающий, что каждое второе сообщение указывает на ошибку или крайне плохой код. А при проверке Media Portal 2 результат был ещё лучше: 27% ложных срабатываний.

С проектом FreeBSD всё сложнее. Дело в том, что анализатор выдал огромное количество предупреждений общего назначения:

• 3577 уровня High
• 2702 уровня Medium

Большинство сообщений, естественно, являются ложными срабатываниями. Посчитать сложно, но думаю, ложных срабатываний будет около 95%.

О чем это говорит? Это говорит, что нет смысла рассуждать о количестве ложных срабатываний на больших проектах, не проведя предварительную настройку анализатора. Большинство ложных предупреждений возникает из-за разных макросов и их легко убрать, используя различные механизмы, предусмотренные в PVS-Studio. Поясню это на примере.

В коде FreeBSD можно встретить вот такой массив:

#ifdef Q
#undef Q
#endif
#define Q(_r) \
  (((_r) == 1.5) ? 0 : (((_r) ==2.25) ? 1 : (((_r) == 3) ? 2 : \
  (((_r) == 4.5) ? 3 : (((_r) ==  6)  ? 4 : (((_r) == 9) ? 5 : \
  (((_r) == 12)  ? 6 : (((_r) == 13.5)? 7 : 0))))))))
static const struct txschedule series_quarter[] = {
  { 3,Q( 1.5),3,Q(1.5), 0,Q(1.5), 0,Q(1.5) },  /* 1.5Mb/s */
  { 4,Q(2.25),3,Q(1.5), 4,Q(1.5), 0,Q(1.5) },  /*2.25Mb/s */
  { 4,Q(   3),3,Q(1.5), 4,Q(1.5), 0,Q(1.5) },  /*   3Mb/s */
  { 4,Q( 4.5),3,Q(  3), 4,Q(1.5), 2,Q(1.5) },  /* 4.5Mb/s */
  { 4,Q(   6),3,Q(4.5), 4,Q(  3), 2,Q(1.5) },  /*   6Mb/s */
  { 4,Q(   9),3,Q(  6), 4,Q(4.5), 2,Q(1.5) },  /*   9Mb/s */
  { 4,Q(  12),3,Q(  9), 4,Q(  6), 2,Q(  3) },  /*  12Mb/s */
  { 4,Q(13.5),3,Q( 12), 4,Q(  9), 2,Q(  6) }  /*13.5Mb/s */
};
#undef Q

Макрос Q (1.5) раскрывается в:

(((1.5) == 1.5) ? 0 : (((1.5) ==2.25) ? 1 : (((1.5) == 3) ? 2 : \
(((1.5) == 4.5) ? 3 : (((1.5) ==  6)  ? 4 : (((1.5) == 9) ? 5 : \
(((1.5) == 12)  ? 6 : (((1.5) == 13.5)? 7 : 0))))))))

Некоторые из сравнений анализатор PVS-Studio считает подозрительными. Например, на выражение (((1.5) == 3) он выдаёт предупреждение:

V674 The '1.5' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the '(1.5) == 3' expression. tx_schedules.h 228

Всего на этот массив анализатор выдаёт 96 сообщений.

В коде FreeBSD можно встретить ещё несколько таких массивов. Суммарно анализатор выдаёт на них 692 предупреждения уровня High. Напомню, что всего предупреждений уровня High насчитывается 3577. Это значит, что такие макросы приводят к возникновению 1/5 этих предупреждений.

Другими словами, чуть-чуть настроив анализатор, можно устранить 20% ложных сообщений уровня High. Сделать это можно по-разному, но, пожалуй, проще всего будет выключить предупреждение V674 для тех файлов, в которых используются подобные массивы. Для этого нужно написать где-то в файле комментарий //-V::674.

Я уже озвучивал важную мысль, но повторю её, так как нас вновь и вновь спрашивают о среднем проценте ложных срабатываний. Даже если мы посчитаем этот средний процент на основании проверки многих проектов, он не будет иметь никакой практической ценности. Это тоже самое, как интересоваться средней температурой по больнице.

Все очень сильно зависит от проекта. Кому-то повезёт и можно работать со списком предупреждений, практически ничего не настраивая. Другим не повезёт, как в случае с проектом FreeBSD. Придётся заниматься настройкой и разметкой макросов. Но это не так страшно, как может показаться на первый взгляд. Выше я как раз показал, каким образом сразу можно устранить много ложных предупреждений. Аналогичная ситуация будет и с другими предупреждениями, возникающими из-за дурацких макросов.

Да и вообще, если бы с шумом было сложно бороться, я бы не смог найти все эти ошибки для статьи за один вечер.

Новый взгляд на мир

Мы решили смотреть на мир более широко. Там, где раньше мы видели ошибки и код с запахом, теперь мы стараемся видеть ещё и потенциальные уязвимости. Для этого мы решили начать с того, что будем классифицировать предупреждения, выдаваемые с помощью PVS-Studio согласно Common Weakness Enumeration (CWE). Подробнее про это можно почитать здесь: PVS-Studio: поиск дефектов безопасности.

Конечно, только малую часть ошибок можно эксплуатировать. Другими словами, только немногие из найденных CWE ошибок могут превратиться в CVE. Однако, чем больше ошибок, попадающих под классификацию CWE, будет найдено с помощью статического анализа, тем лучше.

Используйте PVS-Studio для профилактики уязвимостей. Эта статья продемонстрирует, что анализатор хорошо справляется с этой задачей.

Потенциальные уязвимости

CWE-476: NULL Pointer Dereference

Всего я заметил 22 ошибки этого типа. И, наверное, ещё столько же пропустил.

Давайте начнем с простого случая.

void
ql_mbx_isr(void *arg)
{
  ....
  ha = arg;
  if (ha == NULL) {
    device_printf(ha->pci_dev, "%s: arg == NULL\n", __func__);
    return;
  }
  ....
}

Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'ha' might take place. ql_isr.c 750

Здесь ошибка видна сразу. Если указатель ha равен NULL, то он разыменовывается в выражении ha→pci_dev.

Такая же ситуация обнаруживается ещё в трёх файлах:

• V522 Dereferencing of the null pointer 'sc' might take place. tws_cam.c 1066
• V522 Dereferencing of the null pointer 'ni' might take place. ieee80211_hwmp.c 1925
• V522 Dereferencing of the null pointer 'sbp' might take place. sbp.c 2337

Теперь рассмотрим более сложную ситуацию:

static int ecore_ilt_client_mem_op(struct bxe_softc *sc,
                                   int cli_num, uint8_t memop)
{
  int i, rc;
  struct ecore_ilt *ilt = SC_ILT(sc);
  struct ilt_client_info *ilt_cli = &ilt->clients[cli_num];

  if (!ilt || !ilt->lines)
    return -1;
  ....
}

Предупреждение PVS-Studio: V595 The 'ilt' pointer was utilized before it was verified against nullptr. Check lines: 667, 669. ecore_init_ops.h 667

На этом случае остановимся поподробнее, так как далеко не все понимают всю опасность, которая заключена в этом коде.

В начале указатель ilt разыменовывается:

struct ilt_client_info *ilt_cli = &ilt->clients[cli_num];

Далее он проверяется на равенство NULL:

if (!ilt || !ilt->lines)

Следовательно, возможно разыменование нулевого указателя. Это неизбежно приводит к неопределённому поведению программы.

Здесь некоторые возразят, что никакой беды нет, так как указатель разыменовывается «не по-настоящему». Они скажут, что просто вычисляется адрес ячейки массива. Да, скажут они, этот адрес некорректен и его использовать нельзя. Однако, ниже есть проверка, и произойдёт выход из функции, если указатель ilt будет равен нулю. Следовательно, невалидный указатель ilt_cli не будет нигде использоваться и никакой ошибки в программе нет.

Они не правы. Нельзя так размышлять. Разыменование нулевого указателя приводит к неопределенному поведению. Следовательно, код некорректен и не следует рассуждать, как он будет работать. Он может работать как угодно.

Однако, такое объяснение часто не устраивает некоторых читателей, поэтому я попробую развить мысль. Компилятор знает, что разыменование нулевого указателя — это неопределённое поведение. Следовательно, если указатель разыменовывают, то он не равен NULL. Раз он не равен NULL, компилятор имеет полное право удалить избыточную проверку if (! ilt). В результате, если указатель будет равен NULL, то из функции не произойдёт выход. Поэтому функция начнет обрабатывать невалидные указатели, что может привести к чему угодно.

Некоторые возражают, говоря, что макрос offsetof иногда реализуют следующим образом:

#define offsetof(st, m) ((size_t)(&((st *)0)->m))

Здесь имеет место разыменование нулевого указателя, но код успешно работает. Следовательно, это доказывает, что подобные конструкции вполне допустимы.

И вновь они не правы. Ничего это не доказывает.

При рассмотрении идиоматической реализации offsetof следует учитывать, что компилятору разрешено использовать непереносимые приемы для реализации этой функциональности. Тот факт, что в реализации библиотеки в компиляторе используется константа нулевого указателя при реализации offsetof, вовсе не означает, что в пользовательском коде можно без опаски выполнять &ilt→clients[cli_num] в случае, когда ilt является нулевым указателем.

Более подробно с этой темой можно познакомиться в моей статье «Разыменовывание нулевого указателя приводит к неопределённому поведению».

В итоге, рассмотренный выше код представляет собой самую настоящую ошибку и должен быть исправлен.

Теперь, когда мы разобрались с нюансами разыменования нулевого указателя, становится понятно, что неправильной является и следующая функция:

static struct iscsi_outstanding *
iscsi_outstanding_add(struct iscsi_session *is,
                      struct icl_pdu *request,
                      union ccb *ccb,
                      uint32_t *initiator_task_tagp)
{
  struct iscsi_outstanding *io;
  int error;

  ISCSI_SESSION_LOCK_ASSERT(is);

  io = uma_zalloc(iscsi_outstanding_zone, M_NOWAIT | M_ZERO);
  if (io == NULL) {
    ISCSI_SESSION_WARN(is, "failed to allocate %zd bytes",
        sizeof(*io));
    return (NULL);
  }

  error = icl_conn_task_setup(is->is_conn, request, &ccb->csio,
    initiator_task_tagp, &io->io_icl_prv);
  ....
}

Предупреждение анализатора PVS-Studio: V522 Dereferencing of the null pointer 'ccb' might take place. The null pointer is passed into 'iscsi_outstanding_add' function. Inspect the third argument. Check lines: 'iscsi.c:2157'. iscsi.c 2091

В начале может быть не понятно, почему анализатор решил, что указатель ccb будет нулевым. Поэтому обратим внимание, что анализатор указывает в предупреждении ещё и вот сюда: iscsi.c:2157.

Здесь находится вызов функции iscsi_outstanding_add, в которую в качестве фактического аргумента передаётся NULL:

static void
iscsi_action_abort(struct iscsi_session *is, union ccb *ccb)
{
  ....
  io = iscsi_outstanding_add(is, request, NULL,
                             &initiator_task_tag);
  ....
}

Здесь анализатору понадобилось выполнить межпроцедурный анализ, чтобы найти дефект.

Немного отдохнем от сложных ошибок и рассмотрим совсем простой случай неправильного обработчика ошибок.

int radeon_cs_ioctl(struct drm_device *dev, void *data,
                    struct drm_file *fpriv)
{
  ....
  struct drm_radeon_private *dev_priv = dev->dev_private;
  ....
  if (dev_priv == NULL) {
    DRM_ERROR("called with no initialization\n");
    mtx_unlock(&dev_priv->cs.cs_mutex);
    return -EINVAL;
  }
  ....
}

Предупреждение анализатора PVS-Studio: V522 Dereferencing of the null pointer 'dev_priv' might take place. radeon_cs.c 153

Тело оператора if выполняется только когда указатель dev_priv равен нулю. Таким образом, здесь вычисляется непонятно какой адрес: &dev_priv→cs.cs_mutex. Да и вообще это UB.

Ох. Что-то проблемы с нулевыми указателями никак не заканчиваются. Но что делать, раз это головная боль многих языков программирования. Так что заварите кофе и продолжайте чтение.

static void
bwn_txpwr(void *arg, int npending)
{
  struct bwn_mac *mac = arg;
  struct bwn_softc *sc = mac->mac_sc;

  BWN_LOCK(sc);
  if (mac && mac->mac_status >= BWN_MAC_STATUS_STARTED &&
      mac->mac_phy.set_txpwr != NULL)
    mac->mac_phy.set_txpwr(mac);
  BWN_UNLOCK(sc);
}

Предупреждение анализатора PVS-Studio: V595 The 'mac' pointer was utilized before it was verified against nullptr. Check lines: 6757, 6760. if_bwn.c 6757

Указатель mac в начале разыменовывается, а далее проверяется на равенство NULL. Здесь всё просто, так что подробнее комментировать не буду.

struct opcode_obj_rewrite *ctl3_rewriters;
void
ipfw_add_obj_rewriter(struct opcode_obj_rewrite *rw,
                      size_t count)
{
  ....
  memcpy(tmp, ctl3_rewriters, ctl3_rsize * sizeof(*rw));  // <=
  memcpy(&tmp[ctl3_rsize], rw, count * sizeof(*rw));
  qsort(tmp, sz, sizeof(*rw), compare_opcodes);
  /* Switch new and free old */
  if (ctl3_rewriters != NULL)                             // <=
    free(ctl3_rewriters, M_IPFW);
  ctl3_rewriters = tmp;
  ctl3_rsize = sz;

  CTL3_UNLOCK();
}

Предупреждение анализатора PVS-Studio: V595 The 'ctl3_rewriters' pointer was utilized before it was verified against nullptr. Check lines: 3206, 3210. ip_fw_sockopt.c 3206

Обратите внимание, что вначале указатель ctl3_rewriters используется как фактический аргумент функции memcpy:

memcpy(tmp, ctl3_rewriters, ctl3_rsize * sizeof(*rw));

А затем вдруг вспоминают, что его следует проверять на равенство NULL:

if (ctl3_rewriters != NULL)

Рассмотрим ещё один случай неправильного кода, предназначенного для освобождения ресурсов:

static int
mly_user_command(struct mly_softc *sc, struct mly_user_command *uc)
{
  struct mly_command  *mc;
  ....
  if (mc->mc_data != NULL)           // <=
    free(mc->mc_data, M_DEVBUF);     // <=
  if (mc != NULL) {                  // <=
    MLY_LOCK(sc);
    mly_release_command(mc);
    MLY_UNLOCK(sc);
  }
  return(error);
}

Предупреждение анализатора PVS-Studio: V595 The 'mc' pointer was utilized before it was verified against nullptr. Check lines: 2954, 2955. mly.c 2954

Стоит закругляться с нулевыми указателями, так как, думаю, описание таких ошибок уже наскучило не только мне, но и читателю. Я вижу CWE-476 (NULL Pointer Dereference) ещё в следующих участках кода:

• V595 The 'cm' pointer was utilized before it was verified against nullptr. Check lines: 3361, 3381. mfi.c 3361
• V595 The 'cm' pointer was utilized before it was verified against nullptr. Check lines: 1383, 1394. mpr_sas_lsi.c 1383
• V595 The 'cm' pointer was utilized before it was verified against nullptr. Check lines: 1258, 1269. mps_sas_lsi.c 1258
• V595 The 'ctl3_handlers' pointer was utilized before it was verified against nullptr. Check lines: 3441, 3445. ip_fw_sockopt.c 3441
• V595 The 'ccb' pointer was utilized before it was verified against nullptr. Check lines: 540, 547. iscsi_subr.c 540
• V595 The 'satOrgIOContext' pointer was utilized before it was verified against nullptr. Check lines: 11341, 11344. smsatcb.c 11341
• V595 The 'satOrgIOContext' pointer was utilized before it was verified against nullptr. Check lines: 11498, 11501. smsatcb.c 11498
• V595 The 'm' pointer was utilized before it was verified against nullptr. Check lines: 1153, 1157. midi.c 1153
• V595 The 'm' pointer was utilized before it was verified against nullptr. Check lines: 1153, 1157. midi.c 1153
• V595 The 'es' pointer was utilized before it was verified against nullptr. Check lines: 1882, 1893. es137x.c 1882
• V595 The 'via' pointer was utilized before it was verified against nullptr. Check lines: 1375, 1392. via8233.c 1375
• V595 The 'via' pointer was utilized before it was verified against nullptr. Check lines: 604, 613. via82c686.c 604

Но и это ещё не всё! Мне просто наскучило изучать эту разновидность ошибок, и я перешел к изучению предупреждений другого типа. Анализатор PVS-Studio ждёт героев, которые изучат все предупреждения, которые касаются нулевых указателей.

CWE-467: Use of sizeof () on a Pointer Type

Рассмотрим определение структуры pfloghdr:

struct pfloghdr {
  u_int8_t  length;
  sa_family_t  af;
  u_int8_t  action;
  u_int8_t  reason;
  char    ifname[IFNAMSIZ];
  char    ruleset[PFLOG_RULESET_NAME_SIZE];
  u_int32_t  rulenr;
  u_int32_t  subrulenr;
  uid_t    uid;
  pid_t    pid;
  uid_t    rule_uid;
  pid_t    rule_pid;
  u_int8_t  dir;
  u_int8_t  pad[3];
};

Как видите, такая структура достаточно большая. Для инициализации подобных структур распространена практика, когда вся структура заполняется нулями, а потом устанавливаются значения для отдельных полей.

Однако, в функции nat64lsn_log не удалось корректно произвести инициализацию структуры. Рассмотрим код этой функции:

static void
nat64lsn_log(struct pfloghdr *plog, ....)
{
  memset(plog, 0, sizeof(plog));        // <=
  plog->length = PFLOG_REAL_HDRLEN;
  plog->af = family;
  plog->action = PF_NAT;
  plog->dir = PF_IN;
  plog->rulenr = htonl(n);
  plog->subrulenr = htonl(sn);
  plog->ruleset[0] = '\0';
  strlcpy(plog->ifname, "NAT64LSN", sizeof(plog->ifname));
  ipfw_bpf_mtap2(plog, PFLOG_HDRLEN, m);
}

Предупреждение анализатора PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer 'plog'. nat64lsn.c 218

Обратите внимание, что sizeof (plog) вычисляет не размер структуры, а размер указателя. В результате обнуляется не вся структура, а только несколько первых байт, все остальные поля структуры остаются неинициализированными. Конечно, в некоторые поля затем явно записываются правильные значения. Однако, ряд членов в структуре останется неинициализированными.

Точно такую же ошибку можно наблюдать в файле nat64stl.c: V512 A call of the 'memset' function will lead to underflow of the buffer 'plog'. nat64stl.c 72

CWE-457: Use of Uninitialized Variable

Рассмотрим ещё одну ошибку, из-за которой переменная может быть не инициализирована.

osGLOBAL bit32
tdsaSendTMFIoctl(
      tiRoot_t             *tiRoot,
      tiIOCTLPayload_t     *agIOCTLPayload,
      void                 *agParam1,
      void                 *agParam2,
      unsigned long        resetType
    )
{
  bit32    status;
  tmf_pass_through_req_t  *tmf_req = ....;
#if !(defined(__FreeBSD__))
  status = ostiSendResetDeviceIoctl(tiRoot, agParam2,
    tmf_req->pathId, tmf_req->targetId, tmf_req->lun, resetType);
#endif
  TI_DBG3((
    "Status returned from ostiSendResetDeviceIoctl is %d\n",
    status));
  if(status != IOCTL_CALL_SUCCESS)
  {
    agIOCTLPayload->Status = status;
    return status;
  }
  status = IOCTL_CALL_SUCCESS;
  return status;
}

Предупреждение анализатора PVS-Studio: V614 Uninitialized variable 'status' used. tdioctl.c 3396

Если объявлен макрос __FreeBSD__ (а он объявлен), то условие

#if !(defined(__FreeBSD__))

не выполняется. Как результат, код внутри конструкции #if…#endif не компилируется, и переменная status остаётся неинициализированной.

CWE-805: Buffer Access with Incorrect Length Value

typedef struct qls_mpid_glbl_hdr
{
  uint32_t  cookie;
  uint8_t   id[16];
  uint32_t  time_lo;
  ....
} qls_mpid_glbl_hdr_t;

struct qls_mpi_coredump {
  qls_mpid_glbl_hdr_t  mpi_global_header;
  ....
};

typedef struct qls_mpi_coredump qls_mpi_coredump_t;

int
qls_mpi_core_dump(qla_host_t *ha)
{
  ....
  qls_mpi_coredump_t *mpi_dump = &ql_mpi_coredump;
  ....
  memcpy(mpi_dump->mpi_global_header.id, "MPI Coredump",
         sizeof(mpi_dump->mpi_global_header.id));
  ....
}

Предупреждение анализатора PVS-Studio: V512 A call of the 'memcpy' function will lead to the '«MPI Coredump»' buffer becoming out of range. qls_dump.c 1615

Чтобы показать, как объявлены интересующие нас типы и члены структуры, пришлось привести достаточно много кода. Но не спешите зевать, сейчас я выделю самое существенное:

uint8_t id[16];
memcpy(id, "MPI Coredump", sizeof(id));

Для нас важно:

1. Оператор sizeof вычисляет размер массива и возвращает число 16.
2. Строка «MPI Coredump» с учетом терминального нуля занимает 13 байт.

Будет скопировано 13 байт строки и ещё 3 байта, расположенные после строки. Такой код на практике может даже работать. Просто скопируется 3 байта с мусором или фрагмент другой строки. Однако, формально, это выход за границу массива, а, следовательно, этот код приводит к неопределённому поведению программы.

CWE-129: Improper Validation of Array Index

Вот и представился случай продемонстрировать вам одну из новых диагностик, реализованных в PVS-Studio. Суть диагностики V781:

В начале значение переменной используется в качестве размера или индекса массива. А уже затем это значение сравнивается с 0 или с размером массива. Это может указывать на наличие логической ошибки в коде или опечатку в одном из сравнений.

По своей сути эта диагностика схожа с V595, которая хорошо знакома моим постоянным читателям.

Посмотрим, где сработала эта диагностика при проверке кода FreeBSD.
static void
sbp_targ_mgm_handler(struct fw_xfer *xfer)
{
  ....
  int exclusive = 0, lun;
  ....
  lun = orb4->id;
  lstate = orbi->sc->lstate[lun];

  if (lun >= MAX_LUN || lstate == NULL ||
      (exclusive &&
      STAILQ_FIRST(&lstate->logins) != NULL &&
      STAILQ_FIRST(&lstate->logins)->fwdev != orbi->fwdev)
     ) {
    /* error */
    orbi->status.dead = 1;
    orbi->status.status = STATUS_ACCESS_DENY;
    orbi->status.len = 1;
    break;
  }
  ....
}

Предупреждение анализатора PVS-Studio: V781 The value of the 'lun' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 1617, 1619. sbp_targ.c 1617

В начале программист смело использовал индекс lun для доступа к массиву lstate. И только затем осуществляется проверка: не превышает ли значение индекса предельно допустимое значение, равное MAX_LUN? Если превышает, то эта ситуация обрабатывается как ошибочная. Но уже поздно, так как мы уже могли обратиться далеко за пределы массива.

Формально, это приведёт к неопределённому поведению программы. На практике, вместо корректной обработки неправильного значения индекса может возникнуть Access Violation.

Теперь рассмотрим более интересный случай неправильной индексации массива.

#define R88E_GROUP_2G    6
#define RTWN_RIDX_OFDM6  4
#define RTWN_RIDX_COUNT  28

struct rtwn_r88e_txagc {
  uint8_t pwr[R88E_GROUP_2G][20];  /* RTWN_RIDX_MCS(7) + 1 */
};

void
r88e_get_txpower(struct rtwn_softc *sc, int chain,
    struct ieee80211_channel *c, uint16_t power[RTWN_RIDX_COUNT])
{
  const struct rtwn_r88e_txagc *base = rs->rs_txagc;
  ....
  for (ridx = RTWN_RIDX_OFDM6; ridx < RTWN_RIDX_COUNT; ridx++) {
    if (rs->regulatory == 3)
      power[ridx] = base->pwr[0][ridx];
    else if (rs->regulatory == 1) {
      if (!IEEE80211_IS_CHAN_HT40(c))
        power[ridx] = base->pwr[group][ridx];
    } else if (rs->regulatory != 2)
      power[ridx] = base->pwr[0][ridx];
  }
  ....
}

Анализатор выдал здесь три предупреждения на три выражения, где осуществляется доступ к массиву pwr:

• V557 Array overrun is possible. The value of 'ridx' index could reach 27. r88e_chan.c 115
• V557 Array overrun is possible. The value of 'ridx' index could reach 27. r88e_chan.c 118
• V557 Array overrun is possible. The value of 'ridx' index could reach 27. r88e_chan.c 120

В цикле значение индекса ridx изменяется от значения RTWN_RIDX_OFDM6 до RTWN_RIDX_COUNT. То есть переменная ridx принимает значения в диапазоне [4…27]. На первый взгляд, всё хорошо.

Чтобы найти ошибку, обратим внимание на член pwr, представляющий собой двухмерный массив:

uint8_t pwr[R88E_GROUP_2G][20];    // R88E_GROUP_2G == 6

И давайте ещё раз посмотрим, как происходит доступ к этому массиву в цикле:

base->pwr[0][ridx]                 // ridx=[4..27]
base->pwr[group][ridx]             // ridx=[4..27]
base->pwr[0][ridx]                 // ridx=[4..27]

Здесь явно что-то не так. Происходит выход за границу массива. Однако, я затрудняюсь предположить, как должен работать этот код и что здесь следует изменить.

CWE-483: Incorrect Block Delimitation

static int
smbfs_getattr(ap)
struct vop_getattr_args *ap;
{
  ....
  if (np->n_flag & NOPEN)
    np->n_size = oldsize;
    smbfs_free_scred(scred);
  return 0;
}

Предупреждение анализатора PVS-Studio: V640 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. smbfs_vnops.c 283

Оформление кода не соответствует логике его работы. Визуально кажется, что строчка smbfs_free_scred (scred); выполняется только, если условие истинно. Но на самом деле, эта сточка выполняется всегда.

Возможно, настоящей ошибки здесь нет, и достаточно отформатировать код, но, в любом случае, этот фрагмент кода заслуживает самого пристального внимания.

Анализатор выявил ещё 4 четыре аналогичных подозрительных фрагмента кода, но не буду их здесь приводить, так как они все однотипны. Ограничусь предупреждениями:

• V640 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. ctl.c 8569
• V640 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. ieee80211_ioctl.c 2019
• V640 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. in_mcast.c 1063
• V640 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. in6_mcast.c 1004

CWE-563: Assignment to Variable without Use ('Unused Variable')

int
ipf_p_ftp_port(softf, fin, ip, nat, ftp, dlen)
  ipf_ftp_softc_t *softf;
  fr_info_t *fin;
  ip_t *ip;
  nat_t *nat;
  ftpinfo_t *ftp;
  int dlen;
{
  ....
  if (nat->nat_dir == NAT_INBOUND)
    a1 = ntohl(nat->nat_ndstaddr);   // <=
  else
    a1 = ntohl(ip->ip_src.s_addr);   // <=
  a1 = ntohl(ip->ip_src.s_addr);     // <=
  a2 = (a1 >> 16) & 0xff;
  a3 = (a1 >> 8) & 0xff;
  a4 = a1 & 0xff;
  ....
}

Предупреждение анализатора PVS-Studio: V519 The 'a1' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 397, 400. ip_ftp_pxy.c 400

Независимо от условия, переменной a1 будет присвоено значение ntohl (ip→ip_src.s_addr).

Мне кажется, что последнее присваивание является лишним. Возможно, такой код получился в результате неудачного рефакторинга.

Продолжим рассмотрение ошибок этой разновидности:

static inline int ecore_func_send_switch_update(
  struct bxe_softc *sc,
  struct ecore_func_state_params *params)
{
  ....
  if (ECORE_TEST_BIT(ECORE_F_UPDATE_VLAN_FORCE_PRIO_FLAG,
                     &switch_update_params->changes))
     rdata->sd_vlan_force_pri_flg = 1;
  rdata->sd_vlan_force_pri_flg =
    switch_update_params->vlan_force_prio;
  ....
}

Предупреждение анализатора PVS-Studio: V519 The 'rdata→sd_vlan_force_pri_flg' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 6327, 6328. ecore_sp.c 6328

Ситуация аналогичная, так что не будем на ней останавливаться. Идём дальше.

static int
ixgbe_add_vf(device_t dev, u16 vfnum, const nvlist_t *config)
{
  ....
  if (nvlist_exists_binary(config, "mac-addr")) {
    mac = nvlist_get_binary(config, "mac-addr", NULL);
    bcopy(mac, vf->ether_addr, ETHER_ADDR_LEN);
    if (nvlist_get_bool(config, "allow-set-mac"))
      vf->flags |= IXGBE_VF_CAP_MAC;
  } else
    /*
     * If the administrator has not specified a MAC address then
     * we must allow the VF to choose one.
     */
    vf->flags |= IXGBE_VF_CAP_MAC;

  vf->flags = IXGBE_VF_ACTIVE;
  ....
}

Предупреждение анализатора PVS-Studio: V519 The 'vf→flags' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 5992, 5994. if_ix.c 5994

Скорее всего пропустили »|» и корректный код должен был быть таким:

vf->flags |= IXGBE_VF_ACTIVE;

Вообще, все эти ошибки, которые я рассматриваю, пугают. Точнее, их количество. Ведь я вижу, сколько ошибок выписал, и что конец статьи не близок.

typedef struct {
  uint64_t  __mask;
} l_sigset_t;
int
linux_sigreturn(struct thread *td,
                struct linux_sigreturn_args *args)
{
  l_sigset_t lmask;
  ....
  lmask.__mask = frame.sf_sc.sc_mask;
  lmask.__mask = frame.sf_extramask[0];
  ....
}

Предупреждение анализатора PVS-Studio: V519 The 'lmask.__mask' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 594, 595. linux32_sysvec.c 595

static u_int sysctl_log_level = 0;
....
int sysctl_chg_loglevel(SYSCTL_HANDLER_ARGS)
{
  u_int level = *(u_int *)arg1;
  int error;
  error = sysctl_handle_int(oidp, &level, 0, req);
  if (error) return (error);

  sysctl_log_level =
    (level > SN_LOG_DEBUG_MAX)?(SN_LOG_DEBUG_MAX):(level);
  sysctl_log_level =
    (level < SN_LOG_LOW)?(SN_LOG_LOW):(level);

  return (0);
}

Предупреждение анализатора PVS-Studio: V519 The 'sysctl_log_level' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 423, 424. alias_sctp.c 424

Видимо код писался методом Copy-Paste, и, как всегда, имя самой последней переменной забыли поменять. Следовало написать:

sysctl_log_level =
  (level < SN_LOG_LOW)?(SN_LOG_LOW):(sysctl_log_level);

См. философско-исследовательскую статью на эту тему: «Объяснение эффекта последней строки».

Продолжим изучать, как глубока кроличья нора.

static int
uath_tx_start(struct uath_softc *sc, struct mbuf *m0,
              struct ieee80211_node *ni, struct uath_data *data)
{
  ....
  chunk->flags = (m0->m_flags & M_FRAG) ? 0 : UATH_CFLAGS_FINAL;
  if (m0->m_flags & M_LASTFRAG)
    chunk->flags |= UATH_CFLAGS_FINAL;
  chunk->flags = UATH_CFLAGS_FINAL;
  ....
}

Предупреждение анализатора PVS-Studio: V519 The 'chunk→flags' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1566, 1567. if_uath.c 1567

Пора вставить какую-то картинку для расслабления и отдыха. Думаю, эта как раз подойдёт.

static void ch7017_mode_set(....)
{
  uint8_t lvds_pll_feedback_div, lvds_pll_vco_control;
  ....
  lvds_pll_feedback_div =
    CH7017_LVDS_PLL_FEEDBACK_DEFAULT_RESERVED |
    (2 << CH7017_LVDS_PLL_FEED_BACK_DIVIDER_SHIFT) |
    (3 << CH7017_LVDS_PLL_FEED_FORWARD_DIVIDER_SHIFT);
  lvds_pll_feedback_div = 35;
  ....
}

Предупреждение анализатора PVS-Studio: V519 The 'lvds_pll_feedback_div' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 287, 290. dvo_ch7017.c 290

Перетирать значение переменной магическим числом 35 ну очень странно и подозрительно. Похоже, что кто-то написал эту строчку в отладочных целях, а потом забыл её удалить.

static void
bhnd_pmu1_pllinit0(struct bhnd_pmu_softc *sc, uint32_t xtal)
{
  uint32_t pmuctrl;
  ....
  /* Write XtalFreq. Set the divisor also. */
  pmuctrl = BHND_PMU_READ_4(sc, BHND_PMU_CTRL);
  pmuctrl = ~(BHND_PMU_CTRL_ILP_DIV_MASK |
              BHND_PMU_CTRL_XTALFREQ_MASK);
  pmuctrl |= BHND_PMU_SET_BITS(((xt->fref + 127) / 128) - 1,
                               BHND_PMU_CTRL_ILP_DIV);
  pmuctrl |= BHND_PMU_SET_BITS(xt->xf, BHND_PMU_CTRL_XTALFREQ);
  ....
}

Предупреждение анализатора PVS-Studio: V519 The 'pmuctrl' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2025, 2026. bhnd_pmu_subr.c 2026

Вместо оператора |= случайно написали =.

И последний, на сегодня, дефект этого типа:

void e1000_update_mc_addr_list_vf(struct e1000_hw *hw,
  u8 *mc_addr_list, u32 mc_addr_count)
{
  ....
  if (mc_addr_count > 30) {
    msgbuf[0] |= E1000_VF_SET_MULTICAST_OVERFLOW;
    mc_addr_count = 30;
  }

  msgbuf[0] = E1000_VF_SET_MULTICAST;
  msgbuf[0] |= mc_addr_count << E1000_VT_MSGINFO_SHIFT;
  ....
}

Предупреждение анализатора PVS-Studio: V519 The 'msgbuf[0]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 422, 426. e1000_vf.c 426

CWE-570: Expression is Always False

....
U16  max_ncq_depth;
....
SCI_STATUS scif_user_parameters_set(
   SCI_CONTROLLER_HANDLE_T   controller,
   SCIF_USER_PARAMETERS_T  * scif_parms
)
{
  ....
   if (scif_parms->sas.max_ncq_depth < 1 &&
       scif_parms->sas.max_ncq_depth > 32)
     return SCI_FAILURE_INVALID_PARAMETER_VALUE;
  ....
}

Предупреждение анализатора PVS-Studio: V547 Expression is always false. scif_sas_controller.c 531

Переменная не может быть одновременно меньше 1 и больше 32. Чтобы корректно проверить диапазон, нужно заменить оператор && на оператор ||.

Из-за ошибки функция не проверяет входные значения и может работать с некорректными данными.

Теперь более интересный случай. Для начала рассмотрим прототип функции LibAliasSetMode:

unsigned int LibAliasSetMode(.....);

Функция возвращает значение беззнакового типа. Функция, в случае возникновения внутренней ошибки, возвращается значение -1. Соответственно -1 превращается в UINT_MAX.

Теперь посмотрим, как используется эта функция.

static int
ng_nat_rcvmsg(node_p node, item_p item, hook_p lasthook)
{
  ....
  if (LibAliasSetMode(priv->lib, 
      ng_nat_translate_flags(mode->flags),
      ng_nat_translate_flags(mode->mask)) < 0) {
    error = ENOMEM;
    break;
  }
  ....
}

Предупреждение анализатора PVS-Studio: V547 Expression is always false. Unsigned type value is never < 0. ng_nat.c 374

Естественно, условие всегда ложно. Значение беззнакового типа не может быть меньше нуля.

Для людей, делающих обзор кода, такая ошибка будет плохо заметна. В случае ошибки функция возвращает -1. Проверка вида if (foo () < 0) есть. Создаётся впечатление, что всё хорошо.

Но неправильный тип функции всё портит. Вариантов исправления 2:

• Сделать, чтобы функция LibAliasSetMode возвращала тип signed int;
• Проверять результат работы функции, сравнивая возвращённое ей значение с UINT_MAX.

Как поступить лучше — решать разработчикам.

В следующем фрагменте, скорее всего, настоящей ошибки нет, и код просто избыточен. Хотя кто знает, я не разработчик и не знаю его замысел.

HAL_BOOL
ar9300_reset_tx_queue(struct ath_hal *ah, u_int q)
{
  u_int32_t cw_min, chan_cw_min, value;
  ....
  value = (ahp->ah_beaconInterval * 50 / 100)
    - ah->ah_config.ah_additional_swba_backoff
    - ah->ah_config.ah_sw_beacon_response_time
    + ah->ah_config.ah_dma_beacon_response_time;
  if (value < 10)
    value = 10;
  if (value < 0)
    value = 10;
  ....
}

Предупреждение анализатора PVS-Studio: V547 Expression 'value < 0' is always false. Unsigned type value is never < 0. ar9300_xmit.c 450

Попробуйте самостоятельно заметить ошибку вот здесь:

static void
dtrace_debug_output(void)
{
  ....
  if (d->first < d->next) {
    char *p1 = dtrace_debug_bufr;
    count = (uintptr_t) d->next - (uintptr_t) d->first;
    for (p = d->first; p < d->next; p++)
      *p1++ = *p;
  } else if (d->next > d->first) {
    char *p1 = dtrace_debug_bufr;
    count = (uintptr_t) d->last - (uintptr_t) d->first;
    for (p = d->first; p < d->last; p++)
      *p1++ = *p;
    count += (uintptr_t) d->next - (uintptr_t) d->bufr;
    for (p = d->bufr; p < d->next; p++)
      *p1++ = *p;
  }
  ....
}

Предупреждение анализатора PVS-Studio: V517 The use of 'if (A) {…} else if (A) {…}' pattern was detected. There is a probability of logical error presence. Check lines: 102, 109. dtrace_debug.c 102

Следует обратить внимание вот на эти две строчки:

if (d->first < d->next) {
} else if (d->next > d->first) {

Хотели написать другое условие, но не получилось. Поэтому второе условие всегда будет ложным.

CWE-571: Expression is Always True

int
mfi_tbolt_send_frame(struct mfi_softc *sc, struct mfi_command *cm)
{
  ....
  uint8_t *cdb;
  ....
  /* check for inquiry commands coming from CLI */
  if (cdb[0] != 0x28 || cdb[0] != 0x2A) {
    if ((req_desc = mfi_tbolt_build_mpt_cmd(sc, cm)) == NULL) {
      device_printf(sc->mfi_dev, "Mapping from MFI "
                                 "to MPT Failed \n");
      return 1;
    }
  }
  ....
}

Предупреждение анализатора PVS-Studio: V547 Expression 'cdb[0] != 0×28 || cdb[0] != 0×2A' is always true. Probably the '&&' operator should be used here. mfi_tbolt.c 1110

Условие (cdb[0] != 0×28 || cdb[0] != 0×2A) написано неправильно. Если байт равен значению 0×28, то он не может быть равен 0×2A. И наоборот. В результате условие всегда истинно.

Теперь рассмотрим два цикла, реализованных сложным и опасным образом.

static void
safe_mcopy(struct mbuf *srcm, struct mbuf *dstm, u_int offset)
{
  u_int j, dlen, slen;
  caddr_t dptr, sptr;

  /*
   * Advance src and dst to offset.
   */
  j = offset;
  while (j >= 0) {
    if (srcm->m_len > j)
      break;
    j -= srcm->m_len;
    srcm = srcm->m_next;
    if (srcm == NULL)
      return;
  }
  sptr = mtod(srcm, caddr_t) + j;
  slen = srcm->m_len - j;

  j = offset;
  while (j >= 0) {
    if (dstm->m_len > j)
      break;
    j -= dstm->m_len;
    dstm = dstm->m_next;
    if (dstm == NULL)
      return;
  }
  ....
}

Предупреждения анализатора PVS-Studio:

• V547 Expression 'j >= 0' is always true. Unsigned type value is always >= 0. safe.c 1596
• V547 Expression 'j >= 0' is always true. Unsigned type value is always >= 0. safe.c 1608

Обратите внимание, что переменная j имеет беззнаковый тип. Следовательно, проверка (j >= 0) не имеет смысла. С тем же успехом можно было написать while (true).

Я не знаю, может ли эта неправильная проверка вызвать сбой или циклы в любом случае будут корректно прерваны, благодаря наличию в их телах операторов break и return. Мне кажется, это настоящий баг и стоит изменить тип переменной j с u_int на int.

Если даже ошибки здесь нет, то код стоит переписать, чтобы он не сбивал с толку других разработчиков и не стал в дальнейшем причиной проблемы, когда кто-то будет изменять этот код.

Лично мне нравится ошибка, описанная ниже. Это красивая опечатка. Хотя нет, стоп, я же решил в этот раз говорить про CWE. Итак, перед нами красивая потенциальная уязвимость:

#define OSSA_MPI_ENC_ERR_ILLEGAL_DEK_PARAM            0x2001
#define OSSA_MPI_ERR_DEK_MANAGEMENT_DEK_UNWRAP_FAIL   0x2002

GLOBAL bit32 mpiDekManagementRsp(
  agsaRoot_t               *agRoot,
  agsaDekManagementRsp_t   *pIomb
  )
{
  ....
  if (status == OSSA_MPI_ENC_ERR_ILLEGAL_DEK_PARAM ||
      OSSA_MPI_ERR_DEK_MANAGEMENT_DEK_UNWRAP_FAIL)
  {
    agEvent.eq = errorQualifier;
  }
  ....
}

Предупреждение анализатора PVS-Studio: V560 A part of conditional expression is always true: 0×2002. sampirsp.c 7224

В условии один раз пропущена переменная status. Таким образом, значение статуса на самом деле не проверяется, и условие всегда истинно.

Рассмотрим ещё один аналогичный случай. Попробуйте, не читая описание, найти ошибку в функции ugidfw_rule_valid самостоятельно.

static int
ugidfw_rule_valid(struct mac_bsdextended_rule *rule)
{
  if ((rule->mbr_subject.mbs_flags | MBS_ALL_FLAGS) != MBS_ALL_FLAGS)
    return (EINVAL);
  if ((rule->mbr_subject.mbs_neg | MBS_ALL_FLAGS) != MBS_ALL_FLAGS)
    return (EINVAL);
  if ((rule->mbr_object.mbo_flags | MBO_ALL_FLAGS) != MBO_ALL_FLAGS)
    return (EINVAL);
  if ((rule->mbr_object.mbo_neg | MBO_ALL_FLAGS) != MBO_ALL_FLAGS)
    return (EINVAL);
  if ((rule->mbr_object.mbo_neg | MBO_TYPE_DEFINED) && 
      (rule->mbr_object.mbo_type | MBO_ALL_TYPE) != MBO_ALL_TYPE)
    return (EINVAL);
  if ((rule->mbr_mode | MBI_ALLPERM) != MBI_ALLPERM)
    return (EINVAL);
  return (0);
}

Тяжело?

Думаю, да. Вот поэтому статические анализаторы так важны. Они не зевают и не устают при просмотре подобных функций.

Предупреждение анализатора PVS-Studio: V617 Consider inspecting the condition. The '0×00000080' argument of the '|' bitwise operation contains a non-zero value. mac_bsdextended.c 128

Для начала, взглянем на макрос MBO_TYPE_DEFINED:

#define  MBO_TYPE_DEFINED 0x00000080

А теперь, смотрим вот сюда:

(rule->mbr_object.mbo_neg | MBO_TYPE_DEFINED)

Часть условия всегда истина. Если посмотреть код, расположенный рядом, то становится понятным, что на самом деле, хотели написать вот так:

(rule->mbr_object.mbo_neg | MBO_TYPE_DEFINED) != MBO_TYPE_DEFINED

Что-то статья всё не хочет заканчиваться. Придётся немного ускориться и сократить количество фрагментов кода. Так что просто знайте, что вижу ещё четыре CWE-571:

• V560 A part of conditional expression is always true: 0×7dac. t4_main.c 8001
• V547 Expression 'cfgflags >= 0 || cfgflags <= 3' is always true. hwpmc_piv.c 812
• V547 Expression 'cfgflags >= 0 || cfgflags <= 3' is always true. hwpmc_piv.c 838
• V501 There are identical sub-expressions 'G_Addr→g_addr.s_addr' to the left and to the right of the '==' operator. alias_sctp.c 2132

CWE-14: Compiler Removal of Code to Clear Buffers

int mlx5_core_create_qp(struct mlx5_core_dev *dev,
      struct mlx5_core_qp *qp,
      struct mlx5_create_qp_mbox_in *in,
      int inlen)
{
  ....
  struct mlx5_destroy_qp_mbox_out dout;
  ....
err_cmd:
  memset(&din, 0, sizeof(din));
  memset(&dout, 0, sizeof(dout));
  din.hdr.opcode = cpu_to_be16(MLX5_CMD_OP_DESTROY_QP);
  din.qpn = cpu_to_be32(qp->qpn);
  mlx5_cmd_exec(dev, &din, sizeof(din), &out, sizeof(dout));

  return err;
}

Предупреждение анализатора PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'dout' object. The memset_s () function should be used to erase the private data. mlx5_qp.c 159

Хотели обнулить структуру dout, содержащую приватные данные. Ошибка в том, что эта структура в дальнейшем не используется. Точнее, она используется вот здесь sizeof (dout), но это не считается. Поэтому компилятор удалит вызов функции memset.

Ещё одно неудачное обнуление структуры можно найт

© Habrahabr.ru