media: meson: vdec: size capture planes from the aligned canvas
Subsystems (4)
PATCH 1/1 media: meson: vdec: size capture planes from the aligned canvas
The decoder writes a whole canvas of ALIGN(width, 32) x ALIGN(height, 32) (amvdec_set_canvases()) and reports bytesperline = ALIGN(width, 32), but get_output_size() derives sizeimage from the raw width x height.
When the width is not a multiple of 32 the planes are too small for the advertised stride: for 720x360 NV12M the luma plane is 262144 bytes while 736 * 360 = 264960 are needed (282624 for the canvas the firmware fills). The decoder writes past the buffer, and importing the capture dma-buf into DRM fails: drmModeAddFB2() returns -EINVAL, so e.g. Kodi plays the sound over a black screen. 1280x720 and 1920x1080 are not affected.
Size the planes from the aligned canvas the hardware actually uses.
Tested on an S905X (GXL p212) board with LibreELEC 12 (6.16.0-rc3):
720x360 and 1920x1080 H.264 play through V4L2 m2m + DRM PRIME with no AddFB2 errors.
--- drivers/staging/media/meson/vdec/vdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/media/meson/vdec/vdec.c +1 -1
@@ -34,7 +34,7 @@static u32 get_output_size (u32 width,u32 height) { -return ALIGN (width * height,SZ_64K ); +return ALIGN (ALIGN (width,32 ) *ALIGN (height,32 ),SZ_64K ); }u32 amvdec_get_output_size (struct amvdec_session *sess)
--
Quoted 11 lines · Michael Freidkin
The decoder writes a whole canvas of
ALIGN(width, 32)xALIGN(height, 32)(amvdec_set_canvases()) and reports bytesperline =ALIGN(width, 32), butget_output_size()derives sizeimage from the raw width x height.When the width is not a multiple of 32 the planes are too small for the advertised stride: for 720x360 NV12M the luma plane is 262144 bytes while 736 * 360 = 264960 are needed (282624 for the canvas the firmware fills). The decoder writes past the buffer, and importing the capture dma-buf into DRM fails:
drmModeAddFB2()returns-EINVAL, so e.g. Kodi plays the sound over a black screen. 1280x720 and 1920x1080 are not affected.
Quoted 8 lines
Size the planes from the aligned canvas the hardware actually uses.
Tested on an S905X (GXL p212) board with LibreELEC 12 (6.16.0-rc3):
720x360 and 1920x1080 H.264 play through V4L2 m2m + DRM PRIME with no AddFB2 errors.Fixes 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")Signed-off-by Michael Freidkin <[email protected]>
It would probably be more reliable to just ALIGN() the width and height at the start so we don't have to do it over and over again.
regards,
dan carpenter
Quoted 4 lines · Dan Carpenter
Reviewed-by Dan Carpenter <[email protected]>It would probably be more reliable to just
ALIGN()the width and height at the start so we don't have to do it over and over again.
Thanks for the review, Dan.
Agreed. I kept this one minimal so it backports cleanly with the Fixes tag. If the maintainers prefer, I can send a v2 that aligns width and height once in vdec_try_fmt_common() and derives bytesperline and the plane sizes from them, or do that as a follow-up cleanup on top.
Michael
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] media: meson: vdec: integer overflow in get_output_size() causes OOB DMA write --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] media: meson: vdec: integer overflow in get_output_size() causes OOB DMA write --
commit 28bcd6a9db6c6247ecace49156692588db867097
Author: Michael Freidkin <[email protected]>
media: meson: vdec: size capture planes from the aligned canvas
This commit aligns the width and height to 32 before calculating the output size in get_output_size(). This fixes an issue where the allocated capture planes were too small for the advertised stride when the width is not a multiple of 32.
drivers/staging/media/meson/vdec/vdec.c struct dummy_buf { · line 37
Quoted 10 lines
drivers/staging/media/meson/vdec/vdec.c +1
-1@@ -34,7 +34,7 @@ struct dummy_buf {static u32 get_output_size (u32 width,u32 height) { -return ALIGN (width * height,SZ_64K ); +return ALIGN (ALIGN (width,32 ) *ALIGN (height,32 ),SZ_64K );
[Severity: Critical]
Does this new alignment calculation introduce an integer overflow that results in a 0-byte DMA buffer?
A crafted VP9 bitstream can provide untrusted dimensions up to 65535x65535. The firmware parses these into param->p.width and param->p.height in drivers/staging/media/meson/vdec/codec_vp9.c:codec_vp9_process_rpm(), which passes them directly to amvdec_src_change(). The dimensions are assigned to sess->width and sess->height without clamping.
If a user handles the resulting source change event by calling VIDIOC_REQBUFS directly, skipping VIDIOC_S_FMT which would normally clamp the dimensions, get_output_size() is called with 65535x65535.
When these dimensions are aligned to 32 here, ALIGN(65535, 32) * ALIGN(65535, 32) evaluates to 65536 * 65536, or 4,294,967,296. Because get_output_size() uses u32, this multiplication overflows and wraps exactly to 0.
vdec_queue_setup() then fetches this overflowed output size (0) to initialize the DMA plane sizes, bypassing size validations and requesting a 0-byte DMA buffer. The hardware would then attempt to decode the frame into this tiny buffer, writing gigabytes of data out of bounds.
Prior to this patch, width * height for 65535x65535 was 4,294,836,225, which did not overflow u32 and correctly caused a safe allocation failure.
Quoted 3 lines
drivers/staging/media/meson/vdec/vdec.c +0
-0}u32 amvdec_get_output_size (struct amvdec_session *sess)
Signature · 6 lines
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1 _______________________________________________ linux-amlogic mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-amlogic