[Bug tree-optimization/52798] New: __builtin_object_size() based overflow check is a false positive due to parameter optimisation

[gcc at breakpoint dot cc]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52798

             Bug #: 52798
           Summary: __builtin_object_size() based overflow check is a
                    false positive due to parameter optimisation
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassig...@gcc.gnu.org
        ReportedBy: g...@breakpoint.cc
              Host: x86_64-linux-gnu
            Target: x86_64-linux-gnu
             Build: x86_64-linux-gnu


This is slightly modified from linux kernel fs/binfmt_misc.c:

 static int parse_command(const char __user *buffer, size_t count)
 {       
         char s[40];
         if (!count)
                 return 0;
         if (count > 3)
                 return -EINVAL;
         if (count == 1 || count == 2 || count == 3)
                 if (copy_from_user(s, buffer, count))
                         return -EFAULT;
         if (copy_from_user(s, buffer, count))
                 return -EFAULT;
}

Each copy_from_user() has a __builtin_object_size() based check for length of s
vs length specified in count. The s buffer is 40bytes, we return if count >3.
copy_from_user() produces a #error/#warning for in the overflow case where
count is larger than s.
Since we do check for count >3 and abort, there is no overflow.
gcc on the other hand detects one. Both calls are in my opinion the same. Using
only the first check (count ==1 || count...) avoids the warning, the second
triggers the warning.

The short version of the assembly looks the following way (param via regs in
x86-32, count in edx):
000007e0 <parse_command.part.2>:
    8d 72 ff                lea    -0x1(%edx),%esi
    83 fe 02                cmp    $0x2,%esi
    77 5d                   ja     858 <parse_command.part.2+0x78>

(esi - 1) compared against 2, so this is the >3 check (and 0 I guess).
#1 call to the copy routine

   89 d9                   mov    %ebx,%ecx
   89 fa                   mov    %edi,%edx
   e8 fc ff ff ff          call   81a <parse_command.part.2+0x3a>
#2 call to the copy routine. There is no check for ebx/ecx in between, so the
count check from #1 is 
recognized as unchanged and valid.

This looks to me like gcc knows the exact (and correct) size of s in both cases
but somehow an optimization pass removes it (because it redundant) and the gcc
threats it as not available and therefore produces the warning.

The copy routine looks the following:

 static inline unsigned long __must_check copy_from_user(void *to,
                                           const void __user *from,
                                           unsigned long n)
 {
         int sz = __compiletime_object_size(to);
/*         asm volatile(""); */

         if (likely(sz == -1 || sz >= n))
                 n = _copy_from_user(to, from, n);
         else
                 copy_from_user_overflow();

         return n;
 }

Adding the asm volatile statement (where it is commented out) adds the
follwoing code before the second call:

  83 fb 28                cmp    $0x28,%ebx
  77 65                   ja     875 <parse_command.part.2+0x95>

So here the compiler really assumes that there is no upper limit of count.

I attach the .i output of the testcase which can be compiled with:
 gcc -O2 -m32 -pipe -c -o binfmt_misc.o binfmt_misc.i

The outout "error: call to ‘co......" is returned if gcc detects the overflow.

Sebastian