linux-kernel - Re: [PATCH v7 00/11] VP9 codec V4L2 control interface

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: <ccc29c1f-e2a1-f7c9-77ad-6e9fc1a57c95@collabora.com>
Date:   Wed, 17 Nov 2021 11:51:09 +0100
From:   Andrzej Pietrasiewicz <andrzej.p@...labora.com>
To:     Hans Verkuil <hverkuil-cisco@...all.nl>,
        linux-media@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        linux-staging@...ts.linux.dev
Cc:     Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        Boris Brezillon <boris.brezillon@...labora.com>,
        Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        Fabio Estevam <festevam@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Nicolas Dufresne <nicolas.dufresne@...labora.com>,
        NXP Linux Team <linux-imx@....com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Shawn Guo <shawnguo@...nel.org>, kernel@...labora.com
Subject: Re: [PATCH v7 00/11] VP9 codec V4L2 control interface

Hi again,

W dniu 17.11.2021 o 11:49, Andrzej Pietrasiewicz pisze:
> Hi,
> 
> W dniu 17.11.2021 o 10:59, Hans Verkuil pisze:
>> On 16/11/2021 14:14, Andrzej Pietrasiewicz wrote:
>>> Hi,
>>>
>>> W dniu 16.11.2021 o 09:21, Hans Verkuil pisze:
>>>> On 16/11/2021 09:09, Andrzej Pietrasiewicz wrote:
>>>>> Hi Hans,
>>>>>
>>>>> W dniu 15.11.2021 o 22:16, Hans Verkuil pisze:
>>>>>> On 15/11/2021 18:14, Andrzej Pietrasiewicz wrote:
>>>>>>> Hi Hans,
>>>>>>>
>>>>>>> W dniu 15.11.2021 o 16:07, Hans Verkuil pisze:
>>>>>>>> Andrzej,
>>>>>>>>
>>>>>>>> Can you rebase this series on top of the master branch of
>>>>>>>> https://git.linuxtv.org/media_stage.git/ ? Unfortunately this v7 no longer
>>>>>>>> applies. Specifically "rkvdec: Add the VP9 backend" failed in a non-trivial
>>>>>>>> manner.
>>>>>>>
>>>>>>> This is a branch for you:
>>>>>>>
>>>>>>> https://gitlab.collabora.com/linux/for-upstream/-/tree/vp9-uapi
>>>>>>
>>>>>> I'm getting a bunch of sparse/smatch warnings:
>>>>>>
>>>>>
>>>>> Thanks for finding this, I will re-create the branch and let you know on irc.
>>>>> Some of the below are "false positives, namely:
>>>>>
>>>>> drivers/media/platform/omap3isp/omap3isp.h
>>>>> drivers/media/platform/qcom/venus/core.h
>>>>
>>>> Ah, sorry, I though I had filtered those out. Obviously you can ignore those.
>>>>
>>>> Please post a v8. That way the series is archived on lore. And it works better
>>>> with patchwork.
>>>
>>> Sure, no problem. Also please see below.
>>>
>>>>
>>>> Regards,
>>>>
>>>>     Hans
>>>>
>>>>>
>>>>> which are not touched by the series.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Andrzej
>>>>>
>>>>>> sparse:
>>>>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not 
>>>>>> used [-Wunused-but-set-variable]
>>>>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not 
>>>>>> used [-Wunused-but-set-variable]
>>>>>> SPARSE:hantro/hantro_postproc.c hantro/hantro_postproc.c:37:35: warning: 
>>>>>> symbol 'hantro_g1_postproc_regs' was not declared. Should it be static?
>>>>>>
>>>>>> smatch:
>>>>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not 
>>>>>> used [-Wunused-but-set-variable]
>>>>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not 
>>>>>> used [-Wunused-but-set-variable]
>>>>>> rkvdec/rkvdec-vp9.c: rkvdec/rkvdec-vp9.c:236 init_intra_only_probs() 
>>>>>> error: buffer overflow 'ptr' 90 <= 91
>>>
>>> this looks a false positive.
>>>
>>> A portion of memory pointed to by ptr is indexed with i * 23 + m,
>>> where i ranges from 0 to 3, inclusive, and m ranges from 0 to 22,
>>> inclusive if i < 3, otherwise m ranges from 0 to 20, inclusive.
>>> So the largest index value we compute equals 89 (3 * 23 + 20).
>>> Because ptr points to something that is at least 90 bytes large,
>>> 89 is a valid index and no greater index will be ever computed.
>>
>> But we do need to get rid of this smatch warning, otherwise it will pollute the
>> list of smatch warnings.
>>
>> I was looking at the code and wonder if it wouldn't make more sense to
>> move writing to rkprobs->intra_mode[i].uv_mode[] into a separate for loop:
>>
>>          for (i = 0; i < ARRAY_SIZE(v4l2_vp9_kf_uv_mode_prob); i++)
>>                  rkprobs->intra_mode[i / 23].uv_mode[i % 23] = 
>> v4l2_vp9_kf_uv_mode_prob[i];
>>
>> Wouldn't that do the same as the current code? It looks simpler as well.
>>
> 
> I think it would, but I would slightly change the loop:
>
>      for (i = 0; i < ARRAY_SIZE(v4l2_vp9_kf_uv_mode_prob); i++) {

actually, sizeof(v4l2_vp9_kf_uv_mode_prob)



>          const u8 *ptr = (const u8 *)v4l2_vp9_kf_uv_mode_prob;
> 
>          rkprobs->intra_mode[i / 23].uv_mode[i % 23] = ptr[i];
>      }
> 
> because v4l2_vp9_kf_uv_mode_prob is actually a u8[10][9].
> 
> I will make such a change locally and test whether it causes regressions.
> 
> Once I confirm it works (and I expect I will) would you like me to post a v9,
> only reply to the changed patch with its updated version or do you want to make 
> this change yourself?
> 
> Andrzej