1436773 - Initialize LifoAlloc memory to some non-zero value when reserved

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Nicolas B. Pierron [:nbp]]

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.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Clean-up mem check instrumentation of LifoAlloc's BumpChunks.

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.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Enable LifoAlloc's mem checks on nightly.

Toggle memset poisoning of LifoAlloc on nightly builds.

Comment 3

[Ted Campbell [:tcampbell]]

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

Comment 4

[Ted Campbell [:tcampbell]]

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.

Comment 5

[Jan de Mooij [:jandem]]

Good idea. Do you know if/how this affects performance?

Comment 6

[Nicolas B. Pierron [:nbp]]

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 7

[Ted Campbell [:tcampbell]]

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.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: Clean-up mem check instrumentation of LifoAlloc's BumpChunks.

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)

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: Initialized BumpChunk memory as non-accessible instead of undefined.

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.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Enable LifoAlloc's mem checks on nightly.

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.

Comment 11

[Ted Campbell [:tcampbell]]

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!

Comment 12

[Ted Campbell [:tcampbell]]

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.

Comment 13

[Ted Campbell [:tcampbell]]

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.

Comment 14

[Nicolas B. Pierron [:nbp]]

(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.

Comment 15

[Nicolas B. Pierron [:nbp]]

(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?!)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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 17

[Nicolas B. Pierron [:nbp]]

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.

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/47b2a7db1ac3
https://hg.mozilla.org/mozilla-central/rev/d12cc905dbcd