Thursday, September 17, 2015

bool b = -1; if(b) printk("Yes, -1 maps to true!");

 /* Assign -1 to bool variable */
 bool b = -1;  
 if(b)  
     printk("Yes, -1 maps to true!");  
  
 /* Return -1 in bool function */
 bool f(void)  
 {  
        return -1;  
 }  
  
 if (f())  
     printk("Yes, -1 maps to true!");  

Yesterday I was reading this interesting discussion about the boolean type in C. The most interesting sentence was:

"0 is false, 1 is true, any other value is *undefined behavior*."

Then I started to look for abuses of the bool type in the Linux kernel. I wrote simple semantic patches for getting cases in which a negative values are being returned by bool functions:

 @@  
 identifier f, ret;  
 constant C;  
 typedef bool;  
 @@  
 bool f (...){  
 <+...  
 ret = -C;  
 ...  
 * return ret;  
 ...+>  
 }  

and

 @@  
 identifier f;  
 constant C;  
 typedef bool;  
 @@  
 bool f (...){  
 <+...  
 * return -C;  
 ...+>  
 }  

The first search for boolean functions that assign negative value to a variable, and return the variable, while the second search for boolean functions returning negative values.

Coccinelle found 3 candidates for me:

 diff -u -p ./tools/testing/selftests/powerpc/pmu/lib.c /tmp/nothing/tools/testing/selftests/powerpc/pmu/lib.c  
 --- ./tools/testing/selftests/powerpc/pmu/lib.c  
 +++ /tmp/nothing/tools/testing/selftests/powerpc/pmu/lib.c  
 @@ -248,6 +248,5 @@ bool require_paranoia_below(int level)  
  out_close:  
     fclose(f);  
  out:  
 -    return rc;  
  }  
 diff -u -p ./arch/powerpc/kernel/module_64.c /tmp/nothing/arch/powerpc/kernel/module_64.c  
 --- ./arch/powerpc/kernel/module_64.c  
 +++ /tmp/nothing/arch/powerpc/kernel/module_64.c  
 @@ -160,7 +160,6 @@ bool is_module_trampoline(u32 *p)  
     BUILD_BUG_ON(sizeof(ppc64_stub_insns) != sizeof(ppc64_stub_mask));  
     if (probe_kernel_read(insns, p, sizeof(insns)))  
 -        return -EFAULT;  
     for (i = 0; i < ARRAY_SIZE(ppc64_stub_insns); i++) {  
         u32 insna = insns[i];  
 diff -u -p ./tools/perf/util/util.c /tmp/nothing/tools/perf/util/util.c  
 --- ./tools/perf/util/util.c  
 +++ /tmp/nothing/tools/perf/util/util.c  
 @@ -709,7 +709,6 @@ bool find_process(const char *name)  
     dir = opendir(procfs__mountpoint());  
     if (!dir)  
 -        return -1;  
     /* Walk through the directory. */  
     while (ret && (d = readdir(dir)) != NULL) {  

Finding probable bugs with Coccinelle is easy, verifying which of the candidates are real bugs demands a little bit more work. I was mostly interested in the arch/powerpc/kernel/module_64.c file as is the only candidate that is kernel code, the other two are inside tools directory. I created test code based on examples from the article about booleans. As the file is part of the PowerPC architecture, some cross compiling is needed, but that's not a problem for Fedora. For installing the cross compiler for PowerPC:

 $ sudo dnf install gcc-powerpc64-linux-gnu  

Now that I have the compiler I want to see how does the compiler maps -1 when returning from a boolean function. Two toy examples:

 t.c:  
 _Bool main(void)  
 {  
     return -1;  
 }  
  
 t2.c:  
 int main(void)  
 {  
     return -1;  
 }  

for compiling:

 $ powerpc64-linux-gnu-gcc -O2 -S -fomit-frame-pointer <c file>  

which genarated t.s and t2.s. What is interesting is that the assembly code for t1.c returns 1 while the assembly code for t2.c returns -1.

 t.s:  
 ...  
 .L.main:  
     li 3,1  
     blr  
  
 t2.s:  
 ...  
 .L.main:  
     li 3,-1  
     blr  

This convinced me that -1 was being mapped to true, but it would be better to see the impact of my change on the real source file. For cross compiling two environment variables should be set, and before compiling the kernel should be configured. After that, creating the .s file is easy:

 $ export ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu-  
 $ make allyesconfig  
 $ make arch/powerpc/kernel/module_64.s  

I did one make arch/powerpc/kernel/module_64.s for the original file returning -EFAULT, for a patched file returning false instead of -EFAULT, and for a patched file returning true instead of -EFAULT. Comparing this two files made it clear that -EFAULT is being mapped to true! The diff between the original .s file and the .s file returning false instead of -EFAULT starts with:

 440,441c440,441  
 < .L43:  
 <    li 3,1  # D.25775,  
 ---  
 > .L42:  
 >    li 3,0  # D.25775,  
 ...  

while the diff is empty comparing the original file and the version returning true instead of -EFAULT.

I sent patches earlier today. Let's see how it goes:



No comments:

Post a Comment