memory overruns reported for libyuv update

RESOLVED FIXED in Firefox 54

Status

()

Core
Graphics
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

({csectype-bounds, sec-moderate})

52 Branch
mozilla54
csectype-bounds, sec-moderate
Points:
---

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox51 unaffected, firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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
(Assignee)

Comment 1

a year ago
Created attachment 8841299 [details] [diff] [review]
Adjust ANY11P16

MozReview-Commit-ID: ARnj1K5o2LM
Attachment #8841299 - Flags: review?(sotaro.ikeda.g)
(Assignee)

Comment 2

a year ago
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)
(Assignee)

Comment 4

a year ago
                                  \
> > +    SIMD_ALIGNED(uint16 temp[64 * 2]);                             \
> 
> Don't we need to use sizeof(uint16) here?

It's already uint16_t typed
(Assignee)

Comment 5

a year ago
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+
(Assignee)

Comment 6

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dabb9120d849fec9dd6946ee4c43d15d0b9679ae
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Blocks: 1341543
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox54: --- → fixed
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → unaffected
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.