linux-kernel - Re: [PATCH v3 1/2] media: venus: fix TOCTOU vulnerability when reading packets from shared memory

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]

Message-ID: <13887de6-4f84-9d0c-bd48-de6f0472d9ef@quicinc.com>
Date: Thu, 15 May 2025 23:55:57 +0530
From: Vikash Garodia <quic_vgarodia@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Dikshita Agarwal
	<quic_dikshita@...cinc.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Hans Verkuil
	<hans.verkuil@...co.com>
CC: <linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Vedang Nagar <quic_vnagar@...cinc.com>
Subject: Re: [PATCH v3 1/2] media: venus: fix TOCTOU vulnerability when
 reading packets from shared memory



On 5/15/2025 11:21 PM, Bryan O'Donoghue wrote:
> On 15/05/2025 14:23, Vikash Garodia wrote:
>>> Re-reading to see if the firmware wrote new bad data to the transmitted packet
>>> in-memory is not a fix before or after the memcpy() because the time you do that
>>> re-read is not fixed - locked wrt the freerunning firmware.
>> It would be more meaningful if you can suggest the vulnerability you see with
>> the changes suggested i.e adding the check in local packet against the size read
>> from shared queue. Based on that we can see how to fix it, otherwise this
>> discussion in not leading to any conclusion.
> 
> So to re-iterate.
> 
> TOCTOU is this
> 
> if (*ptr_val >> 2 >= MAX)
>     return -EBAD;
> 
> memcpy(dst, src, *ptr_val >> 2);
> 
> Here a malicious actor can change *ptr_val between our check and our use.
> 
> not
> 
> data_value = *ptr_val >> 2;
> 
> if (data_value >= MAX)
>     return -EBAD;
> 
> memcpy(dst, src, data_value);
> 
> Here the taking a copy of the value and subsequently relying on that value
> mitigates TOCTOU, because the value *ptr_val is latched - read into a local
> variable which cannot be manipulated from an outside agent i.e. venus firmware.
> 
> The example in the commit log is not a TOCTOU for that reason.
> 
> Adding an additional check _after_ the memcpy() seems silly to me because
> 
> data_value = *ptr_val >> 2;
> 
> if (data_value >= MAX)
>     return -EBAD;
> 
> memcpy(dst, src, data_value);
> 
> // This statement could be false
> if (data_value != *ptr_value >> 2)
>     return -EBAD;
> 
> // while this subsequent statement is true
> if (data_value != *ptr_value >> 2)
>     return -EBAD;
> 
Check the pseudo code which i proposed earlier in this conversation [1]. It does
not rely on ptr_val at all to check the sanity after memcpy.

[1] https://lore.kernel.org/all/0c50c24a-35fa-acfb-a807-b4ed5394506b@quicinc.com/
> And in any case this is a post-use sanity check not a mitigation for TOCTOU
> which we don't have.
> 
> ---
> bod