Geekly Articles each Day
📜 ⬆️ ⬇️

Release of PVS-Studio for macOS: 64 weaknesses in Apple XNU Kernel

Bug in apple The new version of PVS-Studio 6.23 runs under macOS and allows you to check projects written in C and C ++. Our team decided to coincide with the XNU Kernel test for this event.

PVS-Studio for macOS


With the release of the analyzer version for macOS, PVS-Studio can be safely called the cross-platform static code analyzer for C and C ++.

Initially, there was a version only for Windows. About two years ago, our team supported Linux: " How PVS-Studio was created under Linux ". Also, attentive readers of our blog should remember the FreeBSD Kernel review articles ( article 1, article 2 ). Then the analyzer was built to run in PC-BSD and TrueOS. And finally, we got to macOS!

Is it easy to develop cross-platform product?
')
This question has an economic and technical component.

From an economic point of view, making the analyzer cross-platform was the right decision. Software development has long been moving in this direction, and a tool for developers of such projects must be appropriate. However, if something is useful - it does not mean that it is immediately worth starting to do. At first, we always make sure that we have enough strength to implement something in a new direction, and then support it.

On the technical side, it is difficult only at the very beginning, if the project is not immediately thought of as cross-platform. We spent several months adapting the analyzer to the Linux system. Compilation of the project under the new platform did not take much time: we do not have a graphical interface and the code is almost not tied to the use of system APIs. Most of the time was taken by adapting the analyzer to new compilers and improving the quality of analysis. In other words, a lot of effort requires the prevention of false positives .

And what about the development under macOS?

At this point, we already had a CMake project file for CMake, easily adaptable for different operating systems. Testing systems of different types were also cross-platform. All this helped to run very quickly on macOS.

A special feature of analyzer development for macOS is the Apple LLVM Compiler. Although the analyzer was assembled with the help of GCC and worked fine, it could still affect the compatibility of the analyzer with users' computers in the future. In order not to create problems for potential users, we decided to support the build of the distribution with the help of this compiler, which comes with Xcode.

C ++ development helps a lot in creating and developing cross-platform projects, but different compilers add such features very unevenly, therefore conditional compilation is still actively used in several places.

In general, everything went smoothly and easily. As before, most of the time was spent on the revision of exceptions, site modification, testing, and other related issues. As the first project tested with PVS-Studio for macOS, we present you the XNU Kernel .

Distributive

You can familiarize yourself with the ways of downloading and installing PVS-Studio for macOS here .

XNU Kernel


Where to start demonstrating the features of PVS-Studio for macOS, if not from checking the kernel of this system! Therefore, the first project tested with the new version of the analyzer was the XNU Kernel.

XNU is the core of computer operating systems developed by Apple and used in the OS OS family (macOS, iOS, tvOS, watchOS). More details .

It is believed that the kernel is written in C and C ++, but in fact it is C. I counted 1302 * .c files and only 97 * .cpp files. The size of the code base is 1929 KLOC. It turns out that this is a relatively small project. For comparison, the code base of the Chromium project is 15 times larger and contains 30 MLOC.

The source code can be conveniently downloaded from a mirror on GitHub: xnu .

Test results


Although the XNU Kernel project is relatively small, studying analyzer warnings alone is a big task and takes a lot of time. Complicating the study of warnings and false positives, since I did not pre-configure the analyzer. I just quickly ran through the warnings, writing out code snippets of interest, in my opinion. This is more than enough to write an article, and, moreover, solid in size. PVS-Studio analyzer easily finds a large number of interesting errors.

A note for XNU Kernel developers . I did not have the task to find as many errors as possible. Therefore, you should not be guided by this article to correct them. Firstly, this is inconvenient, since it is not possible to navigate through warnings. I am sure that it is much better to use one of the formats that PVS-Studio can generate, for example, an HTML report with the ability to navigate (it is similar to what Clang can generate). Secondly, I missed many mistakes simply because I studied the report superficially. I recommend that developers independently carry out a more thorough analysis of the project with the help of PVS-Studio.

As I said, the false positives prevented me, but in fact they are not a problem. If you configure the analyzer, it is quite possible to reduce the number of false positives to 10-15% . Since setting it up also takes time and restarting the analysis process, I missed this step - it’s easy for me to compile errors for the article. If you plan to perform the analysis carefully, then, of course, should be given time to set up.

Basically, false positives occur due to macros and insufficiently qualitatively tagged functions. For example, in XNU Kernel, most of them are related to the use of the panic function.

