Bug 353192

Well AMD64 always supports NX so having a writable code section is bad news because it means you won't get any protection for it at run time. What sort of compiler/linker is producing such a thing?

GCC 4.8.2

Here's the Program Header section of the binary which doesn't get symbols resolved in Valgrind. Perhaps I missed something:

"""
Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align
  PHDR           0x0000000000000040 0x0000000000400040 0x0000000000400040
                 0x0000000000000188 0x0000000000000188  R E    8
  INTERP         0x00000000000001c8 0x00000000004001c8 0x00000000004001c8
                 0x000000000000001c 0x000000000000001c  R      1
      [Requesting program interpreter: /lib64/ld-linux-x86-64.so.2]
  LOAD           0x0000000000000000 0x0000000000400000 0x0000000000400000
                 0x000000000206b0c1 0x00000000021098f8  RWE    200000
  DYNAMIC        0x0000000002062c00 0x0000000002462c00 0x0000000002462c00
                 0x0000000000000350 0x0000000000000350  RW     8
  NOTE           0x00000000000001e4 0x00000000004001e4 0x00000000004001e4
                 0x0000000000000044 0x0000000000000044  R      4
  GNU_EH_FRAME   0x0000000001dde2c0 0x00000000021de2c0 0x00000000021de2c0
                 0x000000000000c9c4 0x000000000000c9c4  R      4
  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x0000000000000000 0x0000000000000000  RW     10
"""

Please let me know if there's more info I can provide.

I was bitten by this as well. I ended up with program headers that looked like this:

Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align
  LOAD           0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x00000000005eabf4 0x00000000005eabf4  R**W**E    200000
  LOAD           0x0000000000600000 0x0000000000800000 0x0000000000800000
                 0x000000000003b3f0 0x000000000004efe8  RW     200000
  DYNAMIC        0x000000000060f460 0x000000000080f460 0x000000000080f460
                 0x0000000000000180 0x0000000000000180  RW     8
  NOTE           0x00000000000001c8 0x00000000000001c8 0x00000000000001c8
                 0x0000000000000024 0x0000000000000024  R      4
  GNU_EH_FRAME   0x000000000057c050 0x000000000057c050 0x000000000057c050
                 0x000000000001600c 0x000000000001600c  R      4
  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x0000000000000000 0x0000000000000000  RW     10
  GNU_RELRO      0x0000000000600000 0x0000000000800000 0x0000000000800000
                 0x0000000000011000 0x0000000000011000  R      1

where the W flag in section 00 was caused by a combination of unusual __attribute__((__section__(foo))) annotations in GCC. I am also on Ubuntu, with amd64. I think this is properly called a bug, at least on amd64, because there is nothing in any standard that prevents .text sections from being mmaped with rwx permissions. 


As far as I can tell, moving amd64 to the same bucket as x86 and accepting r.x permissions would be harmless, since Valgrind will only try to read in debug symbols for a particular section if it finds *both* a section marked as executable *and* a .debug entry that corresponds to that section. I assume the rationale here is that if users are trying to treat a writeable section as a text section, then they're probably doing something wrong --- but this won't change existing behavior unless the user also provides debug info corresponding to that section (in which case they probably really do want to treat that section as text-like).

At the very least, emitting a warning when --trace-symbtab is turned on would be helpful, because this was very difficult to track down.

And if it's useful, I can put together a compileable example that displays this behavior. Please let me know.

I also have this issue. The reason I have an executable data segment is because I create a new section that is writable/executable for patchable code:

> .pushsection .genfuns,\"awx\",@progbits;
> [...]
> .popsection

This causes the linker to make the entire data segment RWX. Regardless of the security implications, it seems Valgrind should be able to debug the file with symbol info.


Also, while debugging Valgrind to see why it didn't load my symbols, I also encountered what seemed to be unintentional behavior in discard_syms_in_range(). On a completely unrelated munmap() call, it discarded the DebugInfo for my executable because of how the in-range test is formulated. It currently looks like this:

>         if (curr->text_present
>             && curr->text_size > 0
>             && (start+length - 1 < curr->text_avma 
>                 || curr->text_avma + curr->text_size - 1 < start)) {
>            /* no overlap */
>         } else {
>            found = True;
>            break;
>         }

