Andrey2008 January 3, 2015 at 15:27

PVS-Studio rummaged in Linux internals (3.18.1)

Co-author: Svyatoslav Razmyslov SvyatoslavMC .

For advertising purposes, we decided to try checking the Linux kernel using our static code analyzer. This task is interesting for its complexity. Linux source codes have not been tested and verified. Therefore, finding at least something new is a very difficult task. But if it works out, it will be a good advertising note about the capabilities of the PVS-Studio analyzer.

What did we check

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 with checking not the latest version.

To those who say that we should have checked the very latest version, I want to answer the following.

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 :).
99% of the errors found remained in their place. So this article can be safely used to slightly improve the Linux kernel code.
The purpose of the article is PVS-Studio advertising. If we can find errors in the draft version X, then we can find something in version Y as well. Our checks are quite superficial (we are not familiar with the project) and their goal is to write such articles. The real benefit to the project is the acquisition of a license for PVS-Studio and its regular use.

As we checked

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

To test Linux Kernel, the Ubuntu-14.04 distribution kit was used, for which there were many detailed instructions for configuring and building the kernel. The analyzer checks the preprocessed files , which are best received for successfully compiled files, so building a project is one of the most important stages of verification.

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

Security in the beginning

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

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

I would like to suggest considering a few 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 well have done this.

Dangerous use of 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 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

Note the 'return' statement at the very end of the function body.

The 'memcmp' function returns the following values of type 'int':

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

Note:

"> 0" means any numbers, not 1 at all;
"<0", this is not necessarily -1.

The returned values can be: -100, 2, 3, 100, 256, 1024, 5555, and so on. This means that this result cannot be converted to the type 'unsigned char' (this is the type that the function returns).

As a result of an implicit type conversion, significant bits can be discarded, which will lead to a violation of the program execution logic.

The danger of such errors is that the return value may depend on the architecture and implementation of a particular function on this architecture. For example, the program will work correctly in the 32-bit version and incorrectly in 64-bit.

So what? Just think, something related to EPROM will be checked incorrectly. An error, of course, but what does the vulnerability have to do with it?

And the fact that V642 diagnostics can reveal a vulnerability! Do not believe? Please, here is the identical code from MySQL / MariaDB.

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

Not PVS-Studio found this error. But he could.

This error caused a serious vulnerability in MySQL / MariaDB prior to versions 5.1.61, 5.2.11, 5.3.5, 5.5.22. The bottom line is that when a MySQL / MariaDB user connects, a token (SHA from password and hash) is calculated, which is compared with the expected value of the 'memcmp' function. On some platforms, the return value may fall outside the range [-128..127]. As a result, in 1 case out of 256, the hash comparison procedure with the expected value always returns the value 'true', regardless of the hash. As a result, a simple bash command gives the attacker root access to the vulnerable MySQL server, even if he does not know the password. The reason for this was the code in the 'sql / password.c' file shown above. A more detailed description of this problem can be found here:Security vulnerability in MySQL / MariaDB .

Let's get back to Linux now. Here is another dangerous piece of code:

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

The result of the memcmp () function is placed in the other variable, which is of type unsigned char. I do not think this is any kind of vulnerability, but the work of the SCSI controller is in danger.

A couple more of these places 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 use of the memset () function

We continue to search for dangerous places. Let us turn our attention to functions that “clean up” private data. These are usually various encryption features. Unfortunately, memory reset is not always written correctly. And then you can get an extremely unpleasant result. You can learn more about these unpleasant moments from this article: " Overwriting memory - why? ".

Consider 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 fine. The crypt_iv_tcw_whitening () function allocated a temporary buffer on the stack, encrypted something, and then reset the buffer with private data by calling memset (). But, in fact, the memset () function call will be deleted by the compiler during optimization. From the point of view of C / C ++, after zeroing the buffer, it is not used in any way. So the buffer can not be nullified.

At the same time, it is extremely difficult to notice such an error. Writing a unit test is difficult. Under the debugger, such an error will not be visible either (in the Debug version there will be a call to the memset function).

At the same time I want to emphasize. This is not “theoretically possible compiler behavior”, but very practical. Compilers really remove the memset () function call. You can read more about this in the description of the V597 diagnostic .

PVS-Studio inappropriately recommends using the RtlSecureZeroMemory () function, as it is oriented to Windows. On Linux, of course, there is no such function. But the main thing here is to warn, and choosing the right function is no longer difficult.

The following is a 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