Here is how this function is declared:

extern void panic(const char *string, ...) __attribute__((__format__ (__printf__, 1, 2)));

The function is annotated so that its input arguments are interpreted by analogy with the arguments of the printf function. This allows compilers and analyzers to find errors of incorrect formatting of strings. However, the function is not marked as non-returnable. As a result, the following code causes false positives:

 if (!ptr) panic("zzzzzz"); memcpy(ptr, src, n);

Here, the analyzer issues a warning that a null pointer can be dereferenced. From his point of view, after calling the panic function, the memcpy function will be called.

To avoid such false positives, you should change the function annotation by adding __attribute __ ((noreturn)) :

 extern __attribute__((noreturn)) void panic(const char *string, ...) __attribute__((__format__ (__printf__, 1, 2)));

Let's now see what interesting things I managed to see in the XNU Kernel code. In total, I wrote out 64 errors and decided to dwell on this beautiful number. I grouped the defects according to the Common Weakness Enumeration , since many people are familiar with this classification, and it will be easier for them to understand what mistakes they are talking about in this or that chapter.

CWE-570 / CWE-571: Expression is Always False / True


Very diverse errors can lead to CWE-570 / CWE-571 , i.e. to situations where a condition or part of a condition is always false / true. In the case of the XNU Kernel project, all these errors, in my opinion, are related to typos. PVS-Studio generally reveals typos very well.

Fragment N1

 int key_parse( struct mbuf *m, struct socket *so) { .... if ((m->m_flags & M_PKTHDR) == 0 || m->m_pkthdr.len != m->m_pkthdr.len) { ipseclog((LOG_DEBUG, "key_parse: invalid message length.\n")); PFKEY_STAT_INCREMENT(pfkeystat.out_invlen); error = EINVAL; goto senderror; } .... }

PVS-Studio warning: V501 CWE-570 There are identical sub-expressions 'm-> M_dat.MH.MH_pkthdr.len' to the left. key.c 9442

Because of a typo, a member of the class is compared to itself:

 m->m_pkthdr.len != m->m_pkthdr.len

A part of the condition is always false and, as a result, the length of a message is checked incorrectly. It turns out that the program will continue to process incorrect data. Perhaps this is not terrible, but many vulnerabilities are precisely related to the fact that some input data was unverified or not adequately checked. So this place in the code clearly deserves the attention of developers.

Fragment N2, N3

 #define VM_PURGABLE_STATE_MASK 3 kern_return_t memory_entry_purgeable_control_internal(...., int *state) { .... if ((control == VM_PURGABLE_SET_STATE || control == VM_PURGABLE_SET_STATE_FROM_KERNEL) && (((*state & ~(VM_PURGABLE_ALL_MASKS)) != 0) || ((*state & VM_PURGABLE_STATE_MASK) > VM_PURGABLE_STATE_MASK))) return(KERN_INVALID_ARGUMENT); .... }

PVS-Studio warning: V560 CWE-570 A part of conditional expression is always false: ((* state & 3)> 3). vm_user.c 3415

Let's take a closer look at this part of the expression:

 (*state & VM_PURGABLE_STATE_MASK) > VM_PURGABLE_STATE_MASK

If we substitute the value of the macro, we get:

 (*state & 3) > 3

The bitwise AND operation can only result in values ​​of 0, 1, or 2. It makes no sense to check whether 0, 1, or 2 is greater than 3. It is very likely that the expression contains some typo.

As in the previous case, a certain status is incorrectly verified, which may cause the processing of incorrect (unreliable) data.

Exactly the same error is found in the file vm_map.c. Apparently, part of the code was written using Copy-Paste. Warning: V560 CWE-570 A part of conditional expression is always false: ((* state & 3)> 3). vm_map.c 15809

Fragment N4

 void pat_init(void) { boolean_t istate; uint64_t pat; if (!(cpuid_features() & CPUID_FEATURE_PAT)) return; istate = ml_set_interrupts_enabled(FALSE); pat = rdmsr64(MSR_IA32_CR_PAT); DBG("CPU%d PAT: was 0x%016llx\n", get_cpu_number(), pat); /* Change PA6 attribute field to WC if required */ if ((pat & ~(0x0FULL << 48)) != (0x01ULL << 48)) { mtrr_update_action(CACHE_CONTROL_PAT); } ml_set_interrupts_enabled(istate); }

PVS-Studio warning: V547 CWE-571 Expression is always true. mtrr.c 692

