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 for Android: free password hash cracker in your pocket

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

Date: Fri, 21 Jun 2024 01:38:30 +0800
From: Jianfeng Liu <liujianfeng1994@...il.com>
To: nicolas.dufresne@...labora.com
Cc: andy.yan@...k-chips.com,
	benjamin.gaignard@...labora.com,
	boris.brezillon@...labora.com,
	conor+dt@...nel.org,
	daniel.almeida@...labora.com,
	detlev.casanova@...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,
	liujianfeng1994@...il.com,
	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 Detlev,

On Thu, 20 Jun 2024 10:07:41 -0400, Detlev Casanova wrote:

>This feels like hacking the driver to please a specific userspace application, 
>so I'd like to understand better what chromium is doing.

Yes that hack is ugly. I have added log print in chromium to see if they
have set frame_mbs_only_flag to zero and found nothing. This sps->flags is
initialized 0 by kernel's v4l2 code. I printed all sps values in function
rkvdec2_h264_validate_sps and they are all initial values when chromiumn
call VIDIOC_STREAMON at the first time.

Hi Nicolas,

On Thu, 20 Jun 2024 11:03:54 -0400, Nicolas Dufresne wrote:

>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.

Yeah that's the root cause. Vaule of sps->flags is initialized to 0 along
with pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1, so this
check would fall with minimal decoder size 16x16.

>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

This patch makes sense and I just confirmed that it works with chromium.
Thank you very much!

Best regards,
Jianfeng