[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <7e6fc439-b948-6777-0a70-a8ea58b1f7bd@quicinc.com>
Date: Tue, 13 May 2025 18:43:53 +0530
From: Dikshita Agarwal <quic_dikshita@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Vedang Nagar
<quic_vnagar@...cinc.com>,
Stanimir Varbanov <stanimir.k.varbanov@...il.com>,
Vikash Garodia <quic_vgarodia@...cinc.com>,
Mauro Carvalho Chehab
<mchehab@...nel.org>
CC: <linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] media: venus: fix OOB read issue due to double
read
On 3/4/2025 7:15 PM, Bryan O'Donoghue wrote:
> On 15/02/2025 17:19, Vedang Nagar wrote:
>> During message queue read, the address is being read twice
>> from the shared memory. The first read is validated against
>> the size of the packet, however the second read is not
>> being validated. Therefore, it's possible for firmware to
>> modify the value to a bigger invalid value which can lead
>> to OOB read access issue while reading the packet.
>> Added fix to reupdate the size of the packet which was
>> read for the first time.
>>
>> Signed-off-by: Vedang Nagar <quic_vnagar@...cinc.com>
>> ---
>> drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c
>> b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index
>> f9437b6412b91c2483670a2b11f4fd43f3206404..c124db8ac79d18f32289a690ee82145dc93daee6 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -298,6 +298,7 @@ static int venus_read_queue(struct venus_hfi_device
>> *hdev,
>> memcpy(pkt, rd_ptr, len);
>> memcpy(pkt + len, queue->qmem.kva, new_rd_idx << 2);
>> }
>> + *(u32 *)pkt = dwords << 2;
>> } else {
>> /* bad packet received, dropping */
>> new_rd_idx = qhdr->write_idx;
>>
>
> This is confusing - where is the read
>
> Your previous code
>
> https://lore.kernel.org/lkml/20250104-venus-security-fixes-v1-1-9d0dd4594cb4@quicinc.com/
>
> memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));
>
> V1 then would have been:
>
> if (new_rd_idx < qsize) {
> memcpy(pkt, rd_ptr, dwords << 2);
> } else {
> size_t len;
>
> new_rd_idx -= qsize;
> len = (dwords - new_rd_idx) << 2;
> memcpy(pkt, rd_ptr, len);
> memcpy(pkt + len, queue->qmem.kva, new_rd_idx << 2);
> }
>
> + memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));
>
> V2 proposed:
>
> if (new_rd_idx < qsize) {
> memcpy(pkt, rd_ptr, dwords << 2);
> } else {
> size_t len;
>
> new_rd_idx -= qsize;
> len = (dwords - new_rd_idx) << 2;
> memcpy(pkt, rd_ptr, len);
> memcpy(pkt + len, queue->qmem.kva, new_rd_idx << 2);
> }
>
> + *(u32 *)pkt = dwords << 2;
>
> My comment wasn't about use of memcpy() it was about why we are doing this.
>
> For example if new_rd_idx < qsize is true then we literally do
>
> a) memcpy(pkt, rd_ptr, dwords << 2);
> b) *(u32 *)pkt = dwords << 2;
>
> and the question is why ? That is an unambiguous cast of pkt to the value
> of dwords << 2;
>
> What is the scope of how the data can change from a to b ?
>
> And why is the data considered potentially invalid @ the memcpy() but valid
> subsequent the cast ?
>
The concern here is not about the value of dword which will be fixed once
we read the *rd_ptr first time and validate it.
The concern here is, with the data contents at rd_ptr which could be
tempered with after validation, because the memory is shared by firmware
So the real problem here is:
- driver read *rd_ptr, extract dwords = *rd_ptr >> 2, and validate it, lets
say this value is 10
- then copy dwords << 2 bytes from rd_ptr to a kernel buffer (pkt).
- But: the first 4 bytes at rd_ptr (which represent the size field again)
could have changed in the meantime — now saying 40 instead of the original 10.
security concern here is:
Even though the outer dwords is correct, the copied content might claim to
be bigger than what was validated — potentially tricking the rest of the
system into processing garbage or running past buffer bounds.
This is not a TOCTOU on the dwords, but a TOCTOU on the contents at rd_ptr,
where the firmware changes the actual data after size is checked, but
before it's copied.
> Assuming rd_ptr contains the length of dwords << 2 to begin with in the
> first 4 bytes - why is it necessary to make _really_ _really_ sure by
> restuffing the data ?
>
> For example if *(u32 *)rd_ptr != dwords << 2 - why shouldn't we just throw
> the whole frame away as containing junk data ?
Agree with returning error during mismatch instead of forcefully
overwriting the content.
Thank,
Dikshita
>
> ---
> bod
>
Powered by blists - more mailing lists