linux-kernel - Re: [PATCH 2/2] media: qcom: camss: vfe: Fix registration sequencing bug

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: <21bc46d0-7e11-48d3-a09d-5e55e96ca122@linaro.org>
Date: Mon, 16 Jun 2025 15:09:52 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
 Robert Foss <rfoss@...nel.org>, Todor Tomov <todor.too@...il.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Hans Verkuil
 <hverkuil@...all.nl>, Depeng Shao <quic_depengs@...cinc.com>,
 Hans Verkuil <hans.verkuil@...co.com>
Cc: linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 linux-kernel@...r.kernel.org, stable@...r.kernel.org,
 Johan Hovold <johan+linaro@...nel.org>
Subject: Re: [PATCH 2/2] media: qcom: camss: vfe: Fix registration sequencing
 bug

On 13/06/2025 10:13, Vladimir Zapolskiy wrote:
> 
> Per se this concurrent execution shall not lead to the encountered bug,

What does that mean ? Please re-read the commit log, the analysis is all 
there.

> both an initialization of media entity pads by media_entity_pads_init()
> and a registration of a v4l2 devnode inside msm_video_register() are
> done under in a proper sequence, aren't they?

No, I clearly haven't explained this clearly enough in the commit log.

vfe0_rdi0 == /dev/video0 is complete. vfe0_rdi1 is not complete there is 
no /dev/video1 in user-space.

vfe_get() is called for an RDI in a VFE, camss_find_sensor_pad() assumes 
all RDIs are populated.

We can't use any VFE mutex to synchronise this because

lock(vfe->mutex);
lock(media->mutex);

and
lock(media->mutex);
lock(vfe->mutex);

happen.

So we can educate vfe_get() about the RDI it is operating on or we can 
flag that a VFE - all of it's subordinate RDIs are available.

I didn't much like teaching vfe_get() about which RDI index because the 
code looked ugly for 8916 you have to assume on one of the code paths 
that it always operates on RDI0, which is an invalid assumption.

The other way to fix this is:

+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -2988,7 +2988,7 @@ struct media_pad *camss_find_sensor_pad(struct 
media_entity *entity)

         while (1) {
                 pad = &entity->pads[0];
-               if (!(pad->flags & MEDIA_PAD_FL_SINK))
+               if (!pad || !(pad->flags & MEDIA_PAD_FL_SINK))

But then you see that every other driver treats pad = &entity->pads[0] 
as always non-NULL.

---
bod