| Message ID | 20241105-venus_oob-v1-3-8d4feedfe2bb@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Venus driver fixes to avoid possible OOB accesses | expand |
On 05/11/2024 08:54, Vikash Garodia wrote: > qsize represents size of shared queued between driver and video > firmware. Firmware can modify this value to an invalid large value. In > such situation, empty_space will be bigger than the space actually > available. Since new_wr_idx is not checked, so the following code will > result in an OOB write. > ... > qsize = qhdr->q_size > > if (wr_idx >= rd_idx) > empty_space = qsize - (wr_idx - rd_idx) > .... > if (new_wr_idx < qsize) { > memcpy(wr_ptr, packet, dwords << 2) --> OOB write > > Add check to ensure qsize is within the allocated size while > reading and writing packets into the queue. > > Cc: stable@vger.kernel.org > Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files") > Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> > --- > drivers/media/platform/qcom/venus/hfi_venus.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c > index f9437b6412b91c2483670a2b11f4fd43f3206404..50d92214190d88eff273a5ba3f95486f758bcc05 100644 > --- a/drivers/media/platform/qcom/venus/hfi_venus.c > +++ b/drivers/media/platform/qcom/venus/hfi_venus.c > @@ -187,6 +187,9 @@ static int venus_write_queue(struct venus_hfi_device *hdev, > /* ensure rd/wr indices's are read from memory */ > rmb(); > > + if (qsize > IFACEQ_QUEUE_SIZE/4) > + return -EINVAL; > + > if (wr_idx >= rd_idx) > empty_space = qsize - (wr_idx - rd_idx); > else > @@ -255,6 +258,9 @@ static int venus_read_queue(struct venus_hfi_device *hdev, > wr_idx = qhdr->write_idx; > qsize = qhdr->q_size; > > + if (qsize > IFACEQ_QUEUE_SIZE/4) > + return -EINVAL; > + > /* make sure data is valid before using it */ > rmb(); > > You have this same calculation in venus_set_qhdr_defaults() really needs a macro or something to stop repeating the same code in another patch later. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c index f9437b6412b91c2483670a2b11f4fd43f3206404..50d92214190d88eff273a5ba3f95486f758bcc05 100644 --- a/drivers/media/platform/qcom/venus/hfi_venus.c +++ b/drivers/media/platform/qcom/venus/hfi_venus.c @@ -187,6 +187,9 @@ static int venus_write_queue(struct venus_hfi_device *hdev, /* ensure rd/wr indices's are read from memory */ rmb(); + if (qsize > IFACEQ_QUEUE_SIZE/4) + return -EINVAL; + if (wr_idx >= rd_idx) empty_space = qsize - (wr_idx - rd_idx); else @@ -255,6 +258,9 @@ static int venus_read_queue(struct venus_hfi_device *hdev, wr_idx = qhdr->write_idx; qsize = qhdr->q_size; + if (qsize > IFACEQ_QUEUE_SIZE/4) + return -EINVAL; + /* make sure data is valid before using it */ rmb();
qsize represents size of shared queued between driver and video firmware. Firmware can modify this value to an invalid large value. In such situation, empty_space will be bigger than the space actually available. Since new_wr_idx is not checked, so the following code will result in an OOB write. ... qsize = qhdr->q_size if (wr_idx >= rd_idx) empty_space = qsize - (wr_idx - rd_idx) .... if (new_wr_idx < qsize) { memcpy(wr_ptr, packet, dwords << 2) --> OOB write Add check to ensure qsize is within the allocated size while reading and writing packets into the queue. Cc: stable@vger.kernel.org Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files") Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com> --- drivers/media/platform/qcom/venus/hfi_venus.c | 6 ++++++ 1 file changed, 6 insertions(+)