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@quicinc.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;
--
2.34.1On 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@quicinc.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 ?
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 ?
---
bod
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@quicinc.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
>
© 2016 - 2025 Red Hat, Inc.