PVS-Studio dives into Linux insides

We took Linux kernel from The Linux Kernel Archives. Latest Stable Kernel 3.18.1 was checked.

While I wrote this article, kernel 3.19-rc1 was already developed. Unfortunately, project check and article writing take a lot of time. That is why it is fine to be satisfied with checking of non-last version.

I want to answer to those who would say we should have checked last version.

• We check large amount of projects on a regular basis. We have many tasks besides free project checking. This is why there are no possibility to start again just because new version was released. This way we would not be able to publish anything :).
• 99% of errors is still on place. One can use this article to improve Linux kernel slightly.
• Goal of this article is to advertise PVS-Studio. If we are able to find errors in version X, we would be able to find something in version Y. Our checks are shallow (we are not familiar with this project a lot) and their purpose is to write articles similar to this one. Real benefit is to buy PVS-Studio license and to use it on a regular basis.

The way we checked

We used PVS-Studio 5.21 static code analyzer to check Linux kernel.

Ubuntu-14.04 was used, which haves a lot of detailed instructions of configuring and building the kernel. Analyzer checks preprocessed files, which is much better to get from files successfully compiled, so building is one of the most important stage of checking.

Next, small utility program was written on C++ which purpose is to save all the environment variables, command line arguments and current directory for every compiler process call. Readers that are familiar with PVS-Studio product should remember PVS-Studio Standalone that allows checking any project under Windows. WinAPI there was used, so monitoring mechanics was rewritten for Linux, the rest of the code that is responsible for preprocessing and check has fully survived, so Linux kernel check became just a matter of time.

First of all about security

It became a tradition that PVS-Studio analyzer is recognized as a tool for searching errors. However, nobody pays attention that PVS-Studio is able to find some vulnerabilities. Of course, we have a guilt for that. We need to correct this situation.

It is possible to perceive all the messages given by PVS-Studio in different ways. For instance, from one point of view, it is a typo; from another it can be a vulnerability. Everything depends on point of view.

I want to propose to review messages generated by PVS-Studio while analyzing Linux. I am not proclaiming that analyzer has found a vulnerability in Linux. However, these warnings can possibly do that.

Dangerous memcmp() usage

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 warning: 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

Look at 'return' operator at the end of this function.

'memcmp' function returns next values of 'int' type:

• < 0 - buf1 less than buf2;
• 0 - buf1 identical to buf2;
• > 0 - buf1 greater than buf2;

Look:

• "> 0" means any numbers, not only 1;
• "< 0" is not only -1.

Returned values may be -100, 2, 3, 100, 256, 1024, 5555 and so on. That means that it is forbidden to cast result into 'unsigned char' type (type that is returned by function).

Implicit cast may truncate significant bits that would alter program execution logic.

Danger of errors of this kind is that error may depend on architecture and on realization of this function on this architecture. For example, this program may run correctly on 32-bit architecture and incorrectly on 64-bit architecture.

So what? Some check would be invalid while working with EPROM. Of course, it is an error. What about the vulnerability?

That is what. Diagnostic V642 is able to find vulnerability. Would you believe that? Here is the identical code from MySQL/MariaDB.

typedef char my_bool;
...
my_bool check(...) {
  return memcmp(...);
}

Not PVS-Studio has found this error. However, it could have been.

This error caused a serious vulnerability in MySQL/MariaDB unless versions 5.1.61, 5.2.11, 5.3.5, 5.5.22.

The idea is while user connects to MySQL/MariaDB, the token (SHA from password and hash) is evaluated and then compared with expected value by using 'memcmp' function. On some platforms this value can exceed [-128..127] range. So in one case out of 256 hash comparing procedure would always return true no matter what. Simple bash script gives an intruder root access to vulnerable MySQL server even if he or she does not know root password. The reason is the code in 'sql/password.c' shown above. You can read the description of this problem in details here: Security vulnerability in MySQL/MariaDB.

Let us go back to Linux. Here is another dangerous code fragment:

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 warning: 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() result is put into 'other' variable of unsigned char type. I do not think that it is a vulnerability, but work with SCSI controller is dangerous.

Another pair of code fragments like this can be found here:

