1267557 - Some code poisoning changes

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Poison header bytes

When we allocate executable code from a pool, there may be a few alignment bytes before the actual code. This patch poisons those "headerSize" bytes too. I noticed this in a few crash reports.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Change poison values

This patch also removes the unused JS_SWEPT_FRAME_PATTERN constant.

Not sure which value to use on MIPS or MIPS64. Most odd values I tried are valid MIPS instructions, so I'll let the MIPS gurus find one.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8745262 [details] [diff] [review]
Part 2 - Change poison values

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

::: js/public/Utility.h
@@ +49,5 @@
>  #define JS_ALLOCATED_TENURED_PATTERN 0x4D
>  #define JS_EMPTY_STOREBUFFER_PATTERN 0x1B
> +
> +#if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64) || defined(JS_CODEGEN_NONE)
> +# define JS_SWEPT_CODE_PATTERN 0xED // IN (%dx),%eax

LLDB disassembles this as: IN %dx, %eax

I'll change this to something like: // IN instruction, will crash.

Comment 4

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8745262 [details] [diff] [review]
Part 2 - Change poison values

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

::: js/public/Utility.h
@@ +53,5 @@
> +# define JS_SWEPT_CODE_PATTERN 0xED // IN (%dx),%eax
> +#elif defined(JS_CODEGEN_ARM) || defined(JS_CODEGEN_ARM64)
> +# define JS_SWEPT_CODE_PATTERN 0xA3 // undefined instruction
> +#else
> +# error "JS_SWEPT_CODE_PATTERN not defined for this platform"

nit: Add a comment above this to mention that we are looking for a pattern which, when repeated to fill an instruction is either an illegal instruction (in user mode), or not the code of any valid instruction.

Comment 5

[Nicolas B. Pierron [:nbp] {away}]

See part 2, which is adding a poison value in order to avoid instruction slides.

Comment 6

[hev [:hev]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> See part 2, which is adding a poison value in order to avoid instruction
> slides.

I suggest define JS_SWEPT_CODE_PATTERN = 0x01 for mips, this instruction encoding '0x01010101' is invalid on all mips ISAs I know (mips1/mips2/mips3/mips4/mips5/mips32r1/mips32r2/mips32r3/mips32r5/mips32r6/mips64r1/mips64r2/mips64r3/mips64r5 and mips64r6).

Comment 7

[hev [:hev]]

Attachment: Part 3 - Change poison values for MIPS

Comment 8

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8745846 [details] [diff] [review]
Part 3 - Change poison values for MIPS

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

Thanks :)

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 0 - Move poison values

These patches fail to compile the browser, because the JS_CODEGEN_* constants are not defined when we #include js/public/Utility.h in code outside SpiderMonkey.

This patch moves the poison values defines to jsutil.h. I also removed some unused values in the process, and added a missing value to IsThingPoisoned.

Comment 10

[Jon Coppeard (:jonco) (PTO until 14th September)]

Comment on attachment 8746425 [details] [diff] [review]
Part 0 - Move poison values

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

They're not used outside the engine so this seems fine.

::: js/src/gc/Marking.cpp
@@ +125,5 @@
>          JS_FRESH_NURSERY_PATTERN,
>          JS_SWEPT_NURSERY_PATTERN,
>          JS_ALLOCATED_NURSERY_PATTERN,
>          JS_FRESH_TENURED_PATTERN,
> +        JS_MOVED_TENURED_PATTERN,

Thanks for updating this.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bccf6d5c4c05
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e9d2cc7c629
https://hg.mozilla.org/integration/mozilla-inbound/rev/16391def96e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f792a06e6ae

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bccf6d5c4c05
https://hg.mozilla.org/mozilla-central/rev/1e9d2cc7c629
https://hg.mozilla.org/mozilla-central/rev/16391def96e4
https://hg.mozilla.org/mozilla-central/rev/9f792a06e6ae

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8746425 [details] [diff] [review]
Part 0 - Move poison values

Requesting approval for all 4 patches.

These patches change our JIT code poisoning to be safer and to make it easier to diagnose crashes. It ensures we always crash when we attempt to execute poisoned code.

Approval Request Comment
[Feature/regressing bug #]: -
[User impact if declined]: Patches help diagnose crashes and make it harder to exploit potential security issues.
[Describe test coverage new/current, TreeHerder]: On m-c for about a week without problems.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Jan, have we looked at data from Nightly (a few sample crash reports where this applies) to validate that this patch is indeed helping improve root causing some class of crashes? I was wondering if we should wait for that kind of validation before uplifting this to Beta47. What do you think?

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8746425 [details] [diff] [review]
Part 0 - Move poison values

Let's uplift to Aurora48 and stabilize before uplifting to Beta47.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e122f4f0fa72
https://hg.mozilla.org/releases/mozilla-aurora/rev/29d746b57ed2
https://hg.mozilla.org/releases/mozilla-aurora/rev/2fa3f71011cc
https://hg.mozilla.org/releases/mozilla-aurora/rev/fa1e3821e0e4

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Ritu Kothari (:ritu) from comment #14)
> Hello Jan, have we looked at data from Nightly (a few sample crash reports
> where this applies) to validate that this patch is indeed helping improve
> root causing some class of crashes? I was wondering if we should wait for
> that kind of validation before uplifting this to Beta47. What do you think?

We are not seeing a lot of those crashes on Nightly so it's hard to say, but here's one where we now crash while executing the new poison pattern: bp-cd18691b-5153-43cb-b9b9-736232160506. Before, we might have continued executing the code after that and the report would have been much more confusing to categorize.

I think this is worth uplifting, as it's low risk (on m-c for at least 10 days) and it turns "might crash when something bad happens" into "will definitely crash when something bad happens", that's a nice security improvement.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Jan de Mooij [:jandem] from comment #17)
> (In reply to Ritu Kothari (:ritu) from comment #14)
> > Hello Jan, have we looked at data from Nightly (a few sample crash reports
> > where this applies) to validate that this patch is indeed helping improve
> > root causing some class of crashes? I was wondering if we should wait for
> > that kind of validation before uplifting this to Beta47. What do you think?
> 
> We are not seeing a lot of those crashes on Nightly so it's hard to say, but
> here's one where we now crash while executing the new poison pattern:
> bp-cd18691b-5153-43cb-b9b9-736232160506. Before, we might have continued
> executing the code after that and the report would have been much more
> confusing to categorize.
> 
> I think this is worth uplifting, as it's low risk (on m-c for at least 10
> days) and it turns "might crash when something bad happens" into "will
> definitely crash when something bad happens", that's a nice security
> improvement.

Thank you for your due diligence. I am glad to hear that there is at least some data that this is helping. I will approve uplift to Beta.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8746425 [details] [diff] [review]
Part 0 - Move poison values

This has baken in Aurora for a few days, Jan has confirmed in comment 18 that this code is helping crash diagnostics, ESR45+, Beta47+

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Part 1 doesn't appear to apply cleanly to esr45.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/507d849ea8f4
https://hg.mozilla.org/releases/mozilla-beta/rev/6a5cce634618
https://hg.mozilla.org/releases/mozilla-beta/rev/686cf3f6d74a
https://hg.mozilla.org/releases/mozilla-beta/rev/cb18fea52de4

Comment 22

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/d692c9984963
https://hg.mozilla.org/releases/mozilla-esr45/rev/47887256a413
https://hg.mozilla.org/releases/mozilla-esr45/rev/8401c10ae0a5
https://hg.mozilla.org/releases/mozilla-esr45/rev/c48f83805d40