Opened 7 years ago

Last modified 9 days ago

#6763 new defect

swscale: Out-of-bounds memory accesses

Reported by: Gramner Owned by:
Priority: important Component: swscale
Version: git-master Keywords: crash
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

Many assembly functions in swscale will read past the end of their input buffers which causes segfaults and/or bus errors if the buffer happens to be located near the end of a memory page and the next page is invalid.

Aligning input buffers isn't even enough for formats like RGB24 (and requiring alignment would be a bad idea anyway since it wouldn't work with memory-mapped input files for example).

Using swscale with x264 CLI seems to be a fairly consistent way to trigger such out-of-bounds crashes. This command line for example will cause segfaults in ff_rgb24ToY_avx():

./x264 -o /dev/null --input-csp rgb --input-res 512x512 <any_input_file>

If asm is disabled in swscale the problem goes away.

Change History (5)

comment:1 by Carl Eugen Hoyos, 7 years ago

Keywords: crash added
Priority: normalimportant

(Version information, backtrace, disassembly and register dump missing.)
Could you confirm that your input buffers are sufficiently padded?

comment:2 by Gramner, 7 years ago

Version: unspecifiedgit-master

Not sure which disassembly you're interested in, but the source shows the entire row loop done using 128-bit loads with no special handling for the tail: http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libswscale/x86/input.asm;h=af9afcaa53a74f51eaa1257bafd1052524046074;hb=HEAD#l243

With width=512 as an example, on the last loop iteration "movu m4, [srcq+12]" reads 16 bytes from offset 1524 which results in an overflow of 4 bytes.

The input buffer is the exact size of the input data with zero padding as no such requirement is documented.

comment:4 by Niklas Haas, 11 days ago

What do we think the ideal resolution is here?

  1. Simply document this requirement.
  2. Modify all assembly routines to handle the tail separately.
  3. Always memcpy the last row before calling into asm.
  4. Always call the C routine for the last row, instead of asm

I am leaning towards 3, with an option for 1.

Edit: added fourth option

Last edited 11 days ago by Niklas Haas (previous) (diff)

comment:5 by Niklas Haas, 9 days ago

It's worth pointing out that these alignment requirements are actually explicitly mentioned, albeit in the documentation for AVFrame:

https://ffmpeg.org/doxygen/2.7/frame_8h_source.html#l00191

Admittedly, this is a very non-obvious place, and also doesn't extend to sws_scale().

After investigating the situation more closely, I have come to notice that practically none of the asm routines are designed to handle images whose stride is not a multiple of at least 8, and I am leaning towards disabling the offending ASM routines entirely as an immediate fix.

A full fix (with a split C wrapper to handle the edges) would be the preferred solution long-term, but this will require a more substantial rewrite of the scaling core - something that is on my to-do list for 2025.

Note: See TracTickets for help on using tickets.