Geekly Articles each Day
📜 ⬆️ ⬇️

PVS-Studio rummaged through the guts of Linux (3.18.1)

Linux and PVS-Studio
Coauthor: Svyatoslav Razmyslov SvyatoslavMC .

For promotional purposes, we decided to try to test the Linux kernel using our static code analyzer. This task is interesting for its complexity. Source code Linux than just not checked and verified. Therefore, to find at least something new, a very difficult task. But if it works, it will be a good promotional note about the capabilities of the PVS-Studio analyzer.

What we tested


The Linux kernel was taken from The Linux Kernel Archives . Checked Latest Stable Kernel 3.18.1 .

By the time I write this article, the kernel 3.19-rc1 has already appeared. Unfortunately, checking a project and writing an article takes a lot of time. Therefore, we will be content to check not the latest version.
')
To those who say that we should have checked the latest version, I want to answer the following.
  1. We regularly check a large number of projects . In addition to free verification of projects, we have many other tasks. And so there is no way to start all over again, just because a new version has appeared. So we can never publish anything :).
  2. 99% of the errors found remained in its place. So this article can be safely used to slightly improve the Linux kernel code.
  3. The purpose of the article is to advertise PVS-Studio. If we can find errors in the draft version of X, then we can find something in version Y. Our checks are quite superficial (we are not familiar with the project) and their goal is to write such articles. The project benefits from the acquisition of a license for PVS-Studio and its regular use.

As we tested


To check the kernel, we used the PVS-Studio static code analyzer version 5.21.

To test the Linux Kernel, the Ubuntu-14.04 distribution was used, for which there were many detailed instructions for configuring and building the kernel. The analyzer checks preprocessed files that are better to get for successfully compiled files, so the project build is one of the most important stages of the check.

Next, a small C ++ utility was written, which for each running process of the compiler saved the command line, the current directory and environment variables. Readers familiar with PVS-Studio products should immediately recall the PVS-Studio Standalone utility, which allows you to test any project under Windows. There, WinAPI is used to access processes, so for Linux this monitoring mechanism was rewritten, the rest of the code associated with running preprocessing and testing was completely transferred, and testing the Linux kernel was only a matter of time.

At the beginning about safety


Somehow it happened that the PVS-Studio analyzer is perceived solely as a tool for finding errors. But no one pays attention that PVS-Studio can detect a number of vulnerabilities. Of course, we are to blame for this ourselves. It is necessary to straighten the situation a bit.

You can perceive the messages that PVS-Studio gives out in different ways. For example, on the one hand it is a typo, and on the other there may be a vulnerability. It all depends on the point of view.

I would like to suggest considering several warnings that PVS-Studio issued when analyzing Linux. I'm not saying that the analyzer found a vulnerability in Linux. But the warnings in question could easily do that.

Dangerous use of the memcmp () function


static unsigned char eprom_try_esi(
  struct atm_dev *dev, unsigned short cmd,
  int offset, int swap)
{
  unsigned char buf[ZEPROM_SIZE];
  struct zatm_dev *zatm_dev;
  int i;

  zatm_dev = ZATM_DEV(dev);
  for (i = 0; i < ZEPROM_SIZE; i += 2) {
    eprom_set(zatm_dev,ZEPROM_CS,cmd); /* select EPROM */
    eprom_put_bits(zatm_dev,ZEPROM_CMD_READ,ZEPROM_CMD_LEN,cmd);
    eprom_put_bits(zatm_dev,i >> 1,ZEPROM_ADDR_LEN,cmd);
    eprom_get_byte(zatm_dev,buf+i+swap,cmd);
    eprom_get_byte(zatm_dev,buf+i+1-swap,cmd);
    eprom_set(zatm_dev,0,cmd); /* deselect EPROM */
  }
  memcpy(dev->esi,buf+offset,ESI_LEN);
  return memcmp(dev->esi,"\0\0\0\0\0",ESI_LEN);
}

