linux-kernel - Re: [PATCH v2 2/4] media: rockchip: Introduce the rkvdec2 driver

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2349746d488f4edf9c7c40df5e15ff70d3ec67b7.camel@collabora.com>
Date: Thu, 20 Jun 2024 11:03:54 -0400
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Jianfeng Liu <liujianfeng1994@...il.com>, detlev.casanova@...labora.com
Cc: andy.yan@...k-chips.com, benjamin.gaignard@...labora.com, 
 boris.brezillon@...labora.com, conor+dt@...nel.org,
 daniel.almeida@...labora.com,  devicetree@...r.kernel.org,
 didi.debian@...ow.org, dsimic@...jaro.org,  ezequiel@...guardiasur.com.ar,
 gregkh@...uxfoundation.org, heiko@...ech.de,  hverkuil-cisco@...all.nl,
 jonas@...boo.se, knaerzche@...il.com, krzk+dt@...nel.org, 
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
 linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org, 
 linux-staging@...ts.linux.dev, mchehab@...nel.org,
 paul.kocialkowski@...tlin.com,  robh@...nel.org,
 sebastian.reichel@...labora.com
Subject: Re: [PATCH v2 2/4] media: rockchip: Introduce the rkvdec2 driver

Hi Jianfeng,

Le jeudi 20 juin 2024 à 01:46 +0800, Jianfeng Liu a écrit :
> Hi Detlev,
> 
> On Wed, 19 Jun 2024 10:57:19 -0400, Detlev Casanova wrote:
> > +	if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY))
> > +		height *= 2;
> > +
> > +	if (width > ctx->coded_fmt.fmt.pix_mp.width ||
> > +	    height > ctx->coded_fmt.fmt.pix_mp.height)
> > +		return -EINVAL;
> 
> I did further invesatigation on chromium. I find that before real video
> is decoded, chromium will call VIDIOC_STREAMON twice with value of
> sps->flags 0:
> 
> At the first time width and height are 16; ctx->coded_fmt.fmt.pix_mp.width
> and coded_fmt.fmt.pix_mp.height are 16, which are the min size of decoder;

This falls short of a specification to support the uninitialized usage by
Chromium. That being said, we do make an effort to try and have a valid default
SPS control and OUTPUT format combination. So my suggestion would be to set
V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY in the default compound control init. This
way, 0x0 get translate to 16x16 instead of 16x32, thus will work with more
drivers.

Chromium these days is not being tested on anything else then MTK, which has a
64x64 minimum size, this is why they never get into this issue. This driver
validation is entirely correct. Removing in some cases is unsafe, it would need
to be replaced with STREAMON only validation of the CAPTURE sizes (which
currently is validate by implied propagation of valid SPS/OUTPUT).

**not even compiled tested, just to illustrate**

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-
core/v4l2-ctrls-core.c
index c4d995f32191..a55e165ea9c3 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
@@ -111,6 +111,7 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl,
u32 idx,
        struct v4l2_ctrl_vp9_frame *p_vp9_frame;
        struct v4l2_ctrl_fwht_params *p_fwht_params;
        struct v4l2_ctrl_h264_scaling_matrix *p_h264_scaling_matrix;
+       struct v4l2_ctrl_h264_sps *p_h264_sps;
        struct v4l2_ctrl_av1_sequence *p_av1_sequence;
        void *p = ptr.p + idx * ctrl->elem_size;
 
@@ -179,6 +180,18 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl,
u32 idx,
                 */
                memset(p_h264_scaling_matrix, 16,
sizeof(*p_h264_scaling_matrix));
                break;
+       case V4L2_CTRL_TYPE_H264_SPS:
+               p_h264_sps = p;
+               /*
+                * Without V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY,
+                * frame_mbs_only_flag set to 0 will translate to a miniumum
+                * height of 32 (see H.264 specification 7-8). Some driver may
+                * have a minimum size lower then 32, which would fail
+                * validation with the SPS value. Set this flag, so that there
+                * is now doubling in the height, allowing a valid default.
+                */
+               p_h264_sps->flags = V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY;
+               break;
        }
 }

Nicolas

> At the second time width and height are still 16; while
> coded_fmt.fmt.pix_mp.width is 1920 and coded_fmt.fmt.pix_mp.height is
> 1088, which are the real size of video.
> 
> So VIDIOC_STREAMON will fall at the first time call because sps->flags is
> 0 so V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY is not set, and then height is
> doubled to 32 which is larger than 16.
> 
> What do you think if we skip doubling height if sps->flags is 0 and at the
> same time V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY is not set? The following hack
> did fix my chromium:
> 
> --- a/drivers/staging/media/rkvdec2/rkvdec2-h264.c
> +++ b/drivers/staging/media/rkvdec2/rkvdec2-h264.c
> @@ -767,7 +767,7 @@ static int rkvdec2_h264_validate_sps(struct rkvdec2_ctx *ctx,
>          * which is half the final height (see (7-18) in the
>          * specification)
>          */
> -       if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY))
> +       if (!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY) && sps->flags)
>                 height *= 2;
>  
>         if (width > ctx->coded_fmt.fmt.pix_mp.width ||
> 
> Best regards
> Jianfeng