This way, `found' is set not only when the range overlaps, but also when there is no range. I don't know if there is any information elsewhere that makes this meaningful, but it seems to me that the test should look like this instead:

>         if (curr->text_present && curr->text_size > 0) {
>             if (start+length - 1 < curr->text_avma 
>                 || curr->text_avma + curr->text_size - 1 < start) {
>                 /* no overlap */
>             } else {
>                 found = True;
>                 break;
>             }
>         }

Technically, I guess this should perhaps be another report, but since it doesn't cause any problems in and of itself, I wasn't sure how to report it. :)

Patrick, could you please supply a reproducible test case as you offered?

This is my reproducible testcase:

#include <stdlib.h>

asm(".pushsection .foo,\"awx\",@progbits;"
    ".type writeablefunction, @function;"
    "writeablefunction:"
    "ret;"
    ".popsection;");

int main(int argc, char **argv)
{
    malloc(128);
    return(0);
}

I compiled with "gcc -g -Wall -o vgtest vgtest.c", but I reckon it should be fairly tolerant with compiler flags. Valgrind output is:

$ valgrind --tool=memcheck --leak-check=full ./vgtest
==27841== Memcheck, a memory error detector
==27841== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==27841== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==27841== Command: ./vgtest
==27841== 
==27841== 
==27841== HEAP SUMMARY:
==27841==     in use at exit: 128 bytes in 1 blocks
==27841==   total heap usage: 1 allocs, 0 frees, 128 bytes allocated
==27841== 
==27841== 128 bytes in 1 blocks are definitely lost in loss record 1 of 1
==27841==    at 0x4C2BBAF: malloc (vg_replace_malloc.c:299)
==27841==    by 0x1086C8: ??? (in /tmp/vgtest)
==27841==    by 0x4E582B0: (below main) (libc-start.c:291)
==27841== 
==27841== LEAK SUMMARY:
==27841==    definitely lost: 128 bytes in 1 blocks
==27841==    indirectly lost: 0 bytes in 0 blocks
==27841==      possibly lost: 0 bytes in 0 blocks
==27841==    still reachable: 0 bytes in 0 blocks
==27841==         suppressed: 0 bytes in 0 blocks
==27841== 
==27841== For counts of detected and suppressed errors, rerun with: -v
==27841== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

To point in this case being the missing symbol for "main" in the loss record.

Also, this is a patch that fixes the issue for me. It does also include the fix I mentioned above.

--- valgrind-3.12.0~svn20160714.orig/coregrind/m_debuginfo/debuginfo.c
+++ valgrind-3.12.0~svn20160714/coregrind/m_debuginfo/debuginfo.c
@@ -359,14 +359,14 @@ static Bool discard_syms_in_range ( Addr
       while (True) {
          if (curr == NULL)
             break;
-         if (curr->text_present
-             && curr->text_size > 0
-             && (start+length - 1 < curr->text_avma 
-                 || curr->text_avma + curr->text_size - 1 < start)) {
-            /* no overlap */
-        } else {
-           found = True;
-           break;
+         if (curr->text_present && curr->text_size > 0) {
+           if (start+length - 1 < curr->text_avma 
+               || curr->text_avma + curr->text_size - 1 < start) {
+              /* no overlap */
+           } else {
+              found = True;
+              break;
+           }
         }
         curr = curr->next;
       }
@@ -944,10 +944,10 @@ ULong VG_(di_notify_mmap)( Addr a, Bool
    is_ro_map = False;
 
 #  if defined(VGA_x86) || defined(VGA_ppc32) || defined(VGA_mips32) \
-      || defined(VGA_mips64)
+      || defined(VGA_mips64) || defined(VGA_amd64)
    is_rx_map = seg->hasR && seg->hasX;
    is_rw_map = seg->hasR && seg->hasW;
-#  elif defined(VGA_amd64) || defined(VGA_ppc64be) || defined(VGA_ppc64le)  \
+#  elif defined(VGA_ppc64be) || defined(VGA_ppc64le)  \
         || defined(VGA_arm) || defined(VGA_arm64)
    is_rx_map = seg->hasR && seg->hasX && !seg->hasW;
    is_rw_map = seg->hasR && seg->hasW && !seg->hasX;

This is against Debian's source tree, however. I hope that doesn't cause too much problem.