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

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

({sec-audit})

unspecified
mozilla52
sec-audit
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [adv-main52-])

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
Created attachment 8797307 [details] [diff] [review]
Avoid GC hazards when accessing typed object data

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)

Updated

2 years ago
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+
(Assignee)

Comment 3

2 years ago
(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.
(Assignee)

Comment 4

2 years ago
Created attachment 8797829 [details] [diff] [review]
Stricter requirements for accessing typed object data

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)
(Assignee)

Updated

2 years ago
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+
(Assignee)

Comment 6

2 years ago
(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
Last Resolved: 2 years ago
status-firefox52: --- → fixed
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.