linux-kernel - Re: [PATCH] media: v4l2-mem2mem: fix mem order in last buf

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]

Date: Thu, 15 Feb 2024 11:16:02 +0800
From: Randy Li <ayaka@...lik.info>
To: Nicolas Dufresne <nicolas.dufresne@...labora.com>
Cc: mchehab@...nel.org, hverkuil-cisco@...all.nl,
 Hsia-Jun Li <randy.li@...aptics.com>, sebastian.fricke@...labora.com,
 alexious@....edu.cn, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: v4l2-mem2mem: fix mem order in last buf


On 2024/2/15 04:38, Nicolas Dufresne wrote:
> Hi,
>
>>   media: v4l2-mem2mem: fix mem order in last buf
> mem order ? Did you mean call order ?
std::memory_order
>
> Le dimanche 11 février 2024 à 02:04 +0800, Hsia-Jun Li a écrit :
>> From: "Hsia-Jun(Randy) Li" <randy.li@...aptics.com>
>>
>> The has_stopped property in struct v4l2_m2m_ctx is operated
>> without a lock protecction. Then the userspace calls to
>                   protection   When ?                   ~~
Access to those 3 booleans you mentioned later.
>> v4l2_m2m_encoder_cmd()/v4l2_m2m_decoder_cmd() may lead to
>> a critical section issue.
> As there is no locking, there is no critical section, perhaps a better phrasing
> could help.

"concurrent accesses to shared resources can lead to unexpected or 
erroneous behavior, so parts of the program where the shared resource is 
accessed need to be protected in ways that avoid the concurrent access."

It didn't say we need a lock here.

>> Signed-off-by: Hsia-Jun(Randy) Li <randy.li@...aptics.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-mem2mem.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> index 75517134a5e9..f1de71031e02 100644
>> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
>> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
>> @@ -635,9 +635,9 @@ void v4l2_m2m_last_buffer_done(struct v4l2_m2m_ctx *m2m_ctx,
>>   			       struct vb2_v4l2_buffer *vbuf)
>>   {
>>   	vbuf->flags |= V4L2_BUF_FLAG_LAST;
>> -	vb2_buffer_done(&vbuf->vb2_buf, VB2_BUF_STATE_DONE);
>> -
>>   	v4l2_m2m_mark_stopped(m2m_ctx);
>> +
>> +	vb2_buffer_done(&vbuf->vb2_buf, VB2_BUF_STATE_DONE);
> While it most likely fix the issue while testing, since userspace most likely
> polls on that queue and don't touch the driver until the poll was signalled, I
> strongly believe this is insufficient. When I look at vicodec and wave5, they
> both add a layer of locking on top of the mem2mem framework to fix this issue.

Maybe a memory barrier is enough. Since vb2_buffer_done() itself would 
invoke the (spin)lock operation.

When the poll() returns in userspace, the future operation for those 
three boolean variables won't happen before the lock.

> I think this is unfortunate, but v4l2_m2m_mark_stopped() is backed by 3 booleans
> accessed in many places that aren't in any known atomic context. I think it
> would be nice to remove the spurious locking in drivers and try and fix this
> issue in the framework itself.
I tend to not introduce more locks here. There is a spinlock in m2m_ctx 
which is a pain in the ass, something we could reuse it to save our CPU 
but it just can't be access.
>
> Nicolas
>
>>   }
>>   EXPORT_SYMBOL_GPL(v4l2_m2m_last_buffer_done);
>>