Geekly Articles each Day
📜 ⬆️ ⬇️

How to find 56 potential vulnerabilities in FreeBSD code in one evening

FreeBSD, CWE

The time has come to check the FreeBSD project again and demonstrate that even in such serious and high-quality projects, the PVS-Studio analyzer easily finds errors. This time I decided to look at the search for errors in terms of detecting potential vulnerabilities. The PVS-Studio analyzer has always been able to detect defects that could potentially be used for an attack. But we never focused on this and described errors as typos, the consequences of unsuccessful Copy-Paste and so on, and did not classify them, for example, according to CWE. Now it’s very popular to talk about security and vulnerabilities, so I’ll try to expand your perception of our analyzer a little. PVS-Studio is not only a search for bugs, but also a tool that improves code security.

About verification


A report on our review of FreeBSD in 2016 can be found here .

As the name implies, what I described in the article I found in one evening. Those. I spent only 2-3 hours searching for potential vulnerabilities. This speaks of the full power of the PVS-Studio static analyzer. I recommend using our analyzer to anyone who cares about the quality of the code, and even more so its reliability and resistance to possible attacks.

I quickly wrote out the code with errors, but I could not find the time to complete my research in the form of this article for three weeks. During this time, we have even corrected some of the errors that will be described in the article, in the framework of the new direction “Security Defects that PVS-Studio team fixed this week”: issue N2 , issue N3 .
')
We ruled what is simpler and where it is clear how to fix it, without going into the essence of the algorithms. Therefore, FreeBSD authors should not be limited to our edits and even this article, but it makes sense to analyze the code yourself and in more detail. I am ready to give them a temporary key for a full check, and also to help in the fight against false positives that will disturb them. By the way, about false positives ...

False alarms


By checking the project with PVS-Studio, you can get a very wide variation in the number of false positives. For example, we recently tested the FAR project , and the number of false positives was 50%. This is a great result, meaning that every second message indicates an error or extremely bad code. And when checking Media Portal 2, the result was even better: 27% of false positives.

The FreeBSD project is more complicated. The fact is that the analyzer issued a huge number of general-purpose warnings:


Most messages, of course, are false positives. It is difficult to calculate, but I think there will be about 95% of false positives.

What does this mean? This says that it makes no sense to talk about the number of false positives on large projects without having to pre-configure the analyzer. Most of the false warnings occur due to different macros and are easy to remove using the various mechanisms provided by PVS-Studio. Let me explain this with an example.

In the FreeBSD code you can find the following array:

#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

Macro Q (1.5) is revealed in:

 (((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))))))))

