844513 - Add AddressSanitizer (ASan) memory check annotations to PLArena

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement, P2)

Description

[Mats Palmgren (inactive)]

Add memory check annotations to PLArena so that Firefox ASan builds
can detect illegal use of the part of the arena that hasn't been
PL_ARENA_ALLOCATE'ed yet.

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

This change is intended to only affect MOZILLA_CLIENT code.
The idea is to initially mark the malloc'ed memory as NOACCESS and then mark
each chunk that is allocated from the arena as UNDEFINED.
This enables ASan to detect any access to the "avail .. limit" area.

Comment 2

[Wan-Teh Chang]

Comment on attachment 717550 [details] [diff] [review]
fix

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

Thank you for the patch.

::: nsprpub/lib/ds/plarena.h
@@ +16,5 @@
>  #include "plarenas.h"
> +#ifdef MOZILLA_CLIENT
> +#include "../../../mozilla-config.h" /* for MOZ_ASAN and MOZ_VALGRIND defines */
> +#include "../mozilla/MemoryChecking.h" /* for MOZ_MAKE_MEM_* macros */
> +#endif

We need a better solution. We'll need to duplicate the MOZ_MAKE_MEM_*
macros in NSPR.

Comment 3

[Mats Palmgren (inactive)]

(In reply to Wan-Teh Chang from comment #2)
> We need a better solution. We'll need to duplicate the MOZ_MAKE_MEM_*
> macros in NSPR.

It's not clear to me what you're suggesting.  I guess I should copy
mfbt/MemoryChecking.h to nsprpub/lib/ds/MemoryChecking.h and then use:

> +#ifdef MOZILLA_CLIENT
> +#include "../../../mozilla-config.h" /* for MOZ_ASAN and MOZ_VALGRIND defines */
> +#endif
> +#include "MemoryChecking.h" /* for MOZ_MAKE_MEM_* macros */

If not, how should I get MOZ_ASAN and MOZ_VALGRIND that comes from
--enable-valgrind / --enable-address-sanitizer configure arguments?

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix, v2

No response so I'm assuming that's OK ;-)

https://tbpl.mozilla.org/?tree=Try&rev=479a10ce10c3

Comment 5

[Wan-Teh Chang]

Comment on attachment 730749 [details] [diff] [review]
fix, v2

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

Thanks for the patch and sorry for the late review.  I reviewed the patch
carefully today.  I suggest some changes.

Why is this bug marked as security-sensitive and labeled with the sec-other
keyword?

::: mfbt/MemoryChecking.h
@@ +1,1 @@
> +/* -*- Mode: C; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

When this file is copied to NSPR, it should be renamed.
I think we can rename it plmemcheck.h or plmarkmem.h.
Update the header include guard macro mozilla_MemoryChecking_h_
to match the new file name.

The macros defined in NSPR's copy of this header should
use the PL_ prefix instead of MOZ_.  So they should be
named PL_MAKE_MEM_NOACCESS, PL_MAKE_MEM_UNDEFINED, etc.

@@ +28,4 @@
>  
>  #if defined(MOZ_ASAN) || defined(MOZ_VALGRIND)
>  #define MOZ_HAVE_MEM_CHECKS 1
>  #endif

Delete these three lines from NSPR's copy of this header.

We can add PL_HAVE_MEM_CHECKS when we need it.

::: nsprpub/lib/ds/plarena.h
@@ +15,5 @@
>  #include "prtypes.h"
>  #include "plarenas.h"
> +#ifdef MOZILLA_CLIENT
> +#include "../mozilla-config.h" /* for MOZ_ASAN and MOZ_VALGRIND defines */
> +#endif

We need a better way to do this.  We should add --enable-address-sanitizer
and --enable-valgrind to nsprpub/configure.in, similar to how they were
added to js/src/configure.in:
http://mxr.mozilla.org/mozilla-central/source/js/src/configure.in

http://hg.mozilla.org/mozilla-central/rev/ad79ffdf94a3

I also wonder if NSPR should test its own versions of the MOZ_ASAN
and MOZ_VALGRIND macros: PL_ASAN and PL_VALGRIND.  It seems convenient
to just test MOZ_ASAN and MOZ_VALGRIND.  Otherwise any Mozilla code that
uses PLArena will need to be compiled with -DPL_ASAN -DPL_VALGRIND in
addition to -DMOZ_ASAN -DMOZ_VALGRIND.

