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 , 7 years ago
Keywords: | crash added |
---|---|
Priority: | normal → important |
comment:2 by , 7 years ago
Version: | unspecified → git-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:3 by , 16 months ago
comment:4 by , 11 days ago
What do we think the ideal resolution is here?
- Simply document this requirement.
- Modify all assembly routines to handle the tail separately.
- Always memcpy the last row before calling into asm.
- 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
comment:5 by , 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.
(Version information, backtrace, disassembly and register dump missing.)
Could you confirm that your input buffers are sufficiently padded?