From: Julian Seward Date: Wed, 27 Jan 2010 10:28:00 +0000 (+0000) Subject: Fix handling of mprotect so as to be more consistent with the handling X-Git-Tag: svn/VALGRIND_3_6_0~393 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=96b3024832c964aeeddfa9e4c6a6dc725c054325;p=thirdparty%2Fvalgrind.git Fix handling of mprotect so as to be more consistent with the handling of mmap. Fixes #205541 and its dup #210268. The fix is simple enough but the analysis is a bit complex, as detailed in comments. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@11031 --- diff --git a/memcheck/mc_main.c b/memcheck/mc_main.c index c262d69d76..119cf2339f 100644 --- a/memcheck/mc_main.c +++ b/memcheck/mc_main.c @@ -1636,6 +1636,22 @@ static void make_mem_defined_if_addressable ( Addr a, SizeT len ) } } +/* Similarly (needed for mprotect handling ..) */ +static void make_mem_defined_if_noaccess ( Addr a, SizeT len ) +{ + SizeT i; + UChar vabits2; + DEBUG("make_mem_defined_if_noaccess(%p, %llu)\n", a, (ULong)len); + for (i = 0; i < len; i++) { + vabits2 = get_vabits2( a+i ); + if (LIKELY(VA_BITS2_NOACCESS == vabits2)) { + set_vabits2(a+i, VA_BITS2_DEFINED); + if (UNLIKELY(MC_(clo_mc_level) >= 3)) { + MC_(helperc_b_store1)( a+i, 0 ); /* clear the origin tag */ + } + } + } +} /* --- Block-copy permissions (needed for implementing realloc() and sys_mremap). --- */ @@ -3701,16 +3717,86 @@ void check_mem_is_defined_asciiz ( CorePart part, ThreadId tid, } } +/* Handling of mmap and mprotect is not as simple as it seems. + + The underlying semantics are that memory obtained from mmap is + always initialised, but may be inaccessible. And changes to the + protection of memory do not change its contents and hence not its + definedness state. Problem is we can't model + inaccessible-but-with-some-definedness state; once we mark memory + as inaccessible we lose all info about definedness, and so can't + restore that if it is later made accessible again. + + One obvious thing to do is this: + + mmap/mprotect NONE -> noaccess + mmap/mprotect other -> defined + + The problem case here is: taking accessible memory, writing + uninitialised data to it, mprotecting it NONE and later mprotecting + it back to some accessible state causes the undefinedness to be + lost. + + A better proposal is: + + (1) mmap NONE -> make noaccess + (2) mmap other -> make defined + + (3) mprotect NONE -> # no change + (4) mprotect other -> change any "noaccess" to "defined" + + (2) is OK because memory newly obtained from mmap really is defined + (zeroed out by the kernel -- doing anything else would + constitute a massive security hole.) + + (1) is OK because the only way to make the memory usable is via + (4), in which case we also wind up correctly marking it all as + defined. + + (3) is the weak case. We choose not to change memory state. + (presumably the range is in some mixture of "defined" and + "undefined", viz, accessible but with arbitrary V bits). Doing + nothing means we retain the V bits, so that if the memory is + later mprotected "other", the V bits remain unchanged, so there + can be no false negatives. The bad effect is that if there's + an access in the area, then MC cannot warn; but at least we'll + get a SEGV to show, so it's better than nothing. + + Consider the sequence (3) followed by (4). Any memory that was + "defined" or "undefined" previously retains its state (as + required). Any memory that was "noaccess" before can only have + been made that way by (1), and so it's OK to change it to + "defined". + + See https://bugs.kde.org/show_bug.cgi?id=205541 + and https://bugs.kde.org/show_bug.cgi?id=210268 +*/ static void mc_new_mem_mmap ( Addr a, SizeT len, Bool rr, Bool ww, Bool xx, ULong di_handle ) { - if (rr || ww || xx) + if (rr || ww || xx) { + /* (2) mmap/mprotect other -> defined */ MC_(make_mem_defined)(a, len); - else + } else { + /* (1) mmap/mprotect NONE -> noaccess */ MC_(make_mem_noaccess)(a, len); + } } +static +void mc_new_mem_mprotect ( Addr a, SizeT len, Bool rr, Bool ww, Bool xx ) +{ + if (rr || ww || xx) { + /* (4) mprotect other -> change any "noaccess" to "defined" */ + make_mem_defined_if_noaccess(a, len); + } else { + /* (3) mprotect NONE -> # no change */ + /* do nothing */ + } +} + + static void mc_new_mem_startup( Addr a, SizeT len, Bool rr, Bool ww, Bool xx, ULong di_handle ) @@ -5780,7 +5866,7 @@ static void mc_pre_clo_init(void) // // So we should arguably observe all this. However: // - The current inaccuracy has caused maybe one complaint in seven years(?) - // - Telying on the zeroed-ness of whole brk'd pages is pretty grotty... I + // - Relying on the zeroed-ness of whole brk'd pages is pretty grotty... I // doubt most programmers know the above information. // So I'm not terribly unhappy with marking it as undefined. --njn. // @@ -5790,21 +5876,15 @@ static void mc_pre_clo_init(void) // just mark all memory it allocates as defined.] // VG_(track_new_mem_brk) ( make_mem_undefined_w_tid ); + + // Handling of mmap and mprotect isn't simple (well, it is simple, + // but the justification isn't.) See comments above, just prior to + // mc_new_mem_mmap. VG_(track_new_mem_mmap) ( mc_new_mem_mmap ); + VG_(track_change_mem_mprotect) ( mc_new_mem_mprotect ); VG_(track_copy_mem_remap) ( MC_(copy_address_range_state) ); - // Nb: we don't do anything with mprotect. This means that V bits are - // preserved if a program, for example, marks some memory as inaccessible - // and then later marks it as accessible again. - // - // If an access violation occurs (eg. writing to read-only memory) we let - // it fault and print an informative termination message. This doesn't - // happen if the program catches the signal, though, which is bad. If we - // had two A bits (for readability and writability) that were completely - // distinct from V bits, then we could handle all this properly. - VG_(track_change_mem_mprotect) ( NULL ); - VG_(track_die_mem_stack_signal)( MC_(make_mem_noaccess) ); VG_(track_die_mem_brk) ( MC_(make_mem_noaccess) ); VG_(track_die_mem_munmap) ( MC_(make_mem_noaccess) );