The PVS-Studio analyzer considers some of the comparisons to be suspicious. For example, on the expression (((1.5) == 3) it gives a warning:

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

In total, the analyzer generates 96 messages for this array.

There are several more such arrays in FreeBSD code. In total, the analyzer generates 692 High level warnings on them. Let me remind you that the total warnings of the High level are 3577. This means that such macros lead to the appearance of 1/5 of these warnings.

In other words, by slightly configuring the analyzer, you can eliminate 20% of the false High level messages. This can be done in different ways, but perhaps the easiest way to turn off the warning is V674 for those files that use similar arrays. To do this, write somewhere in the file comment // - V :: 674.

I have already voiced an important thought, but I will repeat it, since we are again and again asked about the average percentage of false positives. Even if we calculate this average percentage based on the verification of many projects, it will not have any practical value. This is the same as being interested in the average temperature in the hospital.

It all depends very much on the project. Someone will be lucky and you can work with the list of warnings, almost without setting anything up. Others are not lucky, as is the case with the FreeBSD project. We'll have to deal with setting and marking macros. But it is not as scary as it may seem at first glance. Above, I just showed how you can immediately eliminate a lot of false warnings. A similar situation will be with other warnings that arise because of stupid macros.

And in general, if it was difficult to fight the noise, I would not be able to find all these errors for the article in one evening.

New world view


Wide horizons

We decided to look at the world more broadly. Where we used to see errors and the smell code, now we are also trying to see potential vulnerabilities. To do this, we decided to start by classifying the warnings issued by PVS-Studio according to the Common Weakness Enumeration (CWE). You can read more about this here: PVS-Studio: search for security defects .

Of course, only a small fraction of the errors can be exploited. In other words, only a few of the errors found by CWE can turn into CVE . However, the more errors that fall under the CWE classification, found using static analysis, the better.

Use PVS-Studio to prevent vulnerabilities. This article will demonstrate that the analyzer copes well with this task.

Potential vulnerabilities


CWE-476: NULL Pointer Dereference


In total, I noticed 22 errors of this type. And, probably, I missed the same amount.

Let's start with a simple case.

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

PVS-Studio warning : V522 Dereferencing of the null pointer 'ha' might take place. ql_isr.c 750

Here the error is visible immediately. If ha is NULL , then it is dereference in the expression ha-> pci_dev .

The same situation is found in three more files:


Now consider a more complex situation:

 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 warning : V595 Check lines: 667, 669. ecore_init_ops.h 667

Let us dwell on this case in more detail, since not everyone understands all the danger contained in this code.

Silent null


At the beginning, the ilt pointer is dereferenced:

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

Next, it is checked for NULL equality:

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

Consequently, null pointer dereferencing is possible. This inevitably leads to undefined program behavior.

Here, some will argue that there is no misfortune, since the pointer is derefered "not really." They will say that the cell address of the array is simply calculated. Yes, they say, this address is incorrect and cannot be used. However, there is a check below, and the function will exit if the ilt pointer is equal to zero. Therefore, the invalid pointer ilt_cli will not be used anywhere and there is no error in the program.

They are wrong. You can not think so. Dereferencing a null pointer results in undefined behavior. Therefore, the code is incorrect and you should not argue how it will work. It can work as you like.

However, such an explanation often does not suit some readers, so I will try to develop a thought. The compiler knows that dereferencing a null pointer is an undefined behavior. Therefore, if the pointer is dereferenced, then it is not NULL . Since it is not NULL, the compiler has the right to remove the redundant check if (! Ilt) . As a result, if the pointer is NULL , then there will be no exit from the function. Therefore, the function will start processing invalid pointers, which can lead to anything.

Some object, saying that the macro offsetof is sometimes implemented as follows:

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

Here the null pointer dereferencing takes place, but the code works successfully. Consequently, this proves that such constructions are quite admissible.

And again they are wrong. Nothing proves it.

When considering the idiomatic implementation of offsetof, one should take into account that the compiler is allowed to use non-portable techniques for implementing this functionality. The fact that the compiler uses the constant of the pointer in the implementation of the library when implementing the offsetof does not mean that you can safely execute & ilt-> clients [cli_num] in the case when the ilt is a null pointer.

More details on this topic can be found in my article " Dereferencing a null pointer leads to undefined behavior ."

As a result, the code discussed above is a very real error and should be corrected.

Now that we have dealt with the nuances of null pointer dereferencing, it becomes clear that the following function is also incorrect:

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

At the beginning it may not be clear why the analyzer decided that the ccb pointer would be zero. Therefore, we note that the analyzer indicates in the warning also here: iscsi.c: 2157.

Here is the call to the iscsi_outstanding_add function, which is passed NULL as the actual argument:

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

Here, the analyzer needed to perform an interprocedural analysis to find the defect.

Let's take a break from complex errors and consider a very simple case of an incorrect error handler.

 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 analyzer warning: V522 Dereferencing of the null pointer 'dev_priv' might take place. radeon_cs.c 153

The body of the if statement is executed only when the dev_priv pointer is zero. Thus, it is not clear what address is calculated here: & dev_priv-> cs.cs_mutex . And indeed it is UB.

Oh. Something problems with null pointers does not end there. But what to do, since it is the headache of many programming languages. So make coffee and keep reading.

Null


 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 analyzer warning: V595 The 'mac' pointer was used before it was verified against nullptr. Check lines: 6757, 6760. if_bwn.c 6757

The mac pointer at the beginning is dereferenced, and then checked for NULL equality. Everything is simple here, so I will not comment on it in more detail.

 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 analyzer warning: V595 The 'ctl3_rewriters' pointer was used against it. Check lines: 3206, 3210. ip_fw_sockopt.c 3206

Notice that at first the ctl3_rewriters pointer is used as the actual argument of the memcpy function:

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

And then they suddenly remember that it should be checked for NULL equality:

 if (ctl3_rewriters != NULL)

Consider another case of incorrect code designed to release resources:

 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 analyzer warning: V595 The 'mc' pointer was used before it was verified against nullptr. Check lines: 2954, 2955. mly.c 2954

It is necessary to round out with null pointers, because, I think, the description of such errors has already bored not only me, but also the reader. I see CWE-476 (NULL Pointer Dereference) in the following code areas:


But that's not all! I was just bored with studying this kind of error, and I turned to the study of another type of warning. PVS-Studio analyzer is waiting for heroes who will study all the warnings related to null pointers.

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


Consider the definition of the pfloghdr structure:

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

As you can see, this structure is quite large. To initialize such structures, it is common practice when the entire structure is filled with zeros, and then values ​​are set for individual fields.

However, the nat64lsn_log function failed to correctly initialize the structure. Consider the code for this function:

 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 analyzer warning: V512 A call of the 'memset' function will 'plog'. nat64lsn.c 218

Note that sizeof (plog) does not calculate the size of the structure, but the size of the pointer. As a result, not the entire structure is reset, but only a few first bytes, all other fields of the structure remain uninitialized. Of course, the correct values ​​are clearly written to some fields. However, a number of members in the structure will remain uninitialized.

Exactly the same error can be seen in the nat64stl.c file: V512 A call of the pause. nat64stl.c 72

CWE-457: Use of Uninitialized Variable


Consider another error due to which the variable may not be initialized.

 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 analyzer warning: V614 Uninitialized variable 'status' used. tdioctl.c 3396

If the __FreeBSD__ macro is declared (and it is declared), then the condition

 #if !(defined(__FreeBSD__))

not performed. As a result, the code inside the #if ... # endif construct is not compiled, and the status variable remains uninitialized.

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 analyzer warning: V512 A call of the MPI Coredump function will lead to the meeting. qls_dump.c 1615

To show how the types and members of the structure that we are interested in were declared, we had to bring in quite a lot of code. But do not rush to yawn, now I will highlight the most significant:

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

Important for us:

  1. The sizeof operator calculates the size of the array and returns the number 16.
  2. The string “MPI Coredump”, taking into account the terminal zero, takes 13 bytes.

13 bytes of the string and 3 more bytes after the string will be copied. Such code may even work in practice. Just copy 3 bytes with garbage or a fragment of another string. However, formally, this is going beyond the array boundary, and, therefore, this code leads to undefined program behavior.

CWE-129: Improper Validation of Array Index


So I had a chance to show you one of the new diagnostics implemented in PVS-Studio. The essence of diagnostics V781 :

At the beginning, the value of the variable is used as the size or index of the array. And then this value is compared with 0 or with the size of the array. This may indicate a logical error in the code or a typo in one of the comparisons.

At its core, this diagnosis is similar to the V595 , which is well known to my regular readers.

 ,        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 analyzer warning: V781 Perhaps there is a mistake in program logic. Check lines: 1617, 1619. sbp_targ.c 1617

At the beginning, the programmer boldly used the lun index to access the lstate array. And only then a check is made: does the index value exceed the maximum permissible value equal to MAX_LUN ? If it exceeds, then this situation is treated as erroneous. But it is too late, since we could already go far beyond the array.

Formally, this will lead to an undefined program behavior. In practice, instead of correctly processing an incorrect index value, Access Violation may occur.

Now we will consider a more interesting case of incorrect array indexing.

 #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]; } .... }

The analyzer issued here three warnings for three expressions, where the pwr array is accessed :


In a loop, the value of the ridx index changes from RTWN_RIDX_OFDM6 to RTWN_RIDX_COUNT . That is, the variable ridx takes values ​​in the range [4..27]. At first glance, everything is fine.

To find the error, pay attention to the pwr member, which is a two-dimensional array:

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

And let's look again at how this array is accessed in a loop:

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

There is clearly something wrong here. Occurs over the edge of the array. However, I find it difficult to guess how this code should work and what should be changed here.

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 analyzer warning: V640 The code's operational logic does not correspond with its formatting. The statement is indented. It is possible that curly brackets are missing. smbfs_vnops.c 283

The design of the code does not correspond to the logic of its work. Visually, it seems that the line smbfs_free_scred (scred); executed only if the condition is true. But in fact, this line is always executed.

Perhaps there is no real error here, and it is enough to format the code, but, in any case, this code fragment deserves close attention.

The analyzer detected 4 more four similar suspicious code fragments, but I will not give them here, since they are all of the same type. Limited warning:


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 analyzer warning: V519 The 'a1' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 397, 400. ip_ftp_pxy.c 400

Regardless of the condition, the variable a1 will be assigned the value ntohl (ip-> ip_src.s_addr) .

It seems to me that the last assignment is redundant. , .

:

 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:


— .

, , , . , .

 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] != 0x28 || cdb[0] != 0x2A' is always true. Probably the '&&' operator should be used here. mfi_tbolt.c 1110

