Opened 9 years ago

Last modified 15 months 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 (6)

comment:1 by Carl Eugen Hoyos, 9 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, 9 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, 19 months 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 19 months ago by Niklas Haas (previous) (diff)

comment:5 by Niklas Haas, 19 months 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.

comment:6 by hackerfactor, 15 months ago

I just ran into this same problem.

sws_scale(swsctx, FrameIn->data,FrameIn->linesize, 0, FrameIn->height, FrameOut->data, FrameOut->linesize);

I included some debugging print lines:
Frame In: 540x360 linesize=640
Frame Out: 540x360 linesize=1620

The video is mp4, YUV444 and being converted to RGB24. It reports to stderr:
[swscaler @ 0x16c7300] Warning: data is not aligned! This can lead to a speed loss

The output buffer is allocated as 540x360x3 = 583200 bytes.
However, the misalignment causes sws_scale to write-overflow. (Valgrind detected it.) I see the same problem when using vlc and other video players on the same video since they use the same ffmpeg library.

My workaround: Allocate enough space to round up to a width that is divisible by 8. (Allocate 544x360x3 = 587520 bytes.)

Last edited 15 months ago by hackerfactor (previous) (diff)
Note: See TracTickets for help on using tickets.