• 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
• 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 1789

Dangerous memset() usage

Let us continue to find dangerous places. Take a look on functions that "erases" private data after usage. Usually it is some cryptographic functions. Unfortunately, memory zeroing is not always correct. It is possible to receive very unpleasant results this way. You can read the description of this problem in details in this article: "Overwriting memory - why?".

Consider reviewing the example of incorrect code:

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

PVS-Studio warning: 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

At first glance, everything is good. crypt_iv_tcw_whitening() function allocated temporary buffer on stack, enciphered something and then zeroed buffer containing private data using memset() function call. However, memset() function call would be removed by compiler in terms of optimization. From point of view of C/C++ language, it is not used after zeroing, so buffer may not be overwritten.

Herewith this error is very hard to detect. Unit-test is very difficult to cover this case. This error cannot be spotted under debugger too (in Debug-version memset function call would be present anyway).

I want to stress that it is not a "theoretically possible compiler behavior", but very real one. Compilers really delete memset() function call. You can read the description of this problem in details in the description of V597 warning.

PVS-Studio incorrectly recommends to use RtlSecureZeroMemory() function because of Windows orientation. Of course, Linux does not have this function. However, the most important is to warn, finding desired function is not that difficult.

Another similar example:

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 warning: 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

There are an example below where four buffers may not be zeroed: keydvt_out, keydvt_in, ccm_n, and mic. Code was taken from security.c file (lines 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;
  ....
}

And the last example when some kind of password may be left loose in memory:

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 warning: 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

I would stop here. Another three ineffectual memset() calls could be found here:

• sha256_ssse3_glue.c 214
• dev-sysfs.c 104
• qp.c 143

Dangerous checks

PVS-Studio analyzer have V595 warning that detects errors when pointer is dereferenced first and then checked for NULL. Sometimes all is clear about this warning. Let us check this simple case:

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 warning: V595 The 'skb' pointer was utilized before it was verified against nullptr. Check lines: 949, 951. act_api.c 949

Everything is simple here. When pointer 'skb' is null, the fat is in the fire. The pointer is dereferenced in the first line.

It is worth mentioning that analyzer warns not because it found non-checked pointer dereferencing. This way there would have been many false alarms. Function argument may not always be equal to zero. It is possible that this check was made somewhere else.

Logic goes as follows. PVS-Studio considers code dangerous if pointer is dereferenced and then checked. If programmer checks pointer, he or she assumes that is can be zero. Therefore, it is a reason to warn.

OK, we dealt with this simple example. However, we are interested not in this one.

Let us proceed to more difficult case, which is related to compiler optimizations.

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 warning: V595 The 'podhd' pointer was utilized before it was verified against nullptr. Check lines: 96, 98. podhd.c 96

This is a code example that people start argue about. They think this way.

Let 'podhd' pointer be NULL. Expression &podhd->line6 is not pretty, but still there are no error. There is no access to the memory. Address of one of structure member is evaluated. Well, 'line6' pointer value is incorrect because it points to "nowhere". However, this pointer is not used. We have evaluated wrong address. So what? Below there is a check and if 'podhd' is NULL, function will stop its work. Pointer 'line6' is not used, so there would be no error in real life.

Gentlemen, you are not right. It is incorrect anyway. Do not be lazy and fix this code.

In its turn, compiler thinks this way. Pointer is dereferenced here: podhd->line6. OK, programmer knows what he is doing. Therefore, pointer here is not equal to NULL. Good, let us remember that.

Next, compiler finds a check:

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

Then compiler optimizes a check. It thinks that 'podhd' is not NULL. Therefore, check would be reduced to:

if ((interface == NULL))
  return -ENODEV;

As of memset(), additional difficulty is in the fact that we would not see an absence of this check in debug version. This error is very difficult to spot.

In result, when NULL pointer is passed to function, it would not return -ENODEV status, but would resume its work. Results may be unexpected.

Next thing is important. Compiler may delete an important check from badly written code. I.e. there are functions that is only believed to check pointers. Actually, they would work with NULL pointed. I do not know could it be used to exploit something. However, from my point of view, one can see this situation as a potential vulnerability.