Let's look at the meaningless check into which a typo most likely crept in:

 (pat & ~(0x0FULL << 48)) != (0x01ULL << 48)

We calculate some expressions:
An expression (pat & [0xFFF0FFFFFFFFFFFF]) cannot result in a value of 0x0001000000000000 . The condition is always true. As a result, the mtrr_update_action function is always called.

Fragment N5

Now, in my opinion, there will be a very beautiful typo: instead of == they wrote = .

 typedef enum { CMODE_WK = 0, CMODE_LZ4 = 1, CMODE_HYB = 2, VM_COMPRESSOR_DEFAULT_CODEC = 3, CMODE_INVALID = 4 } vm_compressor_mode_t; void vm_compressor_algorithm_init(void) { .... assertf(((new_codec == VM_COMPRESSOR_DEFAULT_CODEC) || (new_codec == CMODE_WK) || (new_codec == CMODE_LZ4) || (new_codec = CMODE_HYB)), "Invalid VM compression codec: %u", new_codec); .... }

PVS-Studio warning: V768 CWE-571 The expression 'new_codec = CMODE_HYB' is of enum type. It is the odd that it is used for an expression of a Boolean-type. vm_compressor_algorithms.c 419

In the process of checking the condition, the variable new_codec is assigned the value 2. As a result, the condition is always true and the assert macro actually does not check anything.

The error could be harmless. Well, you will think, the assert-macro did not check something - it doesn't matter. However, besides this, the debug version also works incorrectly. After all, the value of the variable new_codec is spoiled and the wrong codec is used.

Fragment N6, N7

 void pbuf_copy_back(pbuf_t *pbuf, int off, int len, void *src) { VERIFY(off >= 0); VERIFY(len >= 0); VERIFY((u_int)(off + len) <= pbuf->pb_packet_len); if (pbuf->pb_type == PBUF_TYPE_MBUF) m_copyback(pbuf->pb_mbuf, off, len, src); else if (pbuf->pb_type == PBUF_TYPE_MBUF) { if (len) memcpy(&((uint8_t *)pbuf->pb_data)[off], src, len); } else panic("%s: bad pb_type: %d", __func__, pbuf->pb_type); }

PVS-Studio warning: V517 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a possibility of logical error presence. Check lines: 340, 343. pf_pbuf.c 340

For clarity, I will highlight the essence:

 if (A) foo(); else if (A) Unreachable_code; else panic();

If condition A is true, then the body of the first if statement is executed. If this is not the case, then the repeated check does not make sense and the panic function is immediately called. Part of the code is generally unreachable.

Here is some kind of logic error, or a typo in one of the conditions.

Below in the same file is the function pbuf_copy_data , which, apparently, was written using Copy-Paste and contains the same error. Warning: V517 CWE-570 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 358, 361. pf_pbuf.c 358

CWE-670: Always-Incorrect Control Flow Implementation


Defect CWE-670 says that in the code, most likely, something does not work the way the programmer intended.