::: security/coreconf/config.mk
@@ +176,5 @@
>  USE_UTIL_DIRECTLY = 1
>  
> +ifdef MOZILLA_CLIENT
> +DEFINES += -DMOZILLA_CLIENT=1
> +endif

We are trying to use "feature" make variables/C-preprocessor
macros.  So this should be:

# Define MOZ_ASAN or MOZ_VALGRIND for PLArena.
ifdef MOZ_ASAN
DEFINES += -DMOZ_ASAN
endif
ifdef MOZ_VALGRIND
DEFINES += -DMOZ_VALGRIND
endif

There is a way to pass MOZ_ASAN=1 or MOZ_VALGRIND=1 to the
NSS makefiles.  It used to be done in security/manager/Makefile.in.
But that makefile is no longer there. I don't know where it resides
now.

Comment 6

[Mats Palmgren (inactive)]

> Why is this bug marked as security-sensitive and labeled with the sec-other
> keyword?

It's security-sensitive as a precaution because the patch may reveal exploitable
bugs in shipped products.  We shouldn't make it public until we let our fuzz
team run with the patch for a while to confirm that's not the case.

It's sec-other just to give it a security rating - the bug in itself isn't
a security problem, since it's a tool.

Comment 7

[Mats Palmgren (inactive)]

Sorry, but I've lost interest in pursuing this.
I'm working on improving the pres shell arena and I think it's better
use of my time to finish that work (a side-effect is that it will
stop using plarena).
Feel free to use the patch as you see fit.

Comment 8

[Wan-Teh Chang]

Mats: no problem.

I researched AddressSanitizer and valgrind last night and have a better
understanding of this enhancement request.

If you are mainly interested in AddressSanitizer, the required changes
are small because AddressSanitizer builds can be detected using compiler
pre-defined macros (__has_feature(address_sanitizer) or __SANITIZE_ADDRESS__).
I can adapt your patch easily.  I also know how to add support for valgrind
and I can take care of that.

Comment 9

[Christian Holler (:decoder)]

(In reply to Wan-Teh Chang from comment #8)

> If you are mainly interested in AddressSanitizer, the required changes
> are small because AddressSanitizer builds can be detected using compiler
> pre-defined macros (__has_feature(address_sanitizer) or
> __SANITIZE_ADDRESS__).

Yep, that's right. I think we didn't use this because when I implemented MOZ_ASAN, this way of checking for ASan didn't exist yet, or it wasn't documented/well-known yet. Now all the sanitizers have such a way of detecting at compile-time which could make the --enable-address-sanitizer flag we got, obsolete.

Comment 10

[Wan-Teh Chang]

Attachment: Add ASan memory check annotations (manual poisoning) to PLArena, by Mats Palmgren

This adds ASan memory check annotations.  This does not require any
build system changes, so I suggest we do this first.

This patch is based on Mats's patch.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 733658 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, by Mats Palmgren

In the overview comment in plarena.h:
"... the following three macros are provided:
MOZ_MAKE_MEM_NOACCESS ..."

You probably want to change those three to use PL_

Other than that, I'll leave the actual code review for choller@

Comment 12

[Mats Palmgren (inactive)]

And thanks for picking this up Wan-Teh!
ASan support is indeed what I was looking for.

Comment 13

[Wan-Teh Chang]

Attachment: Add ASan memory check annotations (manual poisoning) to PLArena, v2, by Mats Palmgren

Mats: you're right. The comment should say PL_MAKE_MEM_. Thanks.

Comment 14

[Christian Holler (:decoder)]

Comment on attachment 733664 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v2, by Mats Palmgren

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

Looks good to me, r+ for the ASan stuff. Note that I'm not a peer for NSPR :)

::: lib/ds/plarena.h
@@ +180,5 @@
> +#define PL_CLEAR_ARENA(a) \
> +    PR_BEGIN_MACRO \
> +        PL_MAKE_MEM_UNDEFINED((void*)(a), (a)->limit - (PRUword)(a)); \
> +        memset((void*)(a), PL_FREE_PATTERN, (a)->limit - (PRUword)(a)); \
> +    PR_END_MACRO

Is the MAKE_MEM_UNDEFINED call here because the memory could have been marked NOACCESS before?

Comment 15

[Mats Palmgren (inactive)]

> Is the MAKE_MEM_UNDEFINED call here because the memory could have been
> marked NOACCESS before?

Correct.  See FreeArenaList and PL_ClearArenaPool for example.
The whole arena is marked NOACCESS when in arena_freelist, other in-use
arenas are partly NOACCESS, so when freeing/clearing those things, which
a client can do at will afaict, we need to temporarily mark the arena
UNDEFINED to allow for the memset.  In the case of PL_ClearArenaPool,
it is then promptly marked NOACCESS again, and in the case of FreeArenaList
the memory is free'd which also marks it NOACCESS of course.

Comment 16

[Mats Palmgren (inactive)]

Gary, Jesse, can you take this patch for a spin in the fuzzers before it lands?
To find any mistakes in the code, and to avoid zero-daying ourselves.  Thanks.

Comment 17

[Wan-Teh Chang]

decoder,mats: thank you for the review.

I will wait for the fuzzing report from Gary or Jesse before checking
this in.

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I haven't been working much on Firefox Asan builds recently, so I'll have to differ here.

Comment 19

[Jesse Ruderman]

Comments in the patch mention Valgrind, but the code only seems to support ASan?

Comment 20

[Wan-Teh Chang]

Jesse: yes, I will add Valgrind support later.

Comment 21

[Christian Holler (:decoder)]

For Valgrind support, just extend the macros as in mfbt/MemoryChecking.h.

Comment 22

[Jesse Ruderman]

I'll give this patch a spin and report back on Monday.

Comment 23

[Jesse Ruderman]

ASan DOM fuzzing with the patch didn't uncover any problems over the weekend.

Comment 24

[Wan-Teh Chang]

Attachment: Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

(Please consider the "superreview" request as a second review request.)

I found a bug in the change to PL_ARENA_GROW. You can see my fix by
diff'ing this patch against the v2 patch.

This MXR query shows PL_ARENA_GROW is only used by NSS:
http://mxr.mozilla.org/mozilla-central/ident?i=PL_ARENA_GROW

So this bug did not invalidate the ASan DOM fuzzing that Jesse did
last weekend.

Comment 25

[Wan-Teh Chang]

Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

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

I describe the bug in PL_ARENA_GROW below.

::: lib/ds/plarena.h
@@ +157,5 @@
>          PRUword _p = _a->avail; \
>          PRUword _q = _p + _incr; \
>          if (_p == (PRUword)(p) + PL_ARENA_ALIGN(pool, size) && \
>              _q <= _a->limit) { \
> +            PL_MAKE_MEM_UNDEFINED((void *)((PRUword)(p) + size), incr); \

In the v2 patch, this line is
    PL_MAKE_MEM_UNDEFINED((void *)_p, _incr); \

_p is _a->avail, which is the next aligned address after the last allocation
from the arena.  There may be a hole between the last allocated block and _p.
Therefore, PL_MAKE_MEM_UNDEFINED((void *)_p, _incr) will leave that hole with
the NOACCESS.

Comment 26

[Wan-Teh Chang]

Attachment: Just FYI: NSS patch

With this NSS patch, I can run the NSS test suite with no ASan errors.

The sqlite change is described in this Chromium bug report:
http://code.google.com/p/chromium/issues/detail?id=178677

The NSS secport.c change is necessary because that NSS function is
essentially part of PLArena.

Instructions for running NSS test suite under AddressSanitizer:

1. Install clang 3.3 or later on Linux.

2. Build NSS with this make command line:

make CC="clang -g -fsanitize=address" OPTIMIZER="-O1 -fno-omit-frame-pointer" DEFAULT_COMPILER=clang ZDEFS_FLAG= nss_build_all

3. Run NSS test suite (all.sh) as usual.

4. If there are any ASan errors, the call stacks in the output log do not
have function names. You need to use the asan_symbolize.py script to symbolize
the output log.

Comment 27

[Christian Holler (:decoder)]

Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

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

Sounds good to me :)

Comment 28

[Wan-Teh Chang]

Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

Pushed to the NSPR hg repository (NSPR 4.10):
https://hg.mozilla.org/projects/nspr/rev/608d1530807d

Comment 29

[Mats Palmgren (inactive)]

(In reply to Wan-Teh Chang from comment #25)
> There may be a hole between the last allocated block and _p.

Good catch!

I think the fix is wrong though.  It should work in ASan since it
doesn't detect a read of an UNDEFINED location that has never
been written to (it seems they are working on it though[1]), but it
will cause UMRs in Valgrind if/when you add support for it.

For the three areas - to old allocated area (A), the alignment gap (B),
the new area at the end (C).  These needs to be treated as such:

A: do not change!
B: mark as UNDEFINED
C: mark as UNDEFINED


[1] http://clang.llvm.org/docs/MemorySanitizer.html

Comment 30

[Mats Palmgren (inactive)]

Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

r+ since it should work for ASan, but please add a XXX comment
in the code and file a bug about it so it's not forgotten!

Comment 31

[Wan-Teh Chang]

(In reply to Mats Palmgren [:mats] from comment #29)
>
> For the three areas - to old allocated area (A), the alignment gap (B),
> the new area at the end (C).  These needs to be treated as such:
> 
> A: do not change!
> B: mark as UNDEFINED
> C: mark as UNDEFINED

Mats: thank you for your comment. I believe my fix does exactly what you
specified.

Here is my fix. I omitted the type casts for brevity:

 #define PL_ARENA_GROW(p, pool, size, incr) \
     PR_BEGIN_MACRO \
         ...
+            PL_MAKE_MEM_UNDEFINED(p + size, incr); \

A (old allocated data) is the byte range [p, p + size), which we do not
change.

B (alignment gap) may or may not exist. But if it exists, it will become
part of C (new area at the end) and marked as UNDEFINED below.

C (new area at the end) is the byte range [p + size, p + size + incr),
which we mark as UNDEFINED.

Is my analysis correct?

Comment 32

[Wan-Teh Chang]

Comment on attachment 741561 [details] [diff] [review]
Add ASan memory check annotations (manual poisoning) to PLArena, v3, by Mats Palmgren

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

::: lib/ds/plarena.h
@@ +157,5 @@
>          PRUword _p = _a->avail; \
>          PRUword _q = _p + _incr; \
>          if (_p == (PRUword)(p) + PL_ARENA_ALIGN(pool, size) && \
>              _q <= _a->limit) { \
> +            PL_MAKE_MEM_UNDEFINED((void *)((PRUword)(p) + size), incr); \

|p| is typically a void* pointer, so we can't perform pointer
arithmetic on |p| without typecasting.

Would it be clearer if I cast |p| to an unsigned char* pointer
(or even a char* pointer) instead?

            PL_MAKE_MEM_UNDEFINED((unsigned char *)(p) + size, incr); \

Comment 33

[Mats Palmgren (inactive)]

(In reply to Wan-Teh Chang from comment #31)
> +            PL_MAKE_MEM_UNDEFINED(p + size, incr); \

Ah, I misread the patch the first time, sorry.
Your analysis is correct and this is indeed the right fix.

Comment 34

[Mats Palmgren (inactive)]

(In reply to Wan-Teh Chang from comment #32)
Personally, I think "unsigned char*" would make the code clearer.

Comment 35

[Wan-Teh Chang]

Attachment: Use an unsigned char* typecast for pointer arithmetic

Comment 36

[Wan-Teh Chang]

Attachment: PL_ARENA_ALLOCATE should not let PL_ArenaAllocate unpoison the alignment gap. Use an unsigned char* typecast for pointer arithmetic.

Looking at PL_ARENA_ALLOCATE and PL_ArenaAllocate closer, I
noticed that the |nb| argument that PL_ARENA_ALLOCATE to
PL_ArenaAllocate is the aligned value |_nb|, which may be
larger than the original |nb|.  So when PL_ArenaAllocate
unpoisons the returned memory, it may unpoison more bytes
(the alignment gap) than it should.

This can be fixed by passing the original |nb| to
PL_ArenaAllocate. But I fix this by always unpoisoning
the returned memory in PL_ARENA_ALLOCATE.

Comment 37

[Mats Palmgren (inactive)]

Comment on attachment 742137 [details] [diff] [review]
PL_ARENA_ALLOCATE should not let PL_ArenaAllocate unpoison the alignment gap. Use an unsigned char* typecast for pointer arithmetic.

Another good catch!

But, I don't like removing the annotations for direct calls to
PL_ArenaAllocate.

Can we mark the gap (if any) as NOACCESS in PL_ARENA_ALLOCATE instead?
(directly after the call to PL_ArenaAllocate)

Comment 38

[Wan-Teh Chang]

Attachment: Alternative fix 1 for PL_ARENA_ALLOCATE: mark alignment gap as NOACCESS in PL_ARENA_ALLOCATE

Mats: these MXR queries show there are no direct calls to PL_ArenaAllocate
and its deprecated name PR_ArenaAllocate:
http://mxr.mozilla.org/mozilla-central/ident?i=PL_ArenaAllocate
http://mxr.mozilla.org/mozilla-central/ident?i=PR_ArenaAllocate

I inspected PL_ARENA_ALLOCATE and PL_ArenaAllocate, and convinced myself
that it doesn't make sense to call PL_ArenaAllocate directly. This is why
I think it's fine to always mark the allocated memory as UNDEFINED in
PL_ARENA_ALLOCATE.

This patch implements your proposed approach.  I am testing it with NSS
now.

I will attach another alternative patch later today.

Comment 39

[Wan-Teh Chang]

Attachment: Alternative fix 2 for PL_ARENA_ALLOCATE: pass the original |nb| argument to PL_ArenaAllocate

My "alternative fix 1" patch is broken. I can't spot where the bug is.

This is my alternative fix 2. The NSS test suite passed with this patch.

Comment 40

[Wan-Teh Chang]

Attachment: Alternative fix 1, v2, for PL_ARENA_ALLOCATE: mark alignment gap as NOACCESS in PL_ARENA_ALLOCATE

The bug seems to be that the macro argument |nb| needs to be
parenthesized in the expression |_nb - nb|. I found a
PL_ARENA_ALLOCATE call where |nb| is an expression:

    PL_ARENA_ALLOCATE(newp, pool, size + incr);

Comment 41

[Mats Palmgren (inactive)]

Comment on attachment 742652 [details] [diff] [review]
Alternative fix 1, v2, for PL_ARENA_ALLOCATE: mark alignment gap as NOACCESS in PL_ARENA_ALLOCATE

r=mats with a couple of nits below.
I slightly prefer this fix because it allows direct calls to
PL_ArenaAllocate just in case someone other than Mozilla does that.


>+            PL_MAKE_MEM_NOACCESS((void *)(_p + nb), _nb - (nb)); \

Please wrap the first "nb" in () too.

>+            PL_MAKE_MEM_UNDEFINED((unsigned char *)(p) + size, incr); \

Wrap "size".

Comment 42

[Mats Palmgren (inactive)]

Comment on attachment 742137 [details] [diff] [review]
PL_ARENA_ALLOCATE should not let PL_ArenaAllocate unpoison the alignment gap. Use an unsigned char* typecast for pointer arithmetic.

r=mats, but please wrap "nb" and "size" in ().

This looks fine too.

Comment 43

[Mats Palmgren (inactive)]

Comment on attachment 742647 [details] [diff] [review]
Alternative fix 2 for PL_ARENA_ALLOCATE: pass the original |nb| argument to PL_ArenaAllocate

This affects code that isn't memory annotations so I think it's
too invasive for this bug.  I think you should file a separate bug
if you want to do this cleanup (and ask a NSPR peer for review
which I'm not).

Comment 44

[Wan-Teh Chang]

In the hg commit message for
https://hg.mozilla.org/integration/mozilla-inbound/rev/0314d200873a
I referenced this bug by mistake. I meant to reference the NSS bug 866525.

If that changeset is pushed to mozilla-central, please update bug 866525
instead. Thanks.

Comment 45

[Wan-Teh Chang]

Attachment: PL_ARENA_ALLOCATE should not let PL_ArenaAllocate unpoison the alignment gap. Use an unsigned char* typecast for pointer arithmetic. v2

I found that none of the PL_Arena* friend functions should be
used directly, so I generalized the warning comment to apply
to all of them.

https://hg.mozilla.org/projects/nspr/rev/119d4ef3679c

Comment 46

[Wan-Teh Chang]

Marked the bug fixed.  Changed the summary to reflect what was actually
added.  Valgrind support will be added in a future NSPR release.