Another similar example:

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 warning: 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

Compiler, while optimizing, can reduce check:

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

Linux kernel is huge. There were almost 200 V595 warnings. My shame that I was too lazy to check them and used only one example for article. I ask some developer to check other suspicious places. There are presented as list: Linux-V595.txt.

Yes, not all these warnings are real errors. In most cases pointer could not be NULL at all. However, this list worth checking. It is almost certain that somebody can find couple dozens of errors.

Suspicious places found

It is possible that not all the code fragments in this article contain real error. However, they are very suspicious and require close look.

Incorrect logical conditions

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 warning: V695 Range intersections are possible within conditional expressions. Example: if (A < 5) { ... } else if (A < 2) { ... }. Check lines: 2147, 2162. phy.c 2162

Second condition would never be performed. Let us simplify the code for demonstrative purposes:

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

As you can see, there are no 'A' value with which Y() function can be called.

We found some other similar cases. They does not require any clarifications.

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 warning: 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 warning: V695 Range intersections are possible within conditional expressions. Example: if (A < 5) { ... } else if (A < 2) { ... }. Check lines: 439, 441. ad5933.c 441

There are couple of similar cases I would only mention to make article not that long:

• V695 Range intersections are possible within conditional expressions. Example: if (A < 5) { ... } else if (A < 2) { ... }. Check lines: 1417, 1422. bnx2i_hwi.c 1422
• V695 Range intersections are possible within conditional expressions. Example: if (A < 5) { ... } else if (A < 2) { ... }. Check lines: 4815, 4831. stv090x.c 4831

Now let us look at another kind of suspicious conditions.

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 warning: V590 Consider inspecting the 'module_type == 0 || module_type != 68' expression. The expression is excessive or contains a misprint. dgap.c 6733

I am not familiar with the code and I have no idea how this check should have look like. Therefore, I would not make any comments. Here is another similar place:

• V590 Consider inspecting the 'conc_type == 0 || conc_type != 65' expression. The expression is excessive or contains a misprint. dgap.c 6692

"Rip out eyes"

I have encountered name_msi_vectors() function while studying analyzer warnings. While this function is short, I did not want to read it. Apparently, that is why it contains a very suspicious line.

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

PVS-Studio warning: 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

Suspicious line is the last one:

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

I shall simplify it and weirdness should become clear:

S[strlen(S)] = 0;

This action is pointless. Zero would be written when it is already present. There are suspicion that programmer wanted to do something different.

Eternal wait

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 warning: V654 The condition 'i < 10' of loop is always true. qla3xxx.c 149

Function tries to lock driver. If unsuccessful, it waits one second and then tries again. There should be ten tries overall.

However, in fact there would be unlimited amount of tries. The reason is that 'i' variable is not incremented anywhere.

Wrong information about error

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 warning: V593 Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'. nftlmount.c 92

If an error occurs, function should print an information about it, including error code. However, (err 0) or (err 1) would be printed, but not actual error code.

Reason is that programmer messed up with operations priority. First of all, he or she wanted to put result of nftl_read_oob() into 'ret' variable. Then he or she wanted to compare this variable with zero. If (ret < 0), it is required to print error code.

Actually, this code works different way. In the beginning result of nftl_read_oob() function is compared with zero. Result is value of 0 or 1. This value would be put into 'ret' variable.

Therefore, if function ntfl_read_oob() returned negative value, then ret == 1. There would be an error message, but wrong one.

It is clearly seen that additional parenthesis are used in condition. It is unknown was they written to suppress compiler warning about assignment inside of 'if' or to implicitly point to operations execution order. If second, we are dealing with misprint - closing parenthesis is misplaced. Correct code:

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

Misprint suspicion

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 warning: V570 The 'acx->window_size' variable is assigned to itself. acx.c 1728

Fields of one structure is copied everywhere in fields of another structure. However, in one place in all of a sudden:

acx->window_size = acx->window_size;

Is it an error? Is it the way programmer wanted to? I cannot answer this question.

Suspicious octal number

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 warning: V536 Be advised that the utilized constant value is represented by an octal form. Oct: 0771, Dec: 505. vb_table.h 1379