PVS-Studio: V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. zatm.c 1168

'return' .

'memcmp' 'int':
:: -100, 2, 3, 100, 256, 1024, 5555 . , 'unsigned char' ( , ).

, .

, . , 32- 64-.

? , -, EPROM. , , ?

, V642 ! ? , MySQL/MariaDB.
typedef char my_bool;
...
my_bool check(...) {
  return memcmp(...);
}

PVS-Studio . .

MySQL/MariaDB 5.1.61, 5.2.11, 5.3.5, 5.5.22. , MySQL /MariaDB (SHA ), 'memcmp'. [-128..127]. , 1 256 'true', . , bash MySQL, . 'sql/password.c', . : Security vulnerability in MySQL/MariaDB.

Linux. :
void sci_controller_power_control_queue_insert(....)
{
  ....
  for (i = 0; i < SCI_MAX_PHYS; i++) {
    u8 other;
    current_phy = &ihost->phys[i];
  
    other = memcmp(current_phy->frame_rcvd.iaf.sas_addr,
                   iphy->frame_rcvd.iaf.sas_addr,
                   sizeof(current_phy->frame_rcvd.iaf.sas_addr));

    if (current_phy->sm.current_state_id == SCI_PHY_READY &&
        current_phy->protocol == SAS_PROTOCOL_SSP &&
        other == 0) {
      sci_phy_consume_power_handler(iphy);
      break;
    }
  }
  ....
}

PVS-Studio: V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. host.c 1846

memcmp() other, unsigned char. , - , SCSI .

:

memset()


. , «» . . , . . : " — ?".

:
static int crypt_iv_tcw_whitening(....)
{
  ....
  u8 buf[TCW_WHITENING_SIZE];
  ....
  out:
  memset(buf, 0, sizeof(buf));
  return r;
}

PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'buf' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. dm-crypt.c 708

. crypt_iv_tcw_whitening() , - , memset(). , , memset() . C/C++ . .

, . - . ( Debug- memset).

. «- », . memset(). V597.

PVS-Studio RtlSecureZeroMemory(), Windows. Linux , , . , .

:
static int sha384_ssse3_final(struct shash_desc *desc, u8 *hash)
{
  u8 D[SHA512_DIGEST_SIZE];

  sha512_ssse3_final(desc, D);

  memcpy(hash, D, SHA384_DIGEST_SIZE);
  memset(D, 0, SHA512_DIGEST_SIZE);

  return 0;
}

PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'D' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sha512_ssse3_glue.c 222

, 4 : keydvt_out, keydvt_in, ccm_n, mic. security.c ( 525 — 528).
int wusb_dev_4way_handshake(....)
{
  ....
  struct aes_ccm_nonce ccm_n;
  u8 mic[8];
  struct wusb_keydvt_in keydvt_in;
  struct wusb_keydvt_out keydvt_out;
  ....
error_dev_update_address:
error_wusbhc_set_gtk:
error_wusbhc_set_ptk:
error_hs3:
error_hs2:
error_hs1:
  memset(hs, 0, 3*sizeof(hs[0]));
  memset(&keydvt_out, 0, sizeof(keydvt_out));
  memset(&keydvt_in, 0, sizeof(keydvt_in));
  memset(&ccm_n, 0, sizeof(ccm_n));
  memset(mic, 0, sizeof(mic));
  if (result < 0)
    wusb_dev_set_encryption(usb_dev, 0);
error_dev_set_encryption:
  kfree(hs);
error_kzalloc:
  return result;
  ....
}

, «» - :
int
E_md4hash(const unsigned char *passwd, unsigned char *p16,
  const struct nls_table *codepage)
{
  int rc;
  int len;
  __le16 wpwd[129];

  /* Password cannot be longer than 128 characters */
  if (passwd) /* Password must be converted to NT unicode */
    len = cifs_strtoUTF16(wpwd, passwd, 128, codepage);
  else {
    len = 0;
    *wpwd = 0; /* Ensure string is null terminated */
  }

  rc = mdfour(p16, (unsigned char *) wpwd, len * sizeof(__le16));
  memset(wpwd, 0, 129 * sizeof(__le16));

  return rc;
}

PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'wpwd' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. smbencrypt.c 224

. 3 memset() :


PVS-Studio V595, , , NULL. . :
static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n)
{
  struct net *net = sock_net(skb->sk);
  struct nlattr *tca[TCA_ACT_MAX + 1];
  u32 portid = skb ? NETLINK_CB(skb).portid : 0;
  ....
}

PVS-Studio: V595 The 'skb' pointer was utilized before it was verified against nullptr. Check lines: 949, 951. act_api.c 949

. 'skb' , . .

, , . . 0. , - .

. PVS-Studio , , . , , 0. .

. .

, , .
static int podhd_try_init(struct usb_interface *interface,
        struct usb_line6_podhd *podhd)
{
  int err;
  struct usb_line6 *line6 = &podhd->line6;

  if ((interface == NULL) || (podhd == NULL))
    return -ENODEV;
  ....
}

PVS-Studio: V595 The 'podhd' pointer was utilized before it was verified against nullptr. Check lines: 96, 98. podhd.c 96

, , , . .

podhd NULL. &podhd->line6. . . . , 'line6' . « ». . , ? 'podhd', . 'line6' , .

, ! . .

. : podhd->line6. , . . , .

:
if ((interface == NULL) || (podhd == NULL))
  return -ENODEV;

. , 'podhd' . :
if ((interface == NULL))
  return -ENODEV;

memset() , Debug() . .

, , (-ENODEV), . .

. . .. , , . , . , - . , , .