And below is an example where 4 buffers may not be immediately reset: keydvt_out, keydvt_in, ccm_n, mic. The code is taken from the 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, where some kind of password remains in the 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 will dwell on this. 3 more unsuccessful memset () can be found here:

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

Dangerous Checks

The PVS-Studio analyzer has a V595 diagnostic, which detects situations when the pointer is dereferenced at the beginning and then checked for equality NULL. Sometimes with this diagnosis, everything is simple and clear. Consider 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. If the 'skb' pointer is zero, then be in trouble. The pointer is dereferenced in the first line.

It should be noted that the analyzer does not swear because it saw the dereferencing of an unverified pointer. So there would be too many false positives. After all, the function argument may not always be 0. Perhaps the check was carried out somewhere else.

The logic of the work is different. PVS-Studio considers the code dangerous if the pointer is dereferenced and then checked below. If the pointer is checked, then it is assumed that it may be 0. Therefore, this is an occasion to issue a warning.

This simple example was sorted out. But we are not interested in him at all.

Now let's move on to the more complex case of optimizations that compilers perform.

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 an example of a code, upon seeing which, people begin to argue and say that everything is fine. They reason like that.

Let the podhd pointer be NULL. The expression & podhd-> line6 looks ugly. But there is no mistake. There is no access to memory. The address of one of the class members is simply calculated. Yes, the value of the 'line6' pointer is incorrect. It indicates "nowhere." But this pointer is not used. The wrong address was calculated, so what? Below in the code there is a check and if 'podhd', then the function will complete its work. The 'line6' pointer is not used anywhere, which means that in practice there will be no error.

Gentlemen, you are wrong! You can’t do this anyway. Do not be lazy and correct such a code.

During optimization, the compiler argues like this. Here the pointer is dereferenced: podhd-> line6. Yeah, the programmer knows what he's doing. So the pointer here is definitely not zero. Great, remember that.

Then the compiler encounters a check:

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

And optimizes it. He considers the pointer 'podhd' to be non-zero. Therefore, it will reduce the validation to:

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

As in the case of memset (), the additional complexity is that we will not see the lack of verification in the Debug () version. Such a mistake is very difficult to look for.

As a result, if you pass a null pointer to the function, the function will not return the status (-ENODEV), but will continue its work. The consequences can be unpredictable.

The following is important. The compiler can remove important pointer checking from poorly written code. Those. there are functions that only seem to check pointers. In fact, they will work with a null pointer. I don’t know if it can be used somehow. But, in my opinion, such a situation can be seen 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(¶m->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

The compiler, during optimization, can reduce the scan to:

if (is_broadcast_ether_addr(¶m->addr[0]))

The Linux kernel is big. So V595 warnings have been issued over 200 pieces. To my shame, I was too lazy to look at them and chose only one example for the article. I suggest exploring the rest of the suspicious places to one of the developers. I list them: Linux-V595.txt .

Yes, far from all of these warnings indicate a real mistake. Often, a pointer cannot exactly be zero. However, this list is worth exploring. You can almost certainly find a couple of dozen real mistakes.

Suspicious Locations Found

Perhaps not all of the code snippets described in this article contain a real error. However, they are extremely suspicious and require close attention to study.

Invalid 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

The second condition is never fulfilled. For clarity, simplify the code:

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

As you can see, there is no such value in the variable 'A' at which the Y () function will be called.

Consider other similar cases. They do not need an explanation.

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 a couple of similar situations that I will not give in order not to make the article too 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 consider another variety 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 what this check should look like. Therefore, I will refrain from commenting. 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

"Tear out the eye"

While studying the analyzer messages, I met the name_msi_vectors () function. Although it is short, I do not want to read it at all. Apparently, this 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;
  }
}

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 The

last line is suspicious:

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

Now I’ll simplify it and it will immediately become clear that something is wrong here:

S[strlen(S)] = 0;

There is no point in such an action. Zero will be written to where it is already located. There is a suspicion that they wanted to do something else.

Endless waiting

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

The function tries to lock the driver. If this fails, she waits 1 second and retries. A total of 10 attempts should be made.

But, in fact, there will be an infinite number of attempts. The reason is that the variable 'i' does not increase anywhere.

Incorrect error information

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, the function should print information about it. Including should be printed error code. But, in fact, (err 0) or (err 1) will be printed, not the actual error code.

The reason is that the programmer is confused about the priorities of the operation. In the beginning, he wanted to put the result of the nftl_read_oob () function into the 'ret' variable. Then he wants to compare this variable with 0. If (ret <0), then you need to print an error message.

In fact, everything does not work like that. At the beginning, the result of the nftl_read_oob () function is compared with 0. The result of the comparison is the value 0 or 1. This value will be placed in the variable 'ret'.

Thus, if the nftl_read_oob () function returned a negative value, then ret == 1. A message will be displayed, but it is incorrect.

The condition shows that additional brackets are used. It is not known whether they were written to suppress the compiler warning about assignment inside the 'if' or to explicitly indicate the sequence of operations. If the second, then we are dealing with a typo - the closing bracket is not in its place. It will be correct like this:

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

Typo 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

Everywhere fields of one structure are copied to fields of another structure. And only in one place it suddenly says:

acx->window_size = acx->window_size;

This is mistake? Or did you think so? I can not give an answer.

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 numbers in this structure are given in decimal format. And suddenly there is one octal number: 0771. The analyzer alerted this. And me too.

There is a suspicion that this zero is written in order to beautifully align the column. But then this is clearly an incorrect value.

Suspicious line

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

Arrays contain some magic values. Suspicion calls the contents of the array CONF_Ind []. It alternates with zero characters with the text "x00". It seems to me that this is a typo and, in fact, it should be written like this:

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

Those. '0' before 'x' is superfluous and written by accident. As a result, “x00” is interpreted as text, not as a character 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 don’t think there is a mistake here. But the code is very poorly formatted. Therefore, I quote it in the article. It’s better to check again.

Undefined behavior in shear 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;
}

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

Shifting negative values results in undefined behavior. I wrote about this many times and will not dwell on this in detail. For those who don’t know what’s the matter, I propose to familiarize yourself with the article “ Do not know the ford, do not get into the water. Part Three (about shifts) ”.

I foresee an objection in the spirit: “but it works!” Maybe it works. But, I think, the Linux kernel is not a place to rely on this approach. The code is better to rewrite.

There are a lot of such shifts, so I put them in a separate file: Linux-V610.txt .

Confusion with enum

There are two such 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,
};

Note the constant ISCSI_PARAM_CONN_ADDRESS and ISCSI_HOST_PARAM_IPADDRESS. Their names are similar. Apparently, this was the reason for the confusion.

Consider the code snippet:

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 The

constant ISCSI_HOST_PARAM_IPADDRESS does not apply to enum iscsi_param. This is most likely a typo, and the constant ISCSI_PARAM_CONN_ADDRESS should be used.

Similar warnings from PVS-Studio:

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

Here I can not show the code fragment. The fragment is large, and I do not know how to reduce it and format it nicely. Therefore, I will write pseudo code.

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

The cycle is performed 1 time. There is a suspicion that such a design was created in order to do without the goto operator. If something went wrong, the 'break' statement is called, and the statements located after the loop begin to execute.

It confuses me that in one place the operator 'continue' is written, and not 'break'. In this case, the 'continue' operator works, like 'break'. I will explain.

Here is what the 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.)

Thus, after calling the 'continue' statement, condition (0) will be checked and the loop will end because the condition is false.

Perhaps 2 options.

The code is correct. The 'continue' statement must break the loop. Then I recommend replacing it with 'break' for consistency so that it does not confuse developers who will work with this code in the future.
The 'continue' statement should, as intended by the programmer, resume a cycle. Then the 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

The code was written using Copy-Paste and forgot to replace in one block of text:

DIG_TYPE_THRESH_HIGHPWR_HIGH on DIG_TYPE_THRESH_HIGHPWR_LOW
rssi_high_power_highthresh on rssi_high_power_lowthresh

Plus, I ask the 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

Reinitialization

There are strange places where a variable is assigned different values two times in a row. I think it's worth checking out these sections of code.

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

Заключение

In any large project, you can find some errors. The Linux kernel was no exception. However, one-time code checks by a static analyzer is the wrong way to use it. Yes, they allow you to write just such an advertising article, but they bring little benefit to the project.

Use static analysis regularly and you will save a ton of time by detecting a series of errors at the earliest stage of their occurrence. Protect your project from bugs with a static analyzer!

Linux and PVS-Studio

I suggest everyone to try PVS-Studio on their projects. The analyzer runs in a Windows environment. If someone wants to use it when developing large Linux applications, then write to us, and we will discuss possible options for concluding a contract for adapting PVS-Studio for your projects and tasks.

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. PVS-Studio dives into Linux insides (3.18.1) .

Have you read the article and have a question?

Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio and CppCat, version 2014 . Please see the list.

Tags: