linux-kernel - Re: [PATCH v2] media: qcom: camss: fix VFE pm domain off

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: <a08e95fc03fce6cb0809a06900982c6c@mainlining.org>
Date: Fri, 29 Nov 2024 12:44:51 +0100
From: barnabas.czeman@...nlining.org
To: Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>
Cc: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, Robert Foss
 <rfoss@...nel.org>, Todor Tomov <todor.too@...il.com>, Mauro Carvalho Chehab
 <mchehab@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, Hans Verkuil
 <hverkuil@...all.nl>, linux-media@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, Yassine Oudjana
 <y.oudjana@...tonmail.com>
Subject: Re: [PATCH v2] media: qcom: camss: fix VFE pm domain off

On 2024-11-29 12:20, Vladimir Zapolskiy wrote:
> On 11/29/24 13:06, Bryan O'Donoghue wrote:
>> On 29/11/2024 08:48, Vladimir Zapolskiy wrote:
>>> On 11/28/24 21:39, Barnabás Czémán wrote:
>>>> Fix NULL pointer check before device_link_del
>>>> is called.
>>>> 
>>>> Unable to handle kernel NULL pointer dereference at virtual address
>>>> 000000000000032c
>>>> Call trace:
>>>>    device_link_put_kref+0xc/0xb8
>>>>    device_link_del+0x30/0x48
>>>>    vfe_pm_domain_off+0x24/0x38 [qcom_camss]
>>>>    vfe_put+0x9c/0xd0 [qcom_camss]
>>>>    vfe_set_power+0x48/0x58 [qcom_camss]
>>>>    pipeline_pm_power_one+0x154/0x158 [videodev]
>>>>    pipeline_pm_power+0x74/0xfc [videodev]
>>>>    v4l2_pipeline_pm_use+0x54/0x90 [videodev]
>>>>    v4l2_pipeline_pm_put+0x14/0x34 [videodev]
>>>>    video_release+0x2c/0x44 [qcom_camss]
>>>>    v4l2_release+0xe4/0xec [videodev]
>>>> 
>>>> Fixes: eb73facec2c2 ("media: qcom: camss: Use common VFE 
>>>> pm_domain_on/
>>>> pm_domain_off where applicable")
>>>> Tested-by: Yassine Oudjana <y.oudjana@...tonmail.com>
>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
>>>> Signed-off-by: Barnabás Czémán <barnabas.czeman@...nlining.org>
>>>> ---
>>>> Changes in v2:
>>>> - Add backtrace to the commit message.
>>>> - Link to v1: https://lore.kernel.org/r/20241122-vfe_pm_domain_off-
>>>> v1-1-81d18f56563d@...nlining.org
>>>> ---
>>>>    drivers/media/platform/qcom/camss/camss-vfe.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c 
>>>> b/drivers/
>>>> media/platform/qcom/camss/camss-vfe.c
>>>> index
>>>> 80a62ba11295042802cbaec617fb87c492ea6a55..1bf1473331f63b9ab106d21ea263c84d851c8a31 
>>>> 100644
>>>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>>>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
>>>> @@ -595,7 +595,7 @@ void vfe_isr_reset_ack(struct vfe_device *vfe)
>>>>     */
>>>>    void vfe_pm_domain_off(struct vfe_device *vfe)
>>>>    {
>>>> -    if (!vfe->genpd)
>>>> +    if (!vfe->genpd_link)
>>>>            return;
>>>>        device_link_del(vfe->genpd_link);
>>>> 
>>> 
>>> I object to this change, there might be a problem in the code, 
>>> however it
>>> is not yet identified.
>>> 
>>> vfe->genpd is not NULL, if vfe_pm_domain_on()/vfe_pm_domain_off() are
>>> called appropriately, the "fix" does not fix the real problem, it 
>>> veils it.
>>> 
>>> -- Best wishes,
>>> Vladimir
>>> 
>>> 
>> 
>> Let's walk through the logic.
>> 
>> vfe->genpd =
>> 
>> Can happen in vfe_subdev_init();
>> 
>> vfe_pm_domain_on() can fail @ vfe->genpd_link =
>> 
>> If it fails then I _suppose_ we are still calling vfe_pm_domain_off() 
>> at
>> least that's the only logically way I see this error can manifest.
> 
> There should be no room for suppositions, the source code is open.
> 
> If the described by you case is true, and vfe_pm_domain_on() fails,
> then vfe_pm_domain_off() shall not be called, otherwise that's the
> real problem and it shall be fixed instead of being veiled by the
> proposed change.
> 
>> @Barnabás can you confirm that this is the case ?
>> 
>> If not, can you please provide more detail ?
> 
> The change does not describe how to reproduce the problem, which commit
> base is tested, which platform is testes, there is no enough 
> information,
> unfortunately.
I can reproduce the problem with megapixels-sensorprofile on msm8953 and
it can be reproduced with megapixels on msm8996.
The base is the last commit on next.
> 
> --
> Best wishes,
> Vladimir