:
int wpa_set_keys(struct vnt_private *pDevice, void *ctx,
     bool fcpfkernel) __must_hold(&pDevice->lock)
{
  ....
  if (is_broadcast_ether_addr(&param->addr[0]) ||
      (param->addr == NULL)) {
  ....
}

PVS-Studio: V713 The pointer param->addr was utilized in the logical expression before it was verified against nullptr in the same logical expression. wpactl.c 333

:
if (is_broadcast_ether_addr(&param->addr[0]))

Linux . V595 200 . , . - . : Linux-V595.txt.

, . . , . .


, , , . , .


void b43legacy_phy_set_antenna_diversity(....)
{
  ....
  if (phy->rev >= 2) {
    b43legacy_phy_write(
      dev, 0x0461, b43legacy_phy_read(dev, 0x0461) | 0x0010);
    ....
  } else if (phy->rev >= 6)
    b43legacy_phy_write(dev, 0x049B, 0x00DC);
  ....
}

PVS-Studio: V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 2147, 2162. phy.c 2162

. :
if ( A >= 2)
  X();
else if ( A >= 6)
  Y();

, 'A', Y().

. .
static int __init scsi_debug_init(void)
{
  ....
  if (scsi_debug_dev_size_mb >= 16)
    sdebug_heads = 32;
  else if (scsi_debug_dev_size_mb >= 256)
   sdebug_heads = 64;
  ....
}

PVS-Studio: V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 3858, 3860. scsi_debug.c 3860

static ssize_t ad5933_store(....)
{
  ....
  /* 2x, 4x handling, see datasheet */
  if (val > 511)
    val = (val >> 1) | (1 << 9);
  else if (val > 1022)
    val = (val >> 2) | (3 << 9);
  ....
}

PVS-Studio: V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 439, 441. ad5933.c 441

, , :.
static int dgap_parsefile(char **in)
{
  ....
  int module_type = 0;
  ....
  module_type = dgap_gettok(in);
  if (module_type == 0 || module_type != PORTS ||
      module_type != MODEM) {
    pr_err("failed to set a type of module");
    return -1;
  }
  ....
}

PVS-Studio: V590 Consider inspecting the 'module_type == 0 || module_type != 68' expression. The expression is excessive or contains a misprint. dgap.c 6733

, . . :

« »


name_msi_vectors(). , . , .
static void name_msi_vectors(struct ipr_ioa_cfg *ioa_cfg)
{
  int vec_idx, n = sizeof(ioa_cfg->vectors_info[0].desc) - 1;

  for (vec_idx = 0; vec_idx < ioa_cfg->nvectors; vec_idx++) {
    snprintf(ioa_cfg->vectors_info[vec_idx].desc, n,
       "host%d-%d", ioa_cfg->host->host_no, vec_idx);
    ioa_cfg->vectors_info[vec_idx].
      desc[strlen(ioa_cfg->vectors_info[vec_idx].desc)] = 0;
  }
}

: V692 An inappropriate attempt to append a null character to a string. To determine the length of a string by 'strlen' function correctly, a string ending with a null terminator should be used in the first place. ipr.c 9409

:
ioa_cfg->vectors_info[vec_idx].
  desc[strlen(ioa_cfg->vectors_info[vec_idx].desc)] = 0;

, - :
S[strlen(S)] = 0;

. , . , - .


static int ql_wait_for_drvr_lock(struct ql3_adapter *qdev)
{
  int i = 0;

  while (i < 10) {
    if (i)
      ssleep(1);

    if (ql_sem_lock(qdev,
        QL_DRVR_SEM_MASK,
        (QL_RESOURCE_BITS_BASE_CODE | (qdev->mac_index)
         * 2) << 1)) {
      netdev_printk(KERN_DEBUG, qdev->ndev,
              "driver lock acquired\n");
      return 1;
    }
  }

  netdev_err(qdev->ndev,
             "Timed out waiting for driver lock...\n");
  return 0;
}

PVS-Studio: V654 The condition 'i < 10' of loop is always true. qla3xxx.c 149

. , 1 . 10 .

, , . , 'i' .


static int find_boot_record(struct NFTLrecord *nftl)
{
  ....
  if ((ret = nftl_read_oob(mtd, block * nftl->EraseSize +
                           SECTORSIZE + 8, 8, &retlen,
                           (char *)&h1) < 0) ) {
    printk(KERN_WARNING "ANAND header found at 0x%x in mtd%d, "
           "but OOB data read failed (err %d)\n",
           block * nftl->EraseSize, nftl->mbd.mtd->index, ret);
    continue;
  ....
}

PVS-Studio: V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. nftlmount.c 92

, . . , , (err 0) (err 1), .

— . nftl_read_oob() 'ret'. 0. (ret < 0), .

, . nftl_read_oob() 0. 0 1. 'ret'.

, nftl_read_oob() , ret == 1. , .

, . , 'if' . , — . :
if ((ret = nftl_read_oob(mtd, block * nftl->EraseSize +
                         SECTORSIZE + 8, 8, &retlen,
                         (char *)&h1)) < 0 ) {


int wl12xx_acx_config_hangover(struct wl1271 *wl)
{
  ....
  acx->recover_time = cpu_to_le32(conf->recover_time);
  acx->hangover_period = conf->hangover_period;
  acx->dynamic_mode = conf->dynamic_mode;
  acx->early_termination_mode = conf->early_termination_mode;
  acx->max_period = conf->max_period;
  acx->min_period = conf->min_period;
  acx->increase_delta = conf->increase_delta;
  acx->decrease_delta = conf->decrease_delta;
  acx->quiet_time = conf->quiet_time;
  acx->increase_time = conf->increase_time;
  acx->window_size = acx->window_size;         <<<---
  ....
}

PVS-Studio: V570 The 'acx->window_size' variable is assigned to itself. acx.c 1728

. :
acx->window_size = acx->window_size;

? ? .


static const struct XGI330_LCDDataDesStruct2  XGI_LVDSNoScalingDesData[] = {
  {0,  648,  448,  405,  96, 2}, /* 00 (320x200,320x400,
                                        640x200,640x400) */
  {0,  648,  448,  355,  96, 2}, /* 01 (320x350,640x350) */
  {0,  648,  448,  405,  96, 2}, /* 02 (360x400,720x400) */
  {0,  648,  448,  355,  96, 2}, /* 03 (720x350) */
  {0,  648,    1,  483,  96, 2}, /* 04 (640x480x60Hz) */
  {0,  840,  627,  600, 128, 4}, /* 05 (800x600x60Hz) */
  {0, 1048,  805,  770, 136, 6}, /* 06 (1024x768x60Hz) */
  {0, 1328,    0, 1025, 112, 3}, /* 07 (1280x1024x60Hz) */
  {0, 1438,    0, 1051, 112, 3}, /* 08 (1400x1050x60Hz)*/
  {0, 1664,    0, 1201, 192, 3}, /* 09 (1600x1200x60Hz) */
  {0, 1328,    0, 0771, 112, 6}  /* 0A (1280x768x60Hz) */
                  ^^^^
                  ^^^^
};

PVS-Studio: V536 Be advised that the utilized constant value is represented by an octal form. Oct: 0771, Dec: 505. vb_table.h 1379

. : 0771. . .

, , . .


static void sig_ind(PLCI *plci)
{
  ....
  byte SS_Ind[] =
    "\x05\x02\x00\x02\x00\x00"; /* Hold_Ind struct*/
  byte CF_Ind[] =
    "\x09\x02\x00\x06\x00\x00\x00\x00\x00\x00";
  byte Interr_Err_Ind[] =
    "\x0a\x02\x00\x07\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00";
  byte CONF_Ind[] =
    "\x09\x16\x00\x06\x00\x00\0x00\0x00\0x00\0x00";
                              ^^^^^^^^^^^^^^^^^^^
  ....
}

PVS-Studio: V638 A terminal null is present inside a string. The '\0x00' characters were encountered. Probably meant: '\x00'. message.c 4883

. CONF_Ind[]. «x00». , , , :
byte CONF_Ind[] =
  "\x09\x16\x00\x06\x00\x00\x00\x00\x00\x00";

.. '0' 'x' . «x00» , .


static int grip_xt_read_packet(....)
{
  ....
  if ((u ^ v) & 1) {
    buf = (buf << 1) | (u >> 1);
    t = strobe;
    i++;
  } else

  if ((((u ^ v) & (v ^ w)) >> 1) & ~(u | v | w) & 1) {
  ....
}

PVS-Studio: V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. grip.c 152

, . . . .


static s32 snto32(__u32 value, unsigned n)
{
  switch (n) {
  case 8:  return ((__s8)value);
  case 16: return ((__s16)value);
  case 32: return ((__s32)value);
  }
  return value & (1 << (n - 1)) ? value | (-1 << n) : value;
}

PVS-Studio: V610 Undefined behavior. Check the shift operator '<<. The left operand '-1' is negative. hid-core.c 1016

. . , , " , . ( )".

: « !». . , , Linux , . .

, : Linux-V610.txt.

enum


enum:
enum iscsi_param {
  ....
  ISCSI_PARAM_CONN_PORT,
  ISCSI_PARAM_CONN_ADDRESS,        <<<<----
  ....
};

enum iscsi_host_param {
  ISCSI_HOST_PARAM_HWADDRESS,
  ISCSI_HOST_PARAM_INITIATOR_NAME,
  ISCSI_HOST_PARAM_NETDEV_NAME,
  ISCSI_HOST_PARAM_IPADDRESS,       <<<<----
  ISCSI_HOST_PARAM_PORT_STATE,
  ISCSI_HOST_PARAM_PORT_SPEED,
  ISCSI_HOST_PARAM_MAX,
};

ISCSI_PARAM_CONN_ADDRESS ISCSI_HOST_PARAM_IPADDRESS. . , .

:
int iscsi_conn_get_addr_param(
  struct sockaddr_storage *addr,
  enum iscsi_param param, char *buf)
{
  ....
  switch (param) {
  case ISCSI_PARAM_CONN_ADDRESS:
  case ISCSI_HOST_PARAM_IPADDRESS:        <<<<----
  ....
  case ISCSI_PARAM_CONN_PORT:
  case ISCSI_PARAM_LOCAL_PORT:
  ....
  default:
    return -EINVAL;
  }

  return len;
}

PVS-Studio: V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. libiscsi.c 3501

ISCSI_HOST_PARAM_IPADDRESS enum iscsi_param. , ISCSI_PARAM_CONN_ADDRESS.

PVS-Studio:


. , , . .
void pvr2_encoder_cmd ()
{
  do {
    ....
    if (A) break;
    ....
    if (B) break;
    ....
    if (C) continue;
    ....
    if (E) break;
    ....
  } while(0);
}

1 . , , goto. - , 'break', , .

, 'continue', 'break'. 'continue', 'break'. .

:

§6.6.2 in the standard: «The continue statement (...) causes control to pass to the loop-continuation portion of the smallest enclosing iteration-statement, that is, to the end of the loop.» (Not to the beginning.)

, 'continue' (0), .

2 .
  1. . 'continue' . 'break', , .
  2. 'continue' . .

Copy-Paste


void dm_change_dynamic_initgain_thresh(
  struct net_device *dev, u32 dm_type, u32 dm_value)
{
  ....
  if (dm_type == DIG_TYPE_THRESH_HIGH)
  {
    dm_digtable.rssi_high_thresh = dm_value;
  }
  else if (dm_type == DIG_TYPE_THRESH_LOW)
  {
    dm_digtable.rssi_low_thresh = dm_value;
  }
  else if (dm_type == DIG_TYPE_THRESH_HIGHPWR_HIGH)      <<--
  {                                                      <<--
    dm_digtable.rssi_high_power_highthresh = dm_value;   <<--
  }                                                      <<--
  else if (dm_type == DIG_TYPE_THRESH_HIGHPWR_HIGH)      <<--
  {                                                      <<--
    dm_digtable.rssi_high_power_highthresh = dm_value;   <<--
  }                                                      <<--
  ....
}

PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1755, 1759. r8192U_dm.c 1755

Copy-Paste ::


, . , .
static int saa7164_vbi_fmt(struct file *file, void *priv,
                           struct v4l2_format *f)
{
  /* ntsc */
  f->fmt.vbi.samples_per_line = 1600;           <<<<----
  f->fmt.vbi.samples_per_line = 1440;           <<<<----
  f->fmt.vbi.sampling_rate = 27000000;
  f->fmt.vbi.sample_format = V4L2_PIX_FMT_GREY;
  f->fmt.vbi.offset = 0;
  f->fmt.vbi.flags = 0;
  f->fmt.vbi.start[0] = 10;
  f->fmt.vbi.count[0] = 18;
  f->fmt.vbi.start[1] = 263 + 10 + 1;
  f->fmt.vbi.count[1] = 18;
  return 0;
}

PVS-Studio: V519 The 'f->fmt.vbi.samples_per_line' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1001, 1002. saa7164-vbi.c 1002
static int saa7164_vbi_buffers_alloc(struct saa7164_port *port)
{
  ....
  /* Init and establish defaults */
  params->samplesperline = 1440;
  params->numberoflines = 12;                           <<<<----
  params->numberoflines = 18;                           <<<<----
  params->pitch = 1600;                                 <<<<----
  params->pitch = 1440;                                 <<<<----
  params->numpagetables = 2 +
    ((params->numberoflines * params->pitch) / PAGE_SIZE);
  params->bitspersample = 8;
   ....
}

:


- . Linux . , . , , .

, , . !

Linux and PVS-Studio

PVS-Studio . Windows. - Linux , , PVS-Studio .


, : Andrey Karpov. PVS-Studio dives into Linux insides (3.18.1).

?
. : PVS-Studio CppCat, 2014. , .

Source: https://habr.com/ru/post/247271/

More articles:


All Articles