Fragment N8, N9, N10

 static void in_ifaddr_free(struct ifaddr *ifa) { IFA_LOCK_ASSERT_HELD(ifa); if (ifa->ifa_refcnt != 0) { panic("%s: ifa %p bad ref cnt", __func__, ifa); /* NOTREACHED */ } if (!(ifa->ifa_debug & IFD_ALLOC)) { panic("%s: ifa %p cannot be freed", __func__, ifa); /* NOTREACHED */ } if (ifa->ifa_debug & IFD_DEBUG) { .... }

PVS-Studio Warning: V646 CWE-670 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. in.c 2010

Perhaps there is no error in this code. However, this place looks very suspicious:

 } if (!(ifa->ifa_debug & IFD_ALLOC)) {

The anomaly is that it is not accepted to write like this. It would be more logical to start writing if with a new line. Code authors should check this place. Perhaps, the else keyword is missing here and the code should be like this:

 } else if (!(ifa->ifa_debug & IFD_ALLOC)) {

Or you just need to add a line break so that this code is not confused either by the analyzer or by the colleagues accompanying this code.

Similar suspicious places can be found here:


Fragment N11, N12, N13, N14

 int dup2(proc_t p, struct dup2_args *uap, int32_t *retval) { .... while ((fdp->fd_ofileflags[new] & UF_RESERVED) == UF_RESERVED) { fp_drop(p, old, fp, 1); procfdtbl_waitfd(p, new); #if DIAGNOSTIC proc_fdlock_assert(p, LCK_MTX_ASSERT_OWNED); #endif goto startover; } .... startover: .... }

PVS-Studio warning: V612 CWE-670 An unconditional 'goto' within a loop. kern_descrip.c 628

This is a very strange code. Notice that the body of the while statement ends with the goto statement . In this case, the operator continue is not used anywhere in the loop body. This means that the loop body will be executed no more than once.

Why was it necessary to create a loop, if it still does not perform more than one iteration? It would be better to use the if statement , then it would not cause questions. I think this is a mistake, and something is written incorrectly in the cycle. For example, it is possible that before the goto operator some condition is missing.

Similar "one-time" cycles occur 3 more times:

Dereferencing null pointers: CWE-476, CWE-628, CWE-690


There are various reasons why a null pointer dereference may occur and the PVS-Studio analyzer, depending on the situation, can assign them different CWE-IDs:
When writing an article, I considered it reasonable to collect all the errors of this type in one section.

Fragment N15

I'll start with complex and large functions. First, consider the netagent_send_error_response function, in which the pointer passed in the session argument is dereferenced.

 static int netagent_send_error_response( struct netagent_session *session, u_int8_t message_type, u_int32_t message_id, u_int32_t error_code) { int error = 0; u_int8_t *response = NULL; size_t response_size = sizeof(struct netagent_message_header); MALLOC(response, u_int8_t *, response_size, M_NETAGENT, M_WAITOK); if (response == NULL) { return (ENOMEM); } (void)netagent_buffer_write_message_header(.....); if ((error = netagent_send_ctl_data(session->control_unit, (u_int8_t *)response, response_size))) { NETAGENTLOG0(LOG_ERR, "Failed to send response"); } FREE(response, M_NETAGENT); return (error); }

Note that the session pointer is dereferenced in the session-> control_unit expression without any prior checking. Whether the null pointer is dereferenced or not depends on which actual arguments are passed to this function.

Now let's see how the netagent_send_error_response function described above is used in the netagent_handle_unregister_message function:

 static void netagent_handle_unregister_message( struct netagent_session *session, ....) #pragma unused(payload_length, packet, offset) u_int32_t response_error = NETAGENT_MESSAGE_ERROR_INTERNAL; if (session == NULL) { NETAGENTLOG0(LOG_ERR, "Failed to find session"); response_error = NETAGENT_MESSAGE_ERROR_INTERNAL; goto fail; } netagent_unregister_session_wrapper(session); netagent_send_success_response(session, .....); return; fail: netagent_send_error_response( session, NETAGENT_MESSAGE_TYPE_UNREGISTER, message_id, response_error); }

PVS-Studio warning: V522 CWE-628 Dereferencing of the null pointer 'session' might take place. The null pointer is passed into the netagent_send_error_response 'function. Inspect the first argument. Check lines: 427, 972. network_agent.c 427

Here the Data Flow analysis implemented in PVS-Studio manifests itself. The analyzer notes that if the session pointer was NULL , then some information is written to the log, after which a transition to the fail flag occurs.

Next comes the netagent_send_error_response function call :

 fail: netagent_send_error_response( session, NETAGENT_MESSAGE_TYPE_UNREGISTER, message_id, response_error);

Notice that the ill-fated session pointer, which is NULL, is passed to the function as the actual argument.

As we know, in the netagent_send_error_response function there is no protection for this case, and a null pointer is dereferenced.

Fragment N16

The following situation is similar to the previous one. The function code will be shorter, but you will have to deal with it just as slowly and in detail.

 void * pf_lazy_makewritable(struct pf_pdesc *pd, pbuf_t *pbuf, int len) { void *p; if (pd->lmw < 0) return (NULL); VERIFY(pbuf == pd->mp); p = pbuf->pb_data; if (len > pd->lmw) { .... }

Notice that the pbuf pointer is dereference without first checking for NULL equality. There is a "VERIFY (pbuf == pd-> mp)" check in the code. However, pd-> mp may be NULL , so the check cannot be considered as protection against NULL .

Note. Please remember that I am completely unfamiliar with the XNU Kernel code and may be mistaken. Perhaps pd-> mp will never store a NULL value. Then all my reasoning is wrong and there is no error here. However, such code is still better to check again.

Let's continue and see how the pf_lazy_makewritable function is used .

 static int pf_test_state_icmp(....) { .... if (pf_lazy_makewritable(pd, NULL, off + sizeof (struct icmp6_hdr)) == NULL) return (PF_DROP); .... }

PVS-Studio warning: V522 CWE-628 Dereferencing of the null pointer 'pbuf' might take place. The null pointer is passed into the 'pf_lazy_makewritable' function. Inspect the second argument. Check lines: 349, 7460. pf.c 349

As the second actual argument, a null is passed to the pf_lazy_makewritable function. It is very strange.

Suppose a programmer thinks that the program will be protected from a null pointer by “VERIFY (pbuf == pd-> mp)”. Then the question arises: why write such code at all? Why call a function by passing an obviously incorrect argument?

Therefore, it seems to me that in fact the pf_lazy_makewritable function should be able to take a null pointer and somehow handle this case in a special way, but it does not. This code deserves the most thorough verification by the programmer, and the PVS-Studio analyzer is definitely right to draw our attention to it.

Fragment N17

You can relax a bit: consider the simple case.

 typedef struct vnode * vnode_t; int cache_lookup_path(...., vnode_t dp, ....) { .... if (dp && (dp->v_flag & VISHARDLINK)) { break; } if ((dp->v_flag & VROOT) || dp == ndp->ni_rootdir || dp->v_parent == NULLVP) break; .... }

PVS-Studio warning: V522 CWE-690 There might be a null pointer 'dp'. vfs_cache.c 1449

Take a look at the check:

 if (dp && (dp->v_flag & VISHARDLINK))

It tells us that the dp pointer may be null. However, further the pointer is dereferenced without preliminary check:

 if ((dp->v_flag & VROOT) || ....)

Fragment N18

In the previous example, we encountered a situation where the pointer is checked before dereferencing, and later in the code, the check was forgotten. But much more often you can find a situation where at first the pointer is dereferenced, and only then it is checked. No exception and the project code XNU Kernel. To better understand what we are talking about, let's first consider a synthetic example:

 p[n] = 1; if (!p) return false;

Let's now take a look at how such errors look in reality. Let's start with the name comparison function. Comparison functions are very insidious :).

 bool IORegistryEntry::compareName(....) const { const OSSymbol * sym = copyName(); bool isEqual; isEqual = sym->isEqualTo( name ); // <= if( isEqual && matched) { name->retain(); *matched = name; } if( sym) // <= sym->release(); return( isEqual ); }

PVS-Studio warnings: V595 CWE-476; Check lines: 889, 896. IORegistryEntry.cpp 889

The lines of code that interest us are highlighted by comments like "// <=". As you can see, first the pointer is dereferenced. Further in the code there is a check for equality of the pointer nullptr . But it is immediately clear that if the pointer is zero, then a null pointer will dereference and the function, in fact, is not ready for such a situation.

Fragment N19

The following error occurred due to a typo.

 static int memorystatus_get_priority_list( memorystatus_priority_entry_t **list_ptr, size_t *buffer_size, size_t *list_size, boolean_t size_only) { .... *list_ptr = (memorystatus_priority_entry_t*)kalloc(*list_size); if (!list_ptr) { return ENOMEM; } .... }

PVS-Studio warning: V595 CWE-476 The 'list_ptr' pointer was used against nullptr. Check lines: 7175, 7176. kern_memorystatus.c 7175

The analyzer sees that first the variable is dereferenced, and the next line checks for equality nullptr . This interesting error occurred due to the fact that the programmer forgot to write the character '*'. In fact, the code should have been like this:

 *list_ptr = (memorystatus_priority_entry_t*)kalloc(*list_size); if (!*list_ptr) { return ENOMEM; }

It can be said that the error was revealed indirectly. However, it does not matter, because the most important thing is that the analyzer paid attention to the anomalous code and we saw an error.

Fragment N20 - N35

The XNU Kernel code reveals a lot of errors detected by diagnosing the V595. However, to consider all of them will be boring. Therefore, I will analyze only one more case, and then I will give a list of messages indicating errors.

 inline void inp_decr_sndbytes_unsent(struct socket *so, int32_t len) { struct inpcb *inp = (struct inpcb *)so->so_pcb; struct ifnet *ifp = inp->inp_last_outifp; if (so == NULL || !(so->so_snd.sb_flags & SB_SNDBYTE_CNT)) return; if (ifp != NULL) { if (ifp->if_sndbyte_unsent >= len) OSAddAtomic64(-len, &ifp->if_sndbyte_unsent); else ifp->if_sndbyte_unsent = 0; } }

PVS-Studio warning: V595 CWE-476 Check lines: 3450, 3453. in_pcb.c 3450

I suggest the reader to independently follow the fate of the pointer so and make sure that the code is written incorrectly.

Other errors:


Fragment N36, N37

And the last couple of errors on the use of null pointers.

 static void feth_start(ifnet_t ifp) { .... if_fake_ref fakeif; .... if (fakeif != NULL) { peer = fakeif->iff_peer; flags = fakeif->iff_flags; } /* check for pending TX */ m = fakeif->iff_pending_tx_packet; .... }

PVS-Studio warning: V1004 CWE-476 The "fakeif" pointer was used unsafely after it was verified against nullptr. Check lines: 566, 572. if_fake.c 572

I think this comment is not required. Just see how the fakeif pointer is checked and used.

Last similar case: V1004 CWE-476 The 'rt-> rt_ifp' pointer was used unsafely after it was verified against nullptr. Check lines: 138, 140. netsrc.c 140

CWE-119: Improve Restriction of Memory Buffer


I met a couple of errors related to the buffer overrun. A very unpleasant kind of mistake for such a responsible project as XNU Kernel.

Different options for going beyond the array boundary can be classified by different CWE IDs, but in this case, the analyzer chose CWE-119 .

Fragment N38

To begin, consider how some macros are declared.

 #define IFNAMSIZ 16 #define IFXNAMSIZ (IFNAMSIZ + 8) #define MAX_ROUTE_RULE_INTERFACES 10

It is important for us to remember that:
And now let's consider a function where it is possible to go beyond the buffer boundary using the snprintf and memset functions. Those. There are 2 errors.

 static inline const char * necp_get_result_description(....) { .... char interface_names[IFXNAMSIZ][MAX_ROUTE_RULE_INTERFACES]; .... for (index = 0; index < MAX_ROUTE_RULE_INTERFACES; index++) { if (route_rule->exception_if_indices[index] != 0) { ifnet_t interface = ifindex2ifnet[....]; snprintf(interface_names[index], IFXNAMSIZ, "%s%d", ifnet_name(interface), ifnet_unit(interface)); } else { memset(interface_names[index], 0, IFXNAMSIZ); } } .... }

PVS-Studio warnings:
Notice how the two-dimensional array interface_names is declared:

 char interface_names[IFXNAMSIZ][MAX_ROUTE_RULE_INTERFACES]; //  : char interface_names[24][10];

But it uses this array as if it were like this:

 char interface_names[MAX_ROUTE_RULE_INTERFACES][IFXNAMSIZ]; //  : char interface_names[10][24];

It turns out some kind of porridge from the data.

Someone, without thinking, can say that there is nothing terrible, because both arrays occupy the same number of bytes.

No, everything is bad. The array elements interface_names [10..23] [....] are not used, since the index variable in the loop takes the values ​​[0..9]. But the interface_names [0..9] [....] elements are starting to “cling” to each other. Those. Some data overwrites others.

The result is complete bullshit. , «», .

N39

necp_client.c , .

 #define IFNAMSIZ 16 #define IFXNAMSIZ (IFNAMSIZ + 8) #define NECP_MAX_PARSED_PARAMETERS 16 struct necp_client_parsed_parameters { .... char prohibited_interfaces[IFXNAMSIZ] [NECP_MAX_PARSED_PARAMETERS]; .... }; static int necp_client_parse_parameters(...., struct necp_client_parsed_parameters *parsed_parameters) { .... u_int32_t length = ....; .... if (length <= IFXNAMSIZ && length > 0) { memcpy(parsed_parameters->prohibited_interfaces[ num_prohibited_interfaces], value, length); parsed_parameters->prohibited_interfaces[ num_prohibited_interfaces][length - 1] = 0; .... }

PVS-Studio:
. :

 char prohibited_interfaces[IFXNAMSIZ][NECP_MAX_PARSED_PARAMETERS];

, :

 char prohibited_interfaces[NECP_MAX_PARSED_PARAMETERS][IFXNAMSIZ];

CWE-563: Assignment to Variable without Use


PVS-Studio CWE-563 . .

N40

 uint32_t gss_krb5_3des_unwrap_mbuf(....) { .... for (cflag = 1; cflag >= 0; cflag--) { *minor = gss_krb5_3des_token_get( ctx, &itoken, wrap, &hash, &offset, &length, reverse); if (*minor == 0) break; wrap.Seal_Alg[0] = 0xff; wrap.Seal_Alg[0] = 0xff; } .... }

PVS-Studio: V519 CWE-563 The 'wrap.Seal_Alg[0]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2070, 2071. gss_krb5_mech.c 2071

0xff . , :

 wrap.Seal_Alg[0] = 0xff; wrap.Seal_Alg[1] = 0xff;

, . … - .

PVS-Studio . !

N41, N42, N43, N44

 static struct mbuf * pf_reassemble(struct mbuf *m0, struct pf_fragment **frag, struct pf_frent *frent, int mff) { .... m->m_pkthdr.csum_flags &= ~CSUM_PARTIAL; m->m_pkthdr.csum_flags = CSUM_DATA_VALID | CSUM_PSEUDO_HDR | CSUM_IP_CHECKED | CSUM_IP_VALID; .... }

PVS-Studio: V519 CWE-563 The 'm->M_dat.MH.MH_pkthdr.csum_flags' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 758, 759. pf_norm.c 759

:

 m->m_pkthdr.csum_flags &= ~CSUM_PARTIAL;

. m->m_pkthdr.csum_flags . , , , '|'. , :

 m->m_pkthdr.csum_flags &= ~CSUM_PARTIAL; m->m_pkthdr.csum_flags |= CSUM_DATA_VALID | CSUM_PSEUDO_HDR | CSUM_IP_CHECKED | CSUM_IP_VALID;

3 , :

CWE-14: Compiler Removal of Code to Clear Buffers


, . , , :
  1. Safe Clearing of Private Data .
  2. V597 . The compiler could delete the 'memset' function call, which is used to flush 'Foo' buffer. The RtlSecureZeroMemory() function should be used to erase the private data.
  3. CWE-14 : Compiler Removal of Code to Clear Buffers.

, , , " — ? ".

, , , , , . , XNU Kernel .

N45

 __private_extern__ void YSHA1Final(unsigned char digest[20], YSHA1_CTX* context) { u_int32_t i, j; unsigned char finalcount[8]; .... /* Wipe variables */ i = j = 0; memset(context->buffer, 0, 64); memset(context->state, 0, 20); memset(context->count, 0, 8); memset(finalcount, 0, 8); // <= #ifdef SHA1HANDSOFF YSHA1Transform(context->state, context->buffer); #endif }

PVS-Studio: V597 CWE-14 The compiler could delete the 'memset' function call, which is used to flush 'finalcount' buffer. The memset_s() function should be used to erase the private data. sha1mod.c 188

Release- , "// <=". .

N46

 __private_extern__ void YSHA1Transform(u_int32_t state[5], const unsigned char buffer[64]) { u_int32_t a, b, c, d, e; .... state[0] += a; state[1] += b; state[2] += c; state[3] += d; state[4] += e; /* Wipe variables */ a = b = c = d = e = 0; }

PVS-Studio: V1001 CWE-563 The 'a' variable is assigned but is not used until the end of the function. sha1mod.c 120

, , .

, PVS-Studio CWE-563 . , CWE CWE-563. CWE-14, , .

CWE-783: Operator Precedence Logic Error


CWE-783 , , , . - .

N47

 int getxattr(....) { .... if ((error = copyinstr(uap->attrname, attrname, sizeof(attrname), &namelen) != 0)) { goto out; } .... out: .... return (error); }

PVS-Studio: V593 CWE-783 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. vfs_syscalls.c 10574

. ( proof ). , - .

, :

 Status s = foo(); if (s == Error) return s;

:

 Status s; if (s = foo() == Error) return s;

.
return , 1, , Error .

« » . «» , . - " , ". . :
XNU Kernel. , getxattr 1, .

N48 — N52

 static void memorystatus_init_snapshot_vmstats( memorystatus_jetsam_snapshot_t *snapshot) { kern_return_t kr = KERN_SUCCESS; mach_msg_type_number_t count = HOST_VM_INFO64_COUNT; vm_statistics64_data_t vm_stat; if ((kr = host_statistics64(.....) != KERN_SUCCESS)) { printf("memorystatus_init_jetsam_snapshot_stats: " "host_statistics64 failed with %d\n", kr); memset(&snapshot->stats, 0, sizeof(snapshot->stats)); } else { + .... }

PVS-Studio: V593 CWE-783 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. kern_memorystatus.c 4554

kr : 0 1. - printf 1, , host_statistics64 .

. , , . .

, :


CWE-758: Reliance on Undefined, Unspecified, or Implementation-Defined Behavior


C C++ . PVS-Studio , : V567, V610, V611, V681, V704, V708, V726, V736.

XNU CWE-758 , , .

N53, N54

 static void pfr_prepare_network(union sockaddr_union *sa, int af, int net) { .... sa->sin.sin_addr.s_addr = net ? htonl(-1 << (32-net)) : 0; .... }

PVS-Studio: V610 CWE-758 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. pf_table.c 976

. , . . This can be done like this:

 htonl((unsigned)(-1) << (32-net))

PVS-Studio : V610 CWE-758 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. pf_table.c 983

CWE-401: Improper Release of Memory Before Removing Last Reference ('Memory Leak')


XNU Kernel , ( CWE-401 ). 3 , delete . , .

N55, N56, N57

 IOService * IODTPlatformExpert::createNub(IORegistryEntry * from) { IOService * nub; nub = new IOPlatformDevice; if (nub) { if( !nub->init( from, gIODTPlane )) { nub->free(); nub = 0; } } return (nub); }

V773 CWE-401 The 'nub' pointer was assigned values twice without releasing the memory. A memory leak is possible. IOPlatformExpert.cpp 1287

init , . , delete, :

 if( !nub->init( from, gIODTPlane )) { nub->free(); delete nub; nub = 0; }

, . , free , «delete *this;». , , , .

:

CWE-129: Improper Validation of Array Index


CWE-129 , , . , .

N58 — N61

 IOReturn IOStateReporter::updateChannelValues(int channel_index) { .... state_index = _currentStates[channel_index]; if (channel_index < 0 || channel_index > (_nElements - state_index) / _channelDimension) { result = kIOReturnOverrun; goto finish; } .... }

PVS-Studio: V781 CWE-129 The value of the 'channel_index' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 852, 855. IOStateReporter.cpp 852

. , , .

, :

 IOReturn IOStateReporter::updateChannelValues(int channel_index) { .... if (channel_index < 0) { result = kIOReturnOverrun; goto finish; } state_index = _currentStates[channel_index]; if (channel_index > (_nElements - state_index) / _channelDimension) { result = kIOReturnOverrun; goto finish; } .... }

, , channel_index . , XNU Kernel.

:


CWE-480: Use of Incorrect Operator


CWE-480 - . , , , . , . , , .

N62

 #define NFS_UC_QUEUE_SLEEPING 0x0001 static void nfsrv_uc_proxy(socket_t so, void *arg, int waitflag) { .... if (myqueue->ucq_flags | NFS_UC_QUEUE_SLEEPING) wakeup(myqueue); .... }

PVS-Studio: V617 CWE-480 Consider inspecting the condition. The '0x0001' argument of the '|' bitwise operation contains a non-zero value. nfs_upcall.c 331

- «» , . , «» , . , :

 if (myqueue->ucq_flags & NFS_UC_QUEUE_SLEEPING) wakeup(myqueue);


CWE-665: Improper Initialization


PVS-Studio CWE. , CWE-665 .

N63

 extern void bzero(void *, size_t); static struct thread thread_template, init_thread; struct thread { .... struct thread_qos_override { struct thread_qos_override *override_next; uint32_t override_contended_resource_count; int16_t override_qos; int16_t override_resource_type; user_addr_t override_resource; } *overrides; .... }; void thread_bootstrap(void) { .... bzero(&thread_template.overrides, sizeof(thread_template.overrides)); .... }

PVS-Studio: V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'thread_template.overrides' class object. thread.c 377

, , , bzero . , nullptr .

bzero — . :

 thread_template.overrides = NULL;

, , . :

 bzero(thread_template.overrides, sizeof(*thread_template.overrides));

CWE-691: Insufficient Control Flow Management


CWE-691 . — , . XNU Kernel.

N64

, ! , , , , , , . , XNU Kernel , , , . 64, viva64 .

Note. , , «viva64», " 10 PVS-Studio ".

 void vm_page_release_startup(vm_page_t mem); void pmap_startup( vm_offset_t *startp, vm_offset_t *endp) { .... // -debug code remove if (2 == vm_himemory_mode) { for (i = 1; i <= pages_initialized; i++) { .... } } else // debug code remove- /* * Release pages in reverse order so that physical pages * initially get allocated in ascending addresses. This keeps * the devices (which must address physical memory) happy if * they require several consecutive pages. */ for (i = pages_initialized; i > 0; i--) { if(fill) fillPage(....); vm_page_release_startup(&vm_pages[i - 1]); } .... }

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

, . else . , , . (2 == vm_himemory_mode) .

Conclusion


macOS PVS-Studio , C C++. .

, PVS-Studio macOS.



, : Andrey Karpov. PVS-Studio is now available on macOS: 64 weaknesses in the XNU Kernel .

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/352206/

More articles:


All Articles