All the numbers in this structure is in base 10. However, suddenly we encountered one number in base 8: 0771. It alerted the analyzer. Me too.

There is suspicion that this zero is present here to align column in a fancy way. However, it is clearly incorrect value then.

Suspicious string

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 warning: V638 A terminal null is present inside a string. The '\0x00' characters were encountered. Probably meant: '\x00'. message.c 4883

There are some magic numbers in arrays. Contents of CONF_Ind[] array is suspicious. Zero symbols with "x00" text alternating each other. I believe that it is a misprint and, actually, code should be written this way:

byte CONF_Ind[] =
  "\x09\x16\x00\x06\x00\x00\x00\x00\x00\x00";

I.e. '0' before 'x' is excessive and is written accidently. In result, "x00" is interpreted as text but not as symbol code.

Suspicious code formatting

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 warning: V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. grip.c 152

I do not think there is an error. However, code formatting is bad, that is why I put it into article. It is better to check it once again.

Undefined behavior in shift operations

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 warning: V610 Undefined behavior. Check the shift operator '<<. The left operand '-1' is negative. hid-core.c 1016

Negative operation shifting leads to undefined behavior. I wrote about this many times and would not explain it in details again. For ones who do not know what is wrong here I suggest to read an article "Wade not in unknown waters. Part three".

I foresee an objection like "but it works!" Well, it may work. Nevertheless, I believe that Linux kernel is not a place when it is possible to rely on this approach. It could be better to rewrite the code.

Shifts like this are frequent, so I gathered them into file: Linux-V610.txt.

enum confusion

There are two enums:

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

Look at constants ISCSI_PARAM_CONN_ADDRESS and ISCSI_HOST_PARAM_IPADDRESS. Their names are similar. It seems like this fact is the reason to confusion.

Let us check this code fragment:

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 warning: V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. libiscsi.c 3501

ISCSI_HOST_PARAM_IPADDRESS constant does not belong to enum iscsi_param. Most probably, it is an error and ISCSI_PARAM_CONN_ADDRESS should be used instead.

Similar PVS-Studio warnings:

• V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. svm.c 1360
• V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. vmx.c 2690
• V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. request.c 2842
• V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. request.c 2868

Strange cycle

I would not be able to show code fragment here. It is huge and I do not know how to shrink it and format it prettily. Therefore, I shall write a pseudo code.

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

This cycle runs one time. There is a suspicion that this construction was made to omit goto operator. When something went wrong, break operator is called and code after the cycle is executed.

I am confused that one place contains 'continue' operator instead of 'break'. Herewith 'continue' operator works the same as 'break'. Let me clarify.

This is what standard says:

§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.)

Therefore, after 'continue' call condition (0) would be checked and cycle would end because condition is false.

There are two variants possible.

• Code is correct. 'continue' operator should break the cycle. In this scenario, I recommend to replace it with 'break' to add uniformity for developers that would work with this code in the future.
• Developer wanted that 'continue' operator would make another cycle operation. Then this code is incorrect and should be rewritten.

Copy-Paste error

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 warning: 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

This code was written using Copy-Paste technique and somebody forgot to replace

• DIG_TYPE_THRESH_HIGHPWR_HIGH to DIG_TYPE_THRESH_HIGHPWR_LOW
• rssi_high_power_highthresh to rssi_high_power_lowthresh

In addition, I ask developers to look here:

• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1670, 1672. rtl_dm.c 1670
• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 530, 533. ioctl.c 530

Additional initialization

There are strange places where somebody assigned different values to the same variable. I think it worth to check those code fragments.

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 warning: 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;
   ....
}

Warnings:

• V519 The 'params->numberoflines' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 118, 119. saa7164-vbi.c 119
• V519 The 'params->pitch' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 120, 121. saa7164-vbi.c 121

Conclusion

It is possible to find errors in any large project. Linux kernel is not an exception. However, once-only static code analyzer checks is incorrect way to use it. Yes, they allow writing this article, but give little benefit to the project.

Use static code analyzer regularly, and you will save a lot of time by finding errors at the earliest stage. Defend your project against bugs using static code analyzer!