[v4] Venus driver fixes to avoid possible OOB accesses

[PATCH v4 4/4] media: venus: hfi: add a check to handle OOB in sfr region

Posted by Vikash Garodia 1 year ago

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. Cap the size to allocated size for such cases.

Cc: stable@vger.kernel.org
Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
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 6b615270c5dae470c6fad408c9b5bc037883e56e..c3113420d266e61fcab44688580288d7408b50f4 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)
+		size = ALIGNED_SFR_SIZE;
+
+	p = memchr(sfr->data, '\0', size);
 	/*
 	 * SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates
 	 * that Venus is in the process of crashing.
 	 */
 	if (!p)
-		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

Re: [PATCH v4 4/4] media: venus: hfi: add a check to handle OOB in sfr region

Posted by Hans Verkuil 11 months, 3 weeks ago

On 2/7/25 09:24, 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. Cap the size to allocated size for such cases.
> 
> Cc: stable@vger.kernel.org
> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 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 6b615270c5dae470c6fad408c9b5bc037883e56e..c3113420d266e61fcab44688580288d7408b50f4 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 this is ever 0...

> +	if (size > ALIGNED_SFR_SIZE)
> +		size = ALIGNED_SFR_SIZE;
> +
> +	p = memchr(sfr->data, '\0', size);
>  	/*
>  	 * SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates
>  	 * that Venus is in the process of crashing.
>  	 */
>  	if (!p)
> -		sfr->data[sfr->buf_size - 1] = '\0';
> +		sfr->data[size - 1] = '\0';

...then this will overwrite memory. It probably can't be 0, but a check or perhaps
just a comment might be good. It looks a bit scary.

Regards,

	Hans

>  
>  	dev_err_ratelimited(dev, "SFR message from FW: %s\n", sfr->data);
>  }
>

Re: [PATCH v4 4/4] media: venus: hfi: add a check to handle OOB in sfr region

Posted by Vikash Garodia 11 months, 3 weeks ago

On 2/20/2025 8:53 PM, Hans Verkuil wrote:
> On 2/7/25 09:24, 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. Cap the size to allocated size for such cases.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> 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 6b615270c5dae470c6fad408c9b5bc037883e56e..c3113420d266e61fcab44688580288d7408b50f4 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 this is ever 0...
> 
>> +	if (size > ALIGNED_SFR_SIZE)
>> +		size = ALIGNED_SFR_SIZE;
>> +
>> +	p = memchr(sfr->data, '\0', size);
>>  	/*
>>  	 * SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates
>>  	 * that Venus is in the process of crashing.
>>  	 */
>>  	if (!p)
>> -		sfr->data[sfr->buf_size - 1] = '\0';
>> +		sfr->data[size - 1] = '\0';
> 
> ...then this will overwrite memory. It probably can't be 0, but a check or perhaps
> just a comment might be good. It looks a bit scary.
Thats correct, it would not be 0 as its a prefixed one [1]. I can put up a
comment here.

[1]
https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/media/platform/qcom/venus/hfi_venus.c#L836
> 
> Regards,
> 
> 	Hans
> 
>>  
>>  	dev_err_ratelimited(dev, "SFR message from FW: %s\n", sfr->data);
>>  }
>>
Regards,
Vikash

Re: [PATCH v4 4/4] media: venus: hfi: add a check to handle OOB in sfr region

Posted by Tomasz Figa 11 months, 3 weeks ago

On Fri, Feb 21, 2025 at 12:56 AM Vikash Garodia
<quic_vgarodia@quicinc.com> wrote:
>
>
> On 2/20/2025 8:53 PM, Hans Verkuil wrote:
> > On 2/7/25 09:24, 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. Cap the size to allocated size for such cases.
> >>
> >> Cc: stable@vger.kernel.org
> >> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> >> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> >> 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 6b615270c5dae470c6fad408c9b5bc037883e56e..c3113420d266e61fcab44688580288d7408b50f4 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 this is ever 0...
> >
> >> +    if (size > ALIGNED_SFR_SIZE)
> >> +            size = ALIGNED_SFR_SIZE;
> >> +
> >> +    p = memchr(sfr->data, '\0', size);
> >>      /*
> >>       * SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates
> >>       * that Venus is in the process of crashing.
> >>       */
> >>      if (!p)
> >> -            sfr->data[sfr->buf_size - 1] = '\0';
> >> +            sfr->data[size - 1] = '\0';
> >
> > ...then this will overwrite memory. It probably can't be 0, but a check or perhaps
> > just a comment might be good. It looks a bit scary.
> Thats correct, it would not be 0 as its a prefixed one [1]. I can put up a
> comment here.

Couldn't a bug (or vulnerability) in the firmware actually still cause
it to write 0 there?

>
> [1]
> https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/media/platform/qcom/venus/hfi_venus.c#L836
> >
> > Regards,
> >
> >       Hans
> >
> >>
> >>      dev_err_ratelimited(dev, "SFR message from FW: %s\n", sfr->data);
> >>  }
> >>
> Regards,
> Vikash

Re: [PATCH v4 4/4] media: venus: hfi: add a check to handle OOB in sfr region

Posted by Vikash Garodia 11 months, 3 weeks ago

On 2/21/2025 9:25 AM, Tomasz Figa wrote:
> On Fri, Feb 21, 2025 at 12:56 AM Vikash Garodia
> <quic_vgarodia@quicinc.com> wrote:
>>
>>
>> On 2/20/2025 8:53 PM, Hans Verkuil wrote:
>>> On 2/7/25 09:24, 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. Cap the size to allocated size for such cases.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>> 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 6b615270c5dae470c6fad408c9b5bc037883e56e..c3113420d266e61fcab44688580288d7408b50f4 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 this is ever 0...
>>>
>>>> +    if (size > ALIGNED_SFR_SIZE)
>>>> +            size = ALIGNED_SFR_SIZE;
>>>> +
>>>> +    p = memchr(sfr->data, '\0', size);
>>>>      /*
>>>>       * SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates
>>>>       * that Venus is in the process of crashing.
>>>>       */
>>>>      if (!p)
>>>> -            sfr->data[sfr->buf_size - 1] = '\0';
>>>> +            sfr->data[size - 1] = '\0';
>>>
>>> ...then this will overwrite memory. It probably can't be 0, but a check or perhaps
>>> just a comment might be good. It looks a bit scary.
>> Thats correct, it would not be 0 as its a prefixed one [1]. I can put up a
>> comment here.
> 
> Couldn't a bug (or vulnerability) in the firmware actually still cause
> it to write 0 there?
Possible. Though the size is initialized in driver with "ALIGNED_SFR_SIZE",
there is a possibility that the same could get overwritten by a rogue firmware.
Kept a check in v5, which cache the value locally and then does the check before
using that value.

Regards
Vikash
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/media/platform/qcom/venus/hfi_venus.c#L836
>>>
>>> Regards,
>>>
>>>       Hans
>>>
>>>>
>>>>      dev_err_ratelimited(dev, "SFR message from FW: %s\n", sfr->data);
>>>>  }
>>>>
>> Regards,
>> Vikash