Closed Bug 1342732 Opened 7 years ago Closed 7 years ago

memory overruns reported for libyuv update

Categories

(Core :: Graphics, defect)

52 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Keywords: csectype-bounds, sec-moderate)

Attachments

(1 file)

Triggered by the landing of the libyuv update in bug 1341543.

This appears to be an upstream bug, apparently recently introduced in optimizations in September 2016 for "Add low level support for 12 bit 420, 422 and 444 YUV video frame conversion":
https://codereview.chromium.org/2371293002
It's very unclear if we'd ever use these functions, though perhaps it's possible.

It's unclear to me if the bug is the array being too small, or the offset in the ANY_SIMD call being too large.  Safe solution it to expand the temp array.

I'll report it upstream.


** CID 1401524:  Memory - illegal accesses  (OVERRUN)
/media/libyuv/libyuv/source/row_any.cc: 726 in HalfFloatRow_Any_AVX2()


________________________________________________________________________________________________________
*** CID 1401524:  Memory - illegal accesses  (OVERRUN)
/media/libyuv/libyuv/source/row_any.cc: 726 in HalfFloatRow_Any_AVX2()
720       }
721     
722     #ifdef HAS_HALFFLOATROW_SSE2
723     ANY11P16(HalfFloatRow_Any_SSE2, HalfFloatRow_SSE2, float, 1, 1, 7)
724     #endif
725     #ifdef HAS_HALFFLOATROW_AVX2
>>>     CID 1401524:  Memory - illegal accesses  (OVERRUN)
>>>     Overrunning array of 128 bytes at byte offset 128 by dereferencing pointer "&temp[64]".
726     ANY11P16(HalfFloatRow_Any_AVX2, HalfFloatRow_AVX2, float, 1, 1, 15)
727     #endif
728     #ifdef HAS_HALFFLOATROW_F16C
729     ANY11P16(HalfFloatRow_Any_F16C, HalfFloatRow_F16C, float, 1, 1, 15)
730     ANY11P16(HalfFloat1Row_Any_F16C, HalfFloat1Row_F16C, float, 1, 1, 15)
731     #endif

** CID 1401525:  Memory - illegal accesses  (OVERRUN)
/media/libyuv/libyuv/source/row_any.cc: 723 in HalfFloatRow_Any_SSE2()


________________________________________________________________________________________________________
*** CID 1401525:  Memory - illegal accesses  (OVERRUN)
/media/libyuv/libyuv/source/row_any.cc: 723 in HalfFloatRow_Any_SSE2()
717         memcpy(temp, src_ptr + n * SBPP, r * SBPP);                    \
718         ANY_SIMD(temp, temp + 64, shuffler, MASK + 1);                 \
719         memcpy(dst_ptr + n * BPP, temp + 64, r * BPP);                 \
720       }
721     
722     #ifdef HAS_HALFFLOATROW_SSE2
>>>     CID 1401525:  Memory - illegal accesses  (OVERRUN)
>>>     Overrunning array of 128 bytes at byte offset 128 by dereferencing pointer "&temp[64]".
723     ANY11P16(HalfFloatRow_Any_SSE2, HalfFloatRow_SSE2, float, 1, 1, 7)
724     #endif
725     #ifdef HAS_HALFFLOATROW_AVX2
726     ANY11P16(HalfFloatRow_Any_AVX2, HalfFloatRow_AVX2, float, 1, 1, 15)
727     #endif
728     #ifdef HAS_HALFFLOATROW_F16C
Attached patch Adjust ANY11P16Splinter Review
MozReview-Commit-ID: ARnj1K5o2LM
Attachment #8841299 - Flags: review?(sotaro.ikeda.g)
https://bugs.chromium.org/p/libyuv/issues/detail?id=686
There's no way for externals to submit hidden sec issues...
Comment on attachment 8841299 [details] [diff] [review]
Adjust ANY11P16

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

::: media/libyuv/libyuv/source/row_any.cc
@@ +706,5 @@
>  // Any 1 to 1 with parameter and shorts.  BPP measures in shorts.
>  #define ANY11P16(NAMEANY, ANY_SIMD, T, SBPP, BPP, MASK)            \
>    void NAMEANY(const uint16* src_ptr, uint16* dst_ptr, T shuffler, \
>                 int width) {                                        \
> +    SIMD_ALIGNED(uint16 temp[64 * 2]);                             \

Don't we need to use sizeof(uint16) here?
Attachment #8841299 - Flags: review?(sotaro.ikeda.g)
                                  \
> > +    SIMD_ALIGNED(uint16 temp[64 * 2]);                             \
> 
> Don't we need to use sizeof(uint16) here?

It's already uint16_t typed
Comment on attachment 8841299 [details] [diff] [review]
Adjust ANY11P16

re-requesting
Attachment #8841299 - Flags: review?(sotaro.ikeda.g)
Attachment #8841299 - Flags: review?(sotaro.ikeda.g) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/dabb9120d849fec9dd6946ee4c43d15d0b9679ae
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
This did get merged to m-c, though the bug didn't get marked for some reason.
https://hg.mozilla.org/mozilla-central/rev/dabb9120d849fec9dd6946ee4c43d15d0b9679ae
Target Milestone: --- → mozilla54
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.