Closed Bug 1307296 Opened 3 years ago Closed 3 years ago

Check for rooting hazards arising from pointers into possibly-inlined typed object memory

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

(Keywords: sec-audit, Whiteboard: [adv-main52-])

Attachments

(2 files)

Bug for catching things like bug 1296640 with the rooting hazard analysis.

Elem* val = typedObj->typedMem();
...GC...
f(val[0]); <-- hazard! memory might have moved
I don't know if you'd prefer I used TypedObjectElemArray whenever possible. But it's probably always going to be a mixture -- sometimes we're memcpy'ing or something, so single-element access isn't good enough. I suppose I could add a get() or autoconvert.

I'm kind of tempted to change the exposed typedMem() to require a nogc parameter, with a private accessor for internal use. But maybe that's overkill?
Attachment #8797307 - Flags: review?(jcoppeard)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8797307 [details] [diff] [review]
Avoid GC hazards when accessing typed object data

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

Nice.

::: js/src/builtin/SIMD.cpp
@@ +188,5 @@
>  template bool js::ToSimdConstant<Bool32x4>(JSContext* cx, HandleValue v, jit::SimdConstant* out);
>  
>  template<typename Elem>
>  static Elem
> +TypedObjectMemory(HandleValue v, const JS::AutoAssertOnGC&)

If this is not called anywhere outside of the TypedObjectElemArray constructor, can we inline it into that?
Attachment #8797307 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #2)
> Comment on attachment 8797307 [details] [diff] [review]
> Avoid GC hazards when accessing typed object data
> 
> Review of attachment 8797307 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice.
> 
> ::: js/src/builtin/SIMD.cpp
> @@ +188,5 @@
> >  template bool js::ToSimdConstant<Bool32x4>(JSContext* cx, HandleValue v, jit::SimdConstant* out);
> >  
> >  template<typename Elem>
> >  static Elem
> > +TypedObjectMemory(HandleValue v, const JS::AutoAssertOnGC&)
> 
> If this is not called anywhere outside of the TypedObjectElemArray
> constructor, can we inline it into that?

It is still called in several places where TypedObjectElemArray is not applicable.
Pull out the big stick. This would have prevented at least one sec-crit that I know of (now fixed).
Attachment #8797829 - Flags: review?(jcoppeard)
Depends on: 1303780
Comment on attachment 8797829 [details] [diff] [review]
Stricter requirements for accessing typed object data

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

Great, this will be much safer.

::: js/src/builtin/SIMD.cpp
@@ +1322,5 @@
>      }
>  
>      TypedObjectElemArray<MaskTypeElem> mask(args[0]);
> +    TypedObjectElemArray<Elem> tv(args[1]);
> +    TypedObjectElemArray<Elem> fv(args[2]);

Not sure what's up with this part.  I can see the replacement already in the tree.
Attachment #8797829 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #5)
> Comment on attachment 8797829 [details] [diff] [review]
> Stricter requirements for accessing typed object data
> 
> Review of attachment 8797829 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, this will be much safer.
> 
> ::: js/src/builtin/SIMD.cpp
> @@ +1322,5 @@
> >      }
> >  
> >      TypedObjectElemArray<MaskTypeElem> mask(args[0]);
> > +    TypedObjectElemArray<Elem> tv(args[1]);
> > +    TypedObjectElemArray<Elem> fv(args[2]);
> 
> Not sure what's up with this part.  I can see the replacement already in the
> tree.

Oh, whoops. That's a bug in the earlier patch, fixed in this one. It causes a test failure. I'll move this hunk before landing. I had one other similar copy/paste bug, I'll double-check that I put it in the right patch too.
https://hg.mozilla.org/mozilla-central/rev/6afa90f3a06c
https://hg.mozilla.org/mozilla-central/rev/0ac4f99472a3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Group: core-security → core-security-release
Group: core-security-release
Keywords: sec-audit
Whiteboard: [adv-main52-]
You need to log in before you can comment on or make changes to this bug.