On 11/5/2024 7:28 PM, Dmitry Baryshkov wrote:
On Tue, Nov 05, 2024 at 02:24:57PM +0530, Vikash Garodia wrote:
sfr->buf_size is in shared memory and can be modified by malicious user. OOB write is possible when the size is made higher than actual sfr data buffer.
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 | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c index 50d92214190d88eff273a5ba3f95486f758bcc05..c19d6bf686d0f31c6a2f551de3f7eb08031bde85 100644 --- a/drivers/media/platform/qcom/venus/hfi_venus.c +++ b/drivers/media/platform/qcom/venus/hfi_venus.c @@ -1041,18 +1041,23 @@ static void venus_sfr_print(struct venus_hfi_device *hdev) { struct device *dev = hdev->core->dev; struct hfi_sfr *sfr = hdev->sfr.kva;
- u32 size; void *p;
if (!sfr) return;
- p = memchr(sfr->data, '\0', sfr->buf_size);
- size = sfr->buf_size;
- if (size > ALIGNED_SFR_SIZE)
return;Why can't you just limit size to ALIGNED_SFR_SIZE, still allowing the data to be captured?
That should do as well. Updating above check to below would take care of it. if (size > ALIGNED_SFR_SIZE) size = ALIGNED_SFR_SIZE;
Regards, Vikash
- p = memchr(sfr->data, '\0', size); /*
*/ if (!p)
- SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates
- that Venus is in the process of crashing.
sfr->data[sfr->buf_size - 1] = '\0';
sfr->data[size - 1] = '\0';dev_err_ratelimited(dev, "SFR message from FW: %s\n", sfr->data); }
-- 2.34.1