(cdb[0] != 0x28 || cdb[0] != 0x2A) . 0x28, 0x2A . And vice versa. .

, .

 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:


, 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: 0x2002. 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 '0x00000080' 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:


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 .

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

, , -, :


. memset . . V597 .

CWE-561: Dead Code


 static int wi_pci_resume(device_t dev) { struct wi_softc *sc = device_get_softc(dev); struct ieee80211com *ic = &sc->sc_ic; WI_LOCK(sc); if (sc->wi_bus_type != WI_BUS_PCI_NATIVE) { return (0); // <= WI_UNLOCK(sc); // <= } if (ic->ic_nrunning > 0) wi_init(sc); WI_UNLOCK(sc); return (0); }

PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. if_wi_pci.c 258

return , .

Other


. , CWE, , . , , .

 static void mac_proc_vm_revoke_recurse(struct thread *td, struct ucred *cred, struct vm_map *map) { .... if (!mac_mmap_revocation_via_cow) { vme->max_protection &= ~VM_PROT_WRITE; vme->protection &= ~VM_PROT_WRITE; } if ((revokeperms & VM_PROT_READ) == 0) vme->eflags |= MAP_ENTRY_COW | MAP_ENTRY_NEEDS_COPY; .... }

PVS-Studio: V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. mac_process.c 352

, , , else .

:


Copy-Paste. ?

 static int cyapa_raw_input(struct cyapa_softc *sc, struct cyapa_regs *regs, int freq) { .... if (sc->delta_x > sc->cap_resx) sc->delta_x = sc->cap_resx; if (sc->delta_x < -sc->cap_resx) sc->delta_x = -sc->cap_resx; if (sc->delta_y > sc->cap_resx) sc->delta_y = sc->cap_resy; if (sc->delta_y < -sc->cap_resy) sc->delta_y = -sc->cap_resy; .... }

PVS-Studio: V778 Two similar code fragments were found. Perhaps, this is a typo and 'cap_resy' variable should be used instead of 'cap_resx'. cyapa.c 1458

:

 if (sc->delta_y > sc->cap_resx)

cap_resx cap_resy .

 static int linux_msqid_pushdown(l_int ver, struct l_msqid64_ds *linux_msqid64, caddr_t uaddr) { .... if (linux_msqid64->msg_qnum > USHRT_MAX) linux_msqid.msg_qnum = linux_msqid64->msg_qnum; else linux_msqid.msg_qnum = linux_msqid64->msg_qnum; .... }

PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. linux_ipc.c 353

:


, strncmp :

 int ipf_p_irc_complete(ircp, buf, len) ircinfo_t *ircp; char *buf; size_t len; { .... if (strncmp(s, "PRIVMSG ", 8)) return 0; .... if (strncmp(s, "\001DCC ", 4)) // <= return 0; .... }

PVS-Studio: V666 Consider inspecting third argument of the function 'strncmp'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. ip_irc_pxy.c 140

, , «PRIVMSG ». .

, "\001DCC ". , , 4, 5 . : \001 — .

PVS-Studio


PVS-Studio


FreeBSD Coverity ( Synopsys). , PVS-Studio 56 10 . . ? , PVS-Studio Coverity . PVS-Studio .

Conclusion


, , . , , , , . , . , . .

, PVS-Studio . , PVS-Studio . , support[@]viva64.com .



, : Andrey Karpov. How to find 56 potential vulnerabilities in FreeBSD code in one evening

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, version 2015 . Please review the list.

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

More articles:


All Articles