| Message ID | 20250104-venus-security-fixes-v1-1-9d0dd4594cb4@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | venus driver fixes to avoid possible OOB read access | expand |
On 04/01/2025 05:41, 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. A brief scan of this code doesn't really show the base case you assert here. Could you be a bit more explicit. 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..64cc9e916f53e5a9c82b1ff25c4475d622ebc321 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); > } > + memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32)); I'm not entirely following your reasoning here. Here's how the code looks after your change: 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)); Which of the above memcpy() operations is subject to a pointer that firmware can influence exactly ? Is this a real problem you've seen if so please add a backtrace to this commit log. > } else { > /* bad packet received, dropping */ > new_rd_idx = qhdr->write_idx; > If this is a fix it requires a Fixes: tag. Please add. Still finding the reasoning you are outlining here not obvious. --- bod
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c index f9437b6412b91c2483670a2b11f4fd43f3206404..64cc9e916f53e5a9c82b1ff25c4475d622ebc321 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); } + memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32)); } else { /* bad packet received, dropping */ new_rd_idx = qhdr->write_idx;
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(+)