Closed Bug 1436773 Opened 4 years ago Closed 4 years ago

Initialize LifoAlloc memory to some non-zero value when reserved

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(3 files, 2 obsolete files)

When LifoAlloc memory is requested, we get uninitialized memory from the allocator.  This uninitialized memory is hard to distinguish from free-d memory when manipulating LifoAlloc content.

Thus, I suggest we initialize all LifoAlloc memory when we bump the pointer of a chunk, such that we can distinguish between UAF, and uninitialized memory in crash reports.
This patch factor the memset and the MOZ_MAKE_MEM_* macros under the
LIFO_MAKE_MEM_* macros.

This allow to disable memset in builds which are already being checked with more
efficient mechanism, such as ASan and MSan. The memset is kept for valgrind
builds, as build instrumented with valgrind are not always being executed under
valgrind (for performance reasons).

This simplifies the body of BumpChunk::setBump function, and removes the prev
temporary pointer.
Attachment #8949442 - Flags: review?(tcampbell)
Toggle memset poisoning of LifoAlloc on nightly builds.
Attachment #8949443 - Flags: review?(tcampbell)
Priority: -- → P3
Comment on attachment 8949442 [details] [diff] [review]
Clean-up mem check instrumentation of LifoAlloc's BumpChunks.

Review of attachment 8949442 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense to me
Attachment #8949442 - Flags: review?(tcampbell) → review+
Comment on attachment 8949443 [details] [diff] [review]
Enable LifoAlloc's mem checks on nightly.

Review of attachment 8949443 [details] [diff] [review]:
-----------------------------------------------------------------

Let's see if this gives us any insight.
Attachment #8949443 - Flags: review?(tcampbell) → review+
Good idea. Do you know if/how this affects performance?
Comment on attachment 8949442 [details] [diff] [review]
Clean-up mem check instrumentation of LifoAlloc's BumpChunks.

Review of attachment 8949442 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ds/LifoAlloc.h
@@ +226,5 @@
>  
> +    // Note, that we still instrument with memset if valgrind is enabled,
> +    // because valgrind is not enabled by default when running the executable,
> +    // as opposed to ASan and MSan.
> +#if defined(DEBUG) || defined(MOZ_VALGRIND)

Try highlighted a flaw in this logic which is that we can enable valgrind and asan at the same time in the same build, even if we only generate ASan instrumentation in such cases.

@@ +247,5 @@
> +    do {                                             \
> +        uint8_t* base = (addr);                      \
> +        size_t sz = (size);                          \
> +        memset(base, uninitializedChunkMemory, sz);  \
> +        MOZ_MAKE_MEM_UNDEFINED(base, sz);            \

self-nit: The memory is marked as no-access when free (but not when allocated), this causes Try failures when ASan is enabled, this should be probably be converted into:
  	
        MOZ_MAKE_MEM_UNDEFINED(base, sz);
        memset(base, uninitializedChunkMemory, sz);
        MOZ_MAKE_MEM_UNDEFINED(base, sz);

and I will make another patch to mark the newly allocated BumpChunk as NOACCESS.
Comment on attachment 8949442 [details] [diff] [review]
Clean-up mem check instrumentation of LifoAlloc's BumpChunks.

I'd like to see the modifications you propose, so clearing review for now.
Attachment #8949442 - Flags: review+
Delta:
 - Mark as undefined before using the uninitialized pattern after BumpChunk::release (noaccess).
 - Remove the comment about Valgrind, and always do memset on debug builds.
   (checking that MSan, ASan and others would be verbose not not maintainable)
Attachment #8949750 - Flags: review?(tcampbell)
Attachment #8949442 - Attachment is obsolete: true
Currently, when a BumpChunk is allocated all the memory of the BumpChunk is
writtable. This patch make sure that the memory is not writtable, nor readable
until it is being allocated from the BumpChunk.
Attachment #8949752 - Flags: review?(tcampbell)
If disgnostic assertions are enabled, then we should use memset to mark
uninitialized chunks differently than recently freed chnuks.

If the uninitialize patterns appear, this would highlight a problem in the code
which is using the LifoAlloc. If the jemalloc poison appear, this implies that
the pointer of the Bumpchunk is being used in different places.
Attachment #8949753 - Flags: review?(tcampbell)
Attachment #8949443 - Attachment is obsolete: true
Attachment #8949443 - Flags: review+
Comment on attachment 8949752 [details] [diff] [review]
Initialized BumpChunk memory as non-accessible instead of undefined.

Review of attachment 8949752 [details] [diff] [review]:
-----------------------------------------------------------------

:) Great idea!
Attachment #8949752 - Flags: review?(tcampbell) → review+
Comment on attachment 8949750 [details] [diff] [review]
Clean-up mem check instrumentation of LifoAlloc's BumpChunks.

Review of attachment 8949750 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the change summary.
Attachment #8949750 - Flags: review?(tcampbell) → review+
Comment on attachment 8949753 [details] [diff] [review]
Enable LifoAlloc's mem checks on nightly.

Review of attachment 8949753 [details] [diff] [review]:
-----------------------------------------------------------------

Agree that MOZ_DIAGNOSTIC_ASSERT_ENABLED will be needed to get anything meaningful back.
Attachment #8949753 - Flags: review?(tcampbell) → review+
Depends on: 1437125
(In reply to Jan de Mooij [:jandem] from comment #5)
> Good idea. Do you know if/how this affects performance?

This is a huge perf hit.
Octane goes from ~30500 down to ~23000 on my laptop.
(In reply to Nicolas B. Pierron [:nbp] from comment #14)
> (In reply to Jan de Mooij [:jandem] from comment #5)
> > Good idea. Do you know if/how this affects performance?
> 
> This is a huge perf hit.
> Octane goes from ~30500 down to ~23000 on my laptop.

This is almost the same perf hit if we have a single memset or 2.

So, I think landing this the last patch which enables it when  MOZ_DIAGNOSTIC_ASSERT_ENABLED  is set, should be kept for times when we have a huge volume on nightly / beta.

I will land the other patches, and tests other forms of mitigations.  The parser (Octane-CodeLoad) seems to be the most impacted by these memset. (and strangely Octane-Raytrace seems to benefit from it?!)
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47b2a7db1ac3
Clean-up mem check instrumentation of LifoAlloc's BumpChunks. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/d12cc905dbcd
Initialized BumpChunk memory as non-accessible instead of undefined. r=tcampbell
Comment on attachment 8949753 [details] [diff] [review]
Enable LifoAlloc's mem checks on nightly.

This patch was not pushed as part of comment 16, due to expected performance regressions (comment 14), and might be landed later if we see a spike of nightly / beta crashes to investigate.
Attachment #8949753 - Flags: checkin-
https://hg.mozilla.org/mozilla-central/rev/47b2a7db1ac3
https://hg.mozilla.org/mozilla-central/rev/d12cc905dbcd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.