Re: [PATCH] media: vicodec: fix out-of-bounds write in FWHT encoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix leaving the stateless decoder broken for 4-component unencoded frames due to insufficient sizeimage multiplier.
- [High] Data corruption in `decode_plane()` when processing uncompressed planes for interleaved pixel formats.
--

commit 9d6dcf75887aba570509c5de466de5d05f1da49d
Author: Junrui Luo <moonafterrain@outlook.com>

media: vicodec: fix out-of-bounds write in FWHT encoder

vidioc_s_fmt_vid_out() sizes the encoder CAPTURE buffer from the compressed
descriptor pixfmt_fwht, whose sizeimage_mult is 3. This patch bumps
pixfmt_fwht.sizeimage_mult from 3 to 4, matching the largest components_num
among the supported raw formats, so the capture buffer is always large
enough for the unencoded fallback.

> diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
> index 318e8330f16ae..3ff2d06742c20 100644
> --- a/drivers/media/test-drivers/vicodec/vicodec-core.c
> +++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
> @@ -63,7 +63,7 @@ struct pixfmt_info {
>  };
>  
>  static const struct v4l2_fwht_pixfmt_info pixfmt_fwht = {
> -	V4L2_PIX_FMT_FWHT, 0, 3, 1, 1, 1, 1, 1, 0, 1
> +	V4L2_PIX_FMT_FWHT, 0, 4, 1, 1, 1, 1, 1, 0, 1
>  };
>  
>  static const struct v4l2_fwht_pixfmt_info pixfmt_stateless_fwht = {

[Severity: High]
This isn't a bug introduced by this patch, but does leaving
pixfmt_stateless_fwht.sizeimage_mult at 3 break the stateless decoder for
4-component unencoded frames?

Since pixfmt_stateless_fwht.sizeimage_mult remains 3, vidioc_try_fmt() will
cap the sizeimage for compressed OUTPUT queue buffers at 3 * width * height.
If user space attempts to queue a valid 4-component unencoded frame, which
requires 4 * width * height bytes, will videobuf2 reject the buffer because
the payload exceeds the negotiated sizeimage?

[Severity: High]
This is a pre-existing issue, but looking at decode_plane() in
drivers/media/test-drivers/vicodec/codec-fwht.c, is there a data corruption
issue when processing uncompressed planes for interleaved pixel formats?

When falling back to decoding an uncompressed plane, the code uses:

	if (uncompressed) {
		int i;

		if (end_of_rlco_buf + 1 < *rlco + width * height / 2)
			return false;
		for (i = 0; i < height; i++) {
			memcpy(dst, *rlco, width);
			dst += dst_stride;
			*rlco += width / 2;
		}
		return true;
	}

Since this unpacking ignores the dst_step stride, which spaces out bytes for
interleaved formats like V4L2_PIX_FMT_RGB32 (dst_step = 4), does this
clobber the bytes belonging to other color components for the first
width / dst_step pixels of the row, and leave the rest of the row unwritten?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/SYBPR01MB7881C44480F77338B6A93C61AF152@SYBPR01MB7881.ausprd01.prod.outlook.com?part=1