Closed Bug 1267557 Opened 8 years ago Closed 8 years ago

Some code poisoning changes

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed
firefox-esr45 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(4 files)

      No description provided.
See Also: → 1044077
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.
Attachment #8745260 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8745262 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8745260 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8745262 - Flags: review?(nicolas.b.pierron) → review+
See part 2, which is adding a poison value in order to avoid instruction slides.
Flags: needinfo?(r)
Flags: needinfo?(branislav.rankov)
(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).
Flags: needinfo?(r)
Attachment #8745846 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8745846 [details] [diff] [review]
Part 3 - Change poison values for MIPS

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

Thanks :)
Attachment #8745846 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8746425 - Flags: review?(jcoppeard)
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.
Attachment #8746425 - Flags: review?(jcoppeard) → review+
Flags: needinfo?(branislav.rankov)
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.
Attachment #8746425 - Flags: approval-mozilla-esr45?
Attachment #8746425 - Flags: approval-mozilla-beta?
Attachment #8746425 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(jdemooij)
Comment on attachment 8746425 [details] [diff] [review]
Part 0 - Move poison values

Let's uplift to Aurora48 and stabilize before uplifting to Beta47.
Attachment #8746425 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Flags: needinfo?(jdemooij)
(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 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+
Attachment #8746425 - Flags: approval-mozilla-esr45?
Attachment #8746425 - Flags: approval-mozilla-esr45+
Attachment #8746425 - Flags: approval-mozilla-beta?
Attachment #8746425 - Flags: approval-mozilla-beta+
Part 1 doesn't appear to apply cleanly to esr45.
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.