linux-kernel - RE: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <000001db8838$bc466e00$34d34a00$@samsung.com>
Date: Wed, 26 Feb 2025 15:55:16 +0530
From: "Aakarsh Jain/Aakarsh Jain" <aakarsh.jain@...sung.com>
To: "'Hans Verkuil'" <hverkuil-cisco@...all.nl>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-media@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Cc: <m.szyprowski@...sung.com>, <andrzej.hajda@...el.com>,
	<mchehab@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
	<linux-samsung-soc@...r.kernel.org>, <gost.dev@...sung.com>,
	<aswani.reddy@...sung.com>, <pankaj.dubey@...sung.com>
Subject: RE: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

Hi Hans,

-----Original Message-----
From: Hans Verkuil <hverkuil-cisco@...all.nl> 
Sent: 11 October 2024 18:17
To: Aakarsh Jain <aakarsh.jain@...sung.com>; linux-arm-kernel@...ts.infradead.org; linux-media@...r.kernel.org; linux-kernel@...r.kernel.org
Cc: m.szyprowski@...sung.com; andrzej.hajda@...el.com; mchehab@...nel.org; krzysztof.kozlowski+dt@...aro.org; linux-samsung-soc@...r.kernel.org; gost.dev@...sung.com; aswani.reddy@...sung.com; pankaj.dubey@...sung.com
Subject: Re: [PATCH] media: s5p-mfc: Corrected NV12M/NV21M plane-sizes

On 06/08/2024 13:57, Aakarsh Jain wrote:
> There is a possibility of getting page fault if the overall buffer 
> size is not aligned to 256bytes. Since MFC does read operation only 
> and it won't corrupt the data values even if it reads the extra bytes.
> Corrected luma and chroma plane sizes for V4L2_PIX_FMT_NV12M and 
> V4L2_PIX_FMT_NV21M pixel format.
> 
> Signed-off-by: Aakarsh Jain <aakarsh.jain@...sung.com>
> ---
>  .../media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c    | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c 
> b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> index 73f7af674c01..03c957221fc4 100644
> --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_opr_v6.c
> @@ -498,8 +498,8 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct s5p_mfc_ctx *ctx)
>  	case V4L2_PIX_FMT_NV21M:
>  		ctx->stride[0] = ALIGN(ctx->img_width, S5P_FIMV_NV12MT_HALIGN_V6);
>  		ctx->stride[1] = ALIGN(ctx->img_width, S5P_FIMV_NV12MT_HALIGN_V6);
> -		ctx->luma_size = calc_plane(ctx->stride[0], ctx->img_height);
> -		ctx->chroma_size = calc_plane(ctx->stride[1], (ctx->img_height / 2));
> +		ctx->luma_size = calc_plane(ctx->img_width, ctx->img_height);
> +		ctx->chroma_size = calc_plane(ctx->img_width, (ctx->img_height >> 
> +1));

I don't really understand why this is changed. Looking at the implementation of calc_plane and the various #define values that are used here and in calc_plane, the number should be the same.

I think the original code makes more sense.
okay.

If I missed something, let me know.

>  		break;
>  	case V4L2_PIX_FMT_YUV420M:
>  	case V4L2_PIX_FMT_YVU420M:
> @@ -539,9 +539,11 @@ static void s5p_mfc_dec_calc_dpb_size_v6(struct 
> s5p_mfc_ctx *ctx)  static void s5p_mfc_enc_calc_src_size_v6(struct 
> s5p_mfc_ctx *ctx)  {
>  	unsigned int mb_width, mb_height;
> +	unsigned int default_size;
>  
>  	mb_width = MB_WIDTH(ctx->img_width);
>  	mb_height = MB_HEIGHT(ctx->img_height);
> +	default_size = (mb_width * mb_height) * 256;
>  
>  	if (IS_MFCV12(ctx->dev)) {
>  		switch (ctx->src_fmt->fourcc) {
> @@ -549,8 +551,8 @@ static void s5p_mfc_enc_calc_src_size_v6(struct s5p_mfc_ctx *ctx)
>  		case V4L2_PIX_FMT_NV21M:
>  			ctx->stride[0] = ALIGN(ctx->img_width, S5P_FIMV_NV12M_HALIGN_V6);
>  			ctx->stride[1] = ALIGN(ctx->img_width, S5P_FIMV_NV12M_HALIGN_V6);
> -			ctx->luma_size = ctx->stride[0] * ALIGN(ctx->img_height, 16);
> -			ctx->chroma_size =  ctx->stride[0] * ALIGN(ctx->img_height / 2, 16);
> +			ctx->luma_size = ALIGN(default_size, 256);
> +			ctx->chroma_size = ALIGN(default_size / 2, 256);

Isn't this effectively the same as doing:

			ctx->luma_size = ALIGN(ctx->luma_size, 256);
			ctx->chroma_size = ALIGN(ctx->chroma_size, 256);

I.e., the bug is that these sizes are not rounded up to a multiple of 256, so just add that, rather than changing code elsewhere.

I might be wrong, but this seems a much simpler solution.
sure Hans. Will address in v2.

Thanks for review.

Regards,

	Hans

>  			break;
>  		case V4L2_PIX_FMT_YUV420M:
>  		case V4L2_PIX_FMT_YVU420M: