[PATCH 0/4] Venus driver fixes to avoid possible OOB accesses

[PATCH 0/4] Venus driver fixes to avoid possible OOB accesses

[Vikash Garodia]

This series primarily adds check at relevant places in venus driver where there
are possible OOB accesses due to unexpected payload from venus firmware. The
patches describes the specific OOB possibility.

Please review and share your feedback.

Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
Vikash Garodia (4):
      media: venus: hfi_parser: add check to avoid out of bound access
      media: venus: hfi_parser: avoid OOB access beyond payload word count
      media: venus: hfi: add check to handle incorrect queue size
      media: venus: hfi: add a check to handle OOB in sfr region

 drivers/media/platform/qcom/venus/hfi_parser.c |  6 +++++-
 drivers/media/platform/qcom/venus/hfi_venus.c  | 15 +++++++++++++--
 2 files changed, 18 insertions(+), 3 deletions(-)
---
base-commit: c7ccf3683ac9746b263b0502255f5ce47f64fe0a
change-id: 20241104-venus_oob-0343b143d61d

Best regards,
-- 
Vikash Garodia <quic_vgarodia@quicinc.com>

[PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Vikash Garodia]

There is a possibility that init_codecs is invoked multiple times during
manipulated payload from video firmware. In such case, if codecs_count
can get incremented to value more than MAX_CODEC_NUM, there can be OOB
access. Keep a check for max accessible memory before accessing it.

Cc: stable@vger.kernel.org
Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
 drivers/media/platform/qcom/venus/hfi_parser.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
index 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..27d0172294d5154f4839e8cef172f9a619dfa305 100644
--- a/drivers/media/platform/qcom/venus/hfi_parser.c
+++ b/drivers/media/platform/qcom/venus/hfi_parser.c
@@ -23,6 +23,8 @@ static void init_codecs(struct venus_core *core)
 		return;
 
 	for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
+		if (core->codecs_count >= MAX_CODEC_NUM)
+			return;
 		cap = &caps[core->codecs_count++];
 		cap->codec = BIT(bit);
 		cap->domain = VIDC_SESSION_TYPE_DEC;
@@ -30,6 +32,8 @@ static void init_codecs(struct venus_core *core)
 	}
 
 	for_each_set_bit(bit, &core->enc_codecs, MAX_CODEC_NUM) {
+		if (core->codecs_count >= MAX_CODEC_NUM)
+			return;
 		cap = &caps[core->codecs_count++];
 		cap->codec = BIT(bit);
 		cap->domain = VIDC_SESSION_TYPE_ENC;

-- 
2.34.1

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Bryan O'Donoghue]

On 05/11/2024 08:54, Vikash Garodia wrote:
> There is a possibility that init_codecs is invoked multiple times during
> manipulated payload from video firmware. In such case, if codecs_count
> can get incremented to value more than MAX_CODEC_NUM, there can be OOB
> access. Keep a check for max accessible memory before accessing it.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>   drivers/media/platform/qcom/venus/hfi_parser.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..27d0172294d5154f4839e8cef172f9a619dfa305 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -23,6 +23,8 @@ static void init_codecs(struct venus_core *core)
>   		return;
>   
>   	for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
> +		if (core->codecs_count >= MAX_CODEC_NUM)
> +			return;
>   		cap = &caps[core->codecs_count++];
>   		cap->codec = BIT(bit);
>   		cap->domain = VIDC_SESSION_TYPE_DEC;
> @@ -30,6 +32,8 @@ static void init_codecs(struct venus_core *core)
>   	}
>   
>   	for_each_set_bit(bit, &core->enc_codecs, MAX_CODEC_NUM) {
> +		if (core->codecs_count >= MAX_CODEC_NUM)
> +			return;
>   		cap = &caps[core->codecs_count++];
>   		cap->codec = BIT(bit);
>   		cap->domain = VIDC_SESSION_TYPE_ENC;
> 

I don't see how codecs_count could be greater than the control, since 
you increment by one on each loop but >= is fine too I suppose.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Vikash Garodia]

On 11/5/2024 4:21 PM, Bryan O'Donoghue wrote:
> On 05/11/2024 08:54, Vikash Garodia wrote:
>> There is a possibility that init_codecs is invoked multiple times during
>> manipulated payload from video firmware. In such case, if codecs_count
>> can get incremented to value more than MAX_CODEC_NUM, there can be OOB
>> access. Keep a check for max accessible memory before accessing it.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_parser.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c
>> b/drivers/media/platform/qcom/venus/hfi_parser.c
>> index
>> 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..27d0172294d5154f4839e8cef172f9a619dfa305 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>> @@ -23,6 +23,8 @@ static void init_codecs(struct venus_core *core)
>>           return;
>>         for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
>> +        if (core->codecs_count >= MAX_CODEC_NUM)
>> +            return;
>>           cap = &caps[core->codecs_count++];
>>           cap->codec = BIT(bit);
>>           cap->domain = VIDC_SESSION_TYPE_DEC;
>> @@ -30,6 +32,8 @@ static void init_codecs(struct venus_core *core)
>>       }
>>         for_each_set_bit(bit, &core->enc_codecs, MAX_CODEC_NUM) {
>> +        if (core->codecs_count >= MAX_CODEC_NUM)
>> +            return;
>>           cap = &caps[core->codecs_count++];
>>           cap->codec = BIT(bit);
>>           cap->domain = VIDC_SESSION_TYPE_ENC;
>>
> 
> I don't see how codecs_count could be greater than the control, since you
> increment by one on each loop but >= is fine too I suppose.
Assume the payload from malicious firmware is packed like below
HFI_PROPERTY_PARAM_CODEC_SUPPORTED
HFI_PROPERTY_PARAM_CODEC_SUPPORTED
HFI_PROPERTY_PARAM_CODEC_SUPPORTED
.....
for 32 or more instances of above type

Regards,
Vikash
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Bryan O'Donoghue]

On 06/11/2024 07:25, Vikash Garodia wrote:
>>>            cap = &caps[core->codecs_count++];
>>>            cap->codec = BIT(bit);
>>>            cap->domain = VIDC_SESSION_TYPE_ENC;
>>>
>> I don't see how codecs_count could be greater than the control, since you
>> increment by one on each loop but >= is fine too I suppose.
> Assume the payload from malicious firmware is packed like below
> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> .....
> for 32 or more instances of above type

But you do this

           cap = &caps[core->codecs_count++];

for each bit.

Anyway consider Dmitry's input re only calling this function once instead.

---
bod

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Vikash Garodia]

On 11/6/2024 3:53 PM, Bryan O'Donoghue wrote:
> On 06/11/2024 07:25, Vikash Garodia wrote:
>>>>            cap = &caps[core->codecs_count++];
>>>>            cap->codec = BIT(bit);
>>>>            cap->domain = VIDC_SESSION_TYPE_ENC;
>>>>
>>> I don't see how codecs_count could be greater than the control, since you
>>> increment by one on each loop but >= is fine too I suppose.
>> Assume the payload from malicious firmware is packed like below
>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>> .....
>> for 32 or more instances of above type
> 
> But you do this
> 
>           cap = &caps[core->codecs_count++];
> 
> for each bit.
Yes. Let say that packet is written more than 32 times in the payload response
from bad firmware and each has 1 bit set. core->codecs_count would be
incremented beyond the allocated space.

Regards,
Vikash

> 
> Anyway consider Dmitry's input re only calling this function once instead.
> 
> ---
> bod

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Dmitry Baryshkov]

On Tue, Nov 05, 2024 at 02:24:54PM +0530, Vikash Garodia wrote:
> There is a possibility that init_codecs is invoked multiple times during
> manipulated payload from video firmware. In such case, if codecs_count
> can get incremented to value more than MAX_CODEC_NUM, there can be OOB
> access. Keep a check for max accessible memory before accessing it.

No. Please make sure that init_codecs() does a correct thing, so that
core->codecs_count isn't incremented that much (or even better that
init_codecs() doesn't do anything if it is executed second time).

> 
> Cc: stable@vger.kernel.org
> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>  drivers/media/platform/qcom/venus/hfi_parser.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..27d0172294d5154f4839e8cef172f9a619dfa305 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -23,6 +23,8 @@ static void init_codecs(struct venus_core *core)
>  		return;
>  
>  	for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
> +		if (core->codecs_count >= MAX_CODEC_NUM)
> +			return;
>  		cap = &caps[core->codecs_count++];
>  		cap->codec = BIT(bit);
>  		cap->domain = VIDC_SESSION_TYPE_DEC;
> @@ -30,6 +32,8 @@ static void init_codecs(struct venus_core *core)
>  	}
>  
>  	for_each_set_bit(bit, &core->enc_codecs, MAX_CODEC_NUM) {
> +		if (core->codecs_count >= MAX_CODEC_NUM)
> +			return;
>  		cap = &caps[core->codecs_count++];
>  		cap->codec = BIT(bit);
>  		cap->domain = VIDC_SESSION_TYPE_ENC;
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Vikash Garodia]

On 11/5/2024 7:25 PM, Dmitry Baryshkov wrote:
> On Tue, Nov 05, 2024 at 02:24:54PM +0530, Vikash Garodia wrote:
>> There is a possibility that init_codecs is invoked multiple times during
>> manipulated payload from video firmware. In such case, if codecs_count
>> can get incremented to value more than MAX_CODEC_NUM, there can be OOB
>> access. Keep a check for max accessible memory before accessing it.
> 
> No. Please make sure that init_codecs() does a correct thing, so that
> core->codecs_count isn't incremented that much (or even better that
> init_codecs() doesn't do anything if it is executed second time).
init_codecs() parses the payload received from firmware and . I don't think we
can control this part when we have something like this from a malicious firmware
payload
HFI_PROPERTY_PARAM_CODEC_SUPPORTED
HFI_PROPERTY_PARAM_CODEC_SUPPORTED
HFI_PROPERTY_PARAM_CODEC_SUPPORTED
...
Limiting it to second iteration would restrict the functionality when property
HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs.

Regards,
Vikash
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
>>  drivers/media/platform/qcom/venus/hfi_parser.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
>> index 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..27d0172294d5154f4839e8cef172f9a619dfa305 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>> @@ -23,6 +23,8 @@ static void init_codecs(struct venus_core *core)
>>  		return;
>>  
>>  	for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
>> +		if (core->codecs_count >= MAX_CODEC_NUM)
>> +			return;
>>  		cap = &caps[core->codecs_count++];
>>  		cap->codec = BIT(bit);
>>  		cap->domain = VIDC_SESSION_TYPE_DEC;
>> @@ -30,6 +32,8 @@ static void init_codecs(struct venus_core *core)
>>  	}
>>  
>>  	for_each_set_bit(bit, &core->enc_codecs, MAX_CODEC_NUM) {
>> +		if (core->codecs_count >= MAX_CODEC_NUM)
>> +			return;
>>  		cap = &caps[core->codecs_count++];
>>  		cap->codec = BIT(bit);
>>  		cap->domain = VIDC_SESSION_TYPE_ENC;
>>
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Dmitry Baryshkov]

On Thu, Nov 07, 2024 at 01:47:20PM +0530, Vikash Garodia wrote:
> 
> On 11/5/2024 7:25 PM, Dmitry Baryshkov wrote:
> > On Tue, Nov 05, 2024 at 02:24:54PM +0530, Vikash Garodia wrote:
> >> There is a possibility that init_codecs is invoked multiple times during
> >> manipulated payload from video firmware. In such case, if codecs_count
> >> can get incremented to value more than MAX_CODEC_NUM, there can be OOB
> >> access. Keep a check for max accessible memory before accessing it.
> > 
> > No. Please make sure that init_codecs() does a correct thing, so that
> > core->codecs_count isn't incremented that much (or even better that
> > init_codecs() doesn't do anything if it is executed second time).
> init_codecs() parses the payload received from firmware and . I don't think we
> can control this part when we have something like this from a malicious firmware
> payload
> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> ...
> Limiting it to second iteration would restrict the functionality when property
> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs.

If you can have a malicious firmware (which is owned and signed by
Qualcomm / OEM), then you have to be careful and skip duplicates. So
instead of just adding new cap to core->caps, you have to go through
that array, check that you are not adding a duplicate (and report a
[Firmware Bug] for duplicates), check that there is an empty slot, etc.

Just ignoring the "extra" entries is not enough.

> 
> Regards,
> Vikash
> >>
> >> Cc: stable@vger.kernel.org
> >> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
> >> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> >> ---
> >>  drivers/media/platform/qcom/venus/hfi_parser.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> >> index 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..27d0172294d5154f4839e8cef172f9a619dfa305 100644
> >> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> >> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> >> @@ -23,6 +23,8 @@ static void init_codecs(struct venus_core *core)
> >>  		return;
> >>  
> >>  	for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
> >> +		if (core->codecs_count >= MAX_CODEC_NUM)
> >> +			return;
> >>  		cap = &caps[core->codecs_count++];
> >>  		cap->codec = BIT(bit);
> >>  		cap->domain = VIDC_SESSION_TYPE_DEC;
> >> @@ -30,6 +32,8 @@ static void init_codecs(struct venus_core *core)
> >>  	}
> >>  
> >>  	for_each_set_bit(bit, &core->enc_codecs, MAX_CODEC_NUM) {
> >> +		if (core->codecs_count >= MAX_CODEC_NUM)
> >> +			return;
> >>  		cap = &caps[core->codecs_count++];
> >>  		cap->codec = BIT(bit);
> >>  		cap->domain = VIDC_SESSION_TYPE_ENC;
> >>
> >> -- 
> >> 2.34.1
> >>
> > 

-- 
With best wishes
Dmitry

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Bryan O'Donoghue]

On 07/11/2024 10:41, Dmitry Baryshkov wrote:
>> init_codecs() parses the payload received from firmware and . I don't think we
>> can control this part when we have something like this from a malicious firmware
>> payload
>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>> ...
>> Limiting it to second iteration would restrict the functionality when property
>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs.
> If you can have a malicious firmware (which is owned and signed by
> Qualcomm / OEM), then you have to be careful and skip duplicates. So
> instead of just adding new cap to core->caps, you have to go through
> that array, check that you are not adding a duplicate (and report a
> [Firmware Bug] for duplicates), check that there is an empty slot, etc.
> 
> Just ignoring the "extra" entries is not enough.

+1

This is a more rational argument. If you get a second message, you 
should surely reinit the whole array i.e. update the array with the new 
list, as opposed to throwing away the second message because it 
over-indexes your local storage..

---
bod

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Vikash Garodia]

On 11/7/2024 5:37 PM, Bryan O'Donoghue wrote:
> On 07/11/2024 10:41, Dmitry Baryshkov wrote:
>>> init_codecs() parses the payload received from firmware and . I don't think we
>>> can control this part when we have something like this from a malicious firmware
>>> payload
>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>>> ...
>>> Limiting it to second iteration would restrict the functionality when property
>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs.
>> If you can have a malicious firmware (which is owned and signed by
>> Qualcomm / OEM), then you have to be careful and skip duplicates. So
>> instead of just adding new cap to core->caps, you have to go through
>> that array, check that you are not adding a duplicate (and report a
>> [Firmware Bug] for duplicates), check that there is an empty slot, etc.
>>
>> Just ignoring the "extra" entries is not enough.
Thinking of something like this

for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
    if (core->codecs_count >= MAX_CODEC_NUM)
        return;
    cap = &caps[core->codecs_count++];
    if (cap->codec == BIT(bit)) --> each code would have unique bitfield
        return;
> +1
> 
> This is a more rational argument. If you get a second message, you should surely
> reinit the whole array i.e. update the array with the new list, as opposed to
> throwing away the second message because it over-indexes your local storage..
That would be incorrect to overwrite the array with new list, whenever new
payload is received.

Regards,
Vikash

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Dmitry Baryshkov]

On Thu, Nov 07, 2024 at 06:32:33PM +0530, Vikash Garodia wrote:
> 
> On 11/7/2024 5:37 PM, Bryan O'Donoghue wrote:
> > On 07/11/2024 10:41, Dmitry Baryshkov wrote:
> >>> init_codecs() parses the payload received from firmware and . I don't think we
> >>> can control this part when we have something like this from a malicious firmware
> >>> payload
> >>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> >>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> >>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> >>> ...
> >>> Limiting it to second iteration would restrict the functionality when property
> >>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs.
> >> If you can have a malicious firmware (which is owned and signed by
> >> Qualcomm / OEM), then you have to be careful and skip duplicates. So
> >> instead of just adding new cap to core->caps, you have to go through
> >> that array, check that you are not adding a duplicate (and report a
> >> [Firmware Bug] for duplicates), check that there is an empty slot, etc.
> >>
> >> Just ignoring the "extra" entries is not enough.
> Thinking of something like this
> 
> for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
>     if (core->codecs_count >= MAX_CODEC_NUM)
>         return;
>     cap = &caps[core->codecs_count++];
>     if (cap->codec == BIT(bit)) --> each code would have unique bitfield
>         return;

This won't work and it's pretty obvious why.

> > +1
> > 
> > This is a more rational argument. If you get a second message, you should surely
> > reinit the whole array i.e. update the array with the new list, as opposed to
> > throwing away the second message because it over-indexes your local storage..
> That would be incorrect to overwrite the array with new list, whenever new
> payload is received.

I'd say, don't overwrite the array. Instead the driver should extend it
with the new information.

> 
> Regards,
> Vikash

-- 
With best wishes
Dmitry

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Vikash Garodia]

On 11/7/2024 6:52 PM, Dmitry Baryshkov wrote:
> On Thu, Nov 07, 2024 at 06:32:33PM +0530, Vikash Garodia wrote:
>>
>> On 11/7/2024 5:37 PM, Bryan O'Donoghue wrote:
>>> On 07/11/2024 10:41, Dmitry Baryshkov wrote:
>>>>> init_codecs() parses the payload received from firmware and . I don't think we
>>>>> can control this part when we have something like this from a malicious firmware
>>>>> payload
>>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>>>>> ...
>>>>> Limiting it to second iteration would restrict the functionality when property
>>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs.
>>>> If you can have a malicious firmware (which is owned and signed by
>>>> Qualcomm / OEM), then you have to be careful and skip duplicates. So
>>>> instead of just adding new cap to core->caps, you have to go through
>>>> that array, check that you are not adding a duplicate (and report a
>>>> [Firmware Bug] for duplicates), check that there is an empty slot, etc.
>>>>
>>>> Just ignoring the "extra" entries is not enough.
>> Thinking of something like this
>>
>> for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
>>     if (core->codecs_count >= MAX_CODEC_NUM)
>>         return;
>>     cap = &caps[core->codecs_count++];
>>     if (cap->codec == BIT(bit)) --> each code would have unique bitfield
>>         return;
> 
> This won't work and it's pretty obvious why.
Could you please elaborate what would break in above logic ?

> 
>>> +1
>>>
>>> This is a more rational argument. If you get a second message, you should surely
>>> reinit the whole array i.e. update the array with the new list, as opposed to
>>> throwing away the second message because it over-indexes your local storage..
>> That would be incorrect to overwrite the array with new list, whenever new
>> payload is received.
> 
> I'd say, don't overwrite the array. Instead the driver should extend it
> with the new information.
That is exactly the existing patch is currently doing.

Regards,
Vikash
> 
>>
>> Regards,
>> Vikash
> 

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Dmitry Baryshkov]

On Thu, Nov 07, 2024 at 07:05:15PM +0530, Vikash Garodia wrote:
> 
> On 11/7/2024 6:52 PM, Dmitry Baryshkov wrote:
> > On Thu, Nov 07, 2024 at 06:32:33PM +0530, Vikash Garodia wrote:
> >>
> >> On 11/7/2024 5:37 PM, Bryan O'Donoghue wrote:
> >>> On 07/11/2024 10:41, Dmitry Baryshkov wrote:
> >>>>> init_codecs() parses the payload received from firmware and . I don't think we
> >>>>> can control this part when we have something like this from a malicious firmware
> >>>>> payload
> >>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> >>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> >>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> >>>>> ...
> >>>>> Limiting it to second iteration would restrict the functionality when property
> >>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs.
> >>>> If you can have a malicious firmware (which is owned and signed by
> >>>> Qualcomm / OEM), then you have to be careful and skip duplicates. So
> >>>> instead of just adding new cap to core->caps, you have to go through
> >>>> that array, check that you are not adding a duplicate (and report a
> >>>> [Firmware Bug] for duplicates), check that there is an empty slot, etc.
> >>>>
> >>>> Just ignoring the "extra" entries is not enough.
> >> Thinking of something like this
> >>
> >> for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
> >>     if (core->codecs_count >= MAX_CODEC_NUM)
> >>         return;
> >>     cap = &caps[core->codecs_count++];
> >>     if (cap->codec == BIT(bit)) --> each code would have unique bitfield
> >>         return;
> > 
> > This won't work and it's pretty obvious why.
> Could you please elaborate what would break in above logic ?

After the "cap=&caps[core->codecs_count++]" line 'cap' will point to the
new entry, which should not contain valid data.

Instead, when processing new 'bit' you should loop over the existing
caps and check that there is no match. And only if there is no match
the code should be allocating new entry, checking that codecs_count
doesn't overflow, etc.

> 
> > 
> >>> +1
> >>>
> >>> This is a more rational argument. If you get a second message, you should surely
> >>> reinit the whole array i.e. update the array with the new list, as opposed to
> >>> throwing away the second message because it over-indexes your local storage..
> >> That would be incorrect to overwrite the array with new list, whenever new
> >> payload is received.
> > 
> > I'd say, don't overwrite the array. Instead the driver should extend it
> > with the new information.
> That is exactly the existing patch is currently doing.

_new_ information, not a copy of the existing information.

> 
> Regards,
> Vikash
> > 
> >>
> >> Regards,
> >> Vikash
> > 

-- 
With best wishes
Dmitry

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Bryan O'Donoghue]

On 07/11/2024 13:54, Dmitry Baryshkov wrote:
>>> I'd say, don't overwrite the array. Instead the driver should extend it
>>> with the new information.
>> That is exactly the existing patch is currently doing.
> _new_ information, not a copy of the existing information.

But is this _really_ new information or is it guarding from "malicious" 
additional messages ?

@Vikash is it even a valid use-case for firmware to send one set of 
capabilities and then send a new set ?

It seems to me this should only happen once when the firmware starts up 
- the firmware won't acquire any new abilities once it has enumerated 
its set to APSS.

So why is it valid to process an additional message at all ?

Shouldn't we instead be throwing away redundant updates either silently 
or with some kind of complaint ?

If there's no new data - then this is data we shouldn't bother processing.

If it is new data then surely it should be the _current_ and _only_ 
valid set of data.

And if the update is considered "invalid" then why _would_ we accept the 
update ?

I get we're fixing the OOB but I think we should be clear on the 
validity of the content of the packet.

---
bod

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Vikash Garodia]

On 11/8/2024 5:13 PM, Bryan O'Donoghue wrote:
> On 07/11/2024 13:54, Dmitry Baryshkov wrote:
>>>> I'd say, don't overwrite the array. Instead the driver should extend it
>>>> with the new information.
>>> That is exactly the existing patch is currently doing.
>> _new_ information, not a copy of the existing information.
> 
> But is this _really_ new information or is it guarding from "malicious"
> additional messages ?
> 
> @Vikash is it even a valid use-case for firmware to send one set of capabilities
> and then send a new set ?
> 
> It seems to me this should only happen once when the firmware starts up - the
> firmware won't acquire any new abilities once it has enumerated its set to APSS.
> 
> So why is it valid to process an additional message at all ?
> 
> Shouldn't we instead be throwing away redundant updates either silently or with
> some kind of complaint ?
> 
> If there's no new data - then this is data we shouldn't bother processing.
> 
> If it is new data then surely it should be the _current_ and _only_ valid set of
> data.
> 
> And if the update is considered "invalid" then why _would_ we accept the update ?
> 
> I get we're fixing the OOB but I think we should be clear on the validity of the
> content of the packet.
The payload [1] is all about 2 u32s each for decoder and encoder bit masks,
while each bit signifies which codec each supports. So in a good case, it would
be always first iteration which would be sufficient. Its a very hypothetical
case where a good case would that there are 8 payloads (consider there are 8
supported codecs) with one bit set in each of those 8 payloads. I was initially
thinking to cover for this case as well, seems could be a bit of over designing.

Maybe set core->codecs_count (to 0) in the beginning of the API should cover the
working case. In malicious case, let it continue to override ?

Regards,
Vikash

[1]
https://elixir.bootlin.com/linux/v6.12-rc7/source/drivers/media/platform/qcom/venus/hfi_parser.c#L193

Re: [PATCH 1/4] media: venus: hfi_parser: add check to avoid out of bound access

[Vikash Garodia]

On 11/7/2024 7:24 PM, Dmitry Baryshkov wrote:
> On Thu, Nov 07, 2024 at 07:05:15PM +0530, Vikash Garodia wrote:
>>
>> On 11/7/2024 6:52 PM, Dmitry Baryshkov wrote:
>>> On Thu, Nov 07, 2024 at 06:32:33PM +0530, Vikash Garodia wrote:
>>>>
>>>> On 11/7/2024 5:37 PM, Bryan O'Donoghue wrote:
>>>>> On 07/11/2024 10:41, Dmitry Baryshkov wrote:
>>>>>>> init_codecs() parses the payload received from firmware and . I don't think we
>>>>>>> can control this part when we have something like this from a malicious firmware
>>>>>>> payload
>>>>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>>>>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>>>>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>>>>>>> ...
>>>>>>> Limiting it to second iteration would restrict the functionality when property
>>>>>>> HFI_PROPERTY_PARAM_CODEC_SUPPORTED is sent for supported number of codecs.
>>>>>> If you can have a malicious firmware (which is owned and signed by
>>>>>> Qualcomm / OEM), then you have to be careful and skip duplicates. So
>>>>>> instead of just adding new cap to core->caps, you have to go through
>>>>>> that array, check that you are not adding a duplicate (and report a
>>>>>> [Firmware Bug] for duplicates), check that there is an empty slot, etc.
>>>>>>
>>>>>> Just ignoring the "extra" entries is not enough.
>>>> Thinking of something like this
>>>>
>>>> for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
>>>>     if (core->codecs_count >= MAX_CODEC_NUM)
>>>>         return;
>>>>     cap = &caps[core->codecs_count++];
>>>>     if (cap->codec == BIT(bit)) --> each code would have unique bitfield
>>>>         return;
>>>
>>> This won't work and it's pretty obvious why.
>> Could you please elaborate what would break in above logic ?
> 
> After the "cap=&caps[core->codecs_count++]" line 'cap' will point to the
> new entry, which should not contain valid data.
> 
> Instead, when processing new 'bit' you should loop over the existing
> caps and check that there is no match. And only if there is no match
> the code should be allocating new entry, checking that codecs_count
> doesn't overflow, etc.
Got it.

Regards,
Vikash
>>
>>>
>>>>> +1
>>>>>
>>>>> This is a more rational argument. If you get a second message, you should surely
>>>>> reinit the whole array i.e. update the array with the new list, as opposed to
>>>>> throwing away the second message because it over-indexes your local storage..
>>>> That would be incorrect to overwrite the array with new list, whenever new
>>>> payload is received.
>>>
>>> I'd say, don't overwrite the array. Instead the driver should extend it
>>> with the new information.
>> That is exactly the existing patch is currently doing.
> 
> _new_ information, not a copy of the existing information.
> 
>>
>> Regards,
>> Vikash
>>>
>>>>
>>>> Regards,
>>>> Vikash
>>>
> 

[PATCH 2/4] media: venus: hfi_parser: avoid OOB access beyond payload word count

[Vikash Garodia]

words_count denotes the number of words in total payload, while data
points to payload of various property within it. When words_count
reaches last word, data can access memory beyond the total payload.
Avoid this case by not allowing the loop for the last word count.

Cc: stable@vger.kernel.org
Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
 drivers/media/platform/qcom/venus/hfi_parser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
index 27d0172294d5154f4839e8cef172f9a619dfa305..20d9ea3626e9c4468d5f7dbd678743135f027c86 100644
--- a/drivers/media/platform/qcom/venus/hfi_parser.c
+++ b/drivers/media/platform/qcom/venus/hfi_parser.c
@@ -303,7 +303,7 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
 		memset(core->caps, 0, sizeof(core->caps));
 	}
 
-	while (words_count) {
+	while (words_count > 1) {
 		data = word + 1;
 
 		switch (*word) {

-- 
2.34.1

Re: [PATCH 2/4] media: venus: hfi_parser: avoid OOB access beyond payload word count

[Bryan O'Donoghue]

On 05/11/2024 08:54, Vikash Garodia wrote:
> words_count denotes the number of words in total payload, while data
> points to payload of various property within it. When words_count
> reaches last word, data can access memory beyond the total payload.
> Avoid this case by not allowing the loop for the last word count.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>   drivers/media/platform/qcom/venus/hfi_parser.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 27d0172294d5154f4839e8cef172f9a619dfa305..20d9ea3626e9c4468d5f7dbd678743135f027c86 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -303,7 +303,7 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>   		memset(core->caps, 0, sizeof(core->caps));
>   	}
>   
> -	while (words_count) {
> +	while (words_count > 1) {
>   		data = word + 1;
>   
>   		switch (*word) {
> 

How is it the right thing to do to _not_ process the last u32 ?

How does this overrun ? while (words_count) should be fine because it 
decrements at the bottom of the loop...

assuming your buffer is word aligned obvs

=>

#include <stdio.h>
#include <stdint.h>

char somebuf[64];

void init(char *buf, int len)
{
         int i;
         char c = 0;

         for (i = 0; i < len; i++)
                 buf[i] = c++;
}

int hfi_parser(void *buf, int size)
{
         int word_count = size >> 2;
         uint32_t *my_word = (uint32_t*)buf;

         printf("Size %d word_count %d\n", size, word_count);

         while(word_count) {
                 printf("Myword %d == 0x%08x\n", word_count, *my_word);
                 my_word++;
                 word_count--;
         }
}

int main(int argc, char *argv[])
{
         int i;

         init(somebuf, sizeof(somebuf));
         for (i = 0; i < sizeof(somebuf); i++)
                 printf("%x = %x\n", i, somebuf[i]);

         hfi_parser(somebuf, sizeof(somebuf));

         return 0;
}

0 = 0
1 = 1
2 = 2
3 = 3
4 = 4
5 = 5
6 = 6
7 = 7
8 = 8
9 = 9
a = a
b = b
c = c
d = d
e = e
f = f
10 = 10
11 = 11
12 = 12
13 = 13
14 = 14
15 = 15
16 = 16
17 = 17
18 = 18
19 = 19
1a = 1a
1b = 1b
1c = 1c
1d = 1d
1e = 1e
1f = 1f
20 = 20
21 = 21
22 = 22
23 = 23
24 = 24
25 = 25
26 = 26
27 = 27
28 = 28
29 = 29
2a = 2a
2b = 2b
2c = 2c
2d = 2d
2e = 2e
2f = 2f
30 = 30
31 = 31
32 = 32
33 = 33
34 = 34
35 = 35
36 = 36
37 = 37
38 = 38
39 = 39
3a = 3a
3b = 3b
3c = 3c
3d = 3d
3e = 3e
3f = 3f
Size 64 word_count 16
Myword 16 == 0x03020100
Myword 15 == 0x07060504
Myword 14 == 0x0b0a0908
Myword 13 == 0x0f0e0d0c
Myword 12 == 0x13121110
Myword 11 == 0x17161514
Myword 10 == 0x1b1a1918
Myword 9 == 0x1f1e1d1c
Myword 8 == 0x23222120
Myword 7 == 0x27262524
Myword 6 == 0x2b2a2928
Myword 5 == 0x2f2e2d2c
Myword 4 == 0x33323130
Myword 3 == 0x37363534
Myword 2 == 0x3b3a3938
Myword 1 == 0x3f3e3d3c

---
bod

Re: [PATCH 2/4] media: venus: hfi_parser: avoid OOB access beyond payload word count

[Vikash Garodia]

On 11/5/2024 4:45 PM, Bryan O'Donoghue wrote:
> On 05/11/2024 08:54, Vikash Garodia wrote:
>> words_count denotes the number of words in total payload, while data
>> points to payload of various property within it. When words_count
>> reaches last word, data can access memory beyond the total payload.
>> Avoid this case by not allowing the loop for the last word count.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_parser.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c
>> b/drivers/media/platform/qcom/venus/hfi_parser.c
>> index
>> 27d0172294d5154f4839e8cef172f9a619dfa305..20d9ea3626e9c4468d5f7dbd678743135f027c86 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>> @@ -303,7 +303,7 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst
>> *inst, void *buf,
>>           memset(core->caps, 0, sizeof(core->caps));
>>       }
>>   -    while (words_count) {
>> +    while (words_count > 1) {
>>           data = word + 1;
>>             switch (*word) {
>>
> 
> How is it the right thing to do to _not_ process the last u32 ?
> 
> How does this overrun ? while (words_count) should be fine because it decrements
> at the bottom of the loop...
> 
> assuming your buffer is word aligned obvs
> 
> =>
> 
> #include <stdio.h>
> #include <stdint.h>
> 
> char somebuf[64];
> 
> void init(char *buf, int len)
> {
>         int i;
>         char c = 0;
> 
>         for (i = 0; i < len; i++)
>                 buf[i] = c++;
> }
> 
> int hfi_parser(void *buf, int size)
> {
>         int word_count = size >> 2;
>         uint32_t *my_word = (uint32_t*)buf;
Make this as below and it should lead to OOB
uint32_t *my_word = (uint32_t*)buf + 1

Regards,
Vikash
> 
>         printf("Size %d word_count %d\n", size, word_count);
> 
>         while(word_count) {
>                 printf("Myword %d == 0x%08x\n", word_count, *my_word);
>                 my_word++;
>                 word_count--;
>         }
> }
> 
> int main(int argc, char *argv[])
> {
>         int i;
> 
>         init(somebuf, sizeof(somebuf));
>         for (i = 0; i < sizeof(somebuf); i++)
>                 printf("%x = %x\n", i, somebuf[i]);
> 
>         hfi_parser(somebuf, sizeof(somebuf));
> 
>         return 0;
> }
> 
> 0 = 0
> 1 = 1
> 2 = 2
> 3 = 3
> 4 = 4
> 5 = 5
> 6 = 6
> 7 = 7
> 8 = 8
> 9 = 9
> a = a
> b = b
> c = c
> d = d
> e = e
> f = f
> 10 = 10
> 11 = 11
> 12 = 12
> 13 = 13
> 14 = 14
> 15 = 15
> 16 = 16
> 17 = 17
> 18 = 18
> 19 = 19
> 1a = 1a
> 1b = 1b
> 1c = 1c
> 1d = 1d
> 1e = 1e
> 1f = 1f
> 20 = 20
> 21 = 21
> 22 = 22
> 23 = 23
> 24 = 24
> 25 = 25
> 26 = 26
> 27 = 27
> 28 = 28
> 29 = 29
> 2a = 2a
> 2b = 2b
> 2c = 2c
> 2d = 2d
> 2e = 2e
> 2f = 2f
> 30 = 30
> 31 = 31
> 32 = 32
> 33 = 33
> 34 = 34
> 35 = 35
> 36 = 36
> 37 = 37
> 38 = 38
> 39 = 39
> 3a = 3a
> 3b = 3b
> 3c = 3c
> 3d = 3d
> 3e = 3e
> 3f = 3f
> Size 64 word_count 16
> Myword 16 == 0x03020100
> Myword 15 == 0x07060504
> Myword 14 == 0x0b0a0908
> Myword 13 == 0x0f0e0d0c
> Myword 12 == 0x13121110
> Myword 11 == 0x17161514
> Myword 10 == 0x1b1a1918
> Myword 9 == 0x1f1e1d1c
> Myword 8 == 0x23222120
> Myword 7 == 0x27262524
> Myword 6 == 0x2b2a2928
> Myword 5 == 0x2f2e2d2c
> Myword 4 == 0x33323130
> Myword 3 == 0x37363534
> Myword 2 == 0x3b3a3938
> Myword 1 == 0x3f3e3d3c
> 
> ---
> bod

Re: [PATCH 2/4] media: venus: hfi_parser: avoid OOB access beyond payload word count

[Bryan O'Donoghue]

On 11/11/2024 14:36, Vikash Garodia wrote:
>> int hfi_parser(void *buf, int size)
>> {
>>          int word_count = size >> 2;
>>          uint32_t*my_word = (uint32_t*)buf;
> Make this as below and it should lead to OOB
> uint32_t*my_word = (uint32_t*)buf + 1
> 
> Regards,
> Vikash

How does this code make sense ?


         while (words_count) {
                 data = word + 1;

                 switch (*word) {
                 case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
                         parse_codecs(core, data);
                         init_codecs(core);
                         break;
                 case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
                         parse_max_sessions(core, data);
                         break;
                 case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
                         parse_codecs_mask(&codecs, &domain, data);
                         break;
                 case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
                         parse_raw_formats(core, codecs, domain, data);
                         break;
                 case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
                         parse_caps(core, codecs, domain, data);
                         break;
                 case HFI_PROPERTY_PARAM_PROFILE_LEVEL_SUPPORTED:
                         parse_profile_level(core, codecs, domain, data);
                         break;
                 case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE_SUPPORTED:
                         parse_alloc_mode(core, codecs, domain, data);
                         break;
                 default:
                         break;
                 }

                 word++;
                 words_count--;
         }


word[] = { 0, 1, 2, 3 };

words_count = 4;

while(words_count);

	data = word + 1;

	switch(*word) {
	case WHATEVER:
		do_something(param, data);
	}

	word++;
	words_count--;
}

// iteration 0
data = 1;
*word = 0;

// iteration 1
data = 2;
*word = 1;

????

How can the step size of word be correct ?

Do we ever actually process more than one pair here ?

#include <stdio.h>
#include <stdint.h>

char somebuf[16];

void init(char *buf, int len)
{
         int i;
         char c = 0;

         for (i = 0; i < len; i++)
                 buf[i] = c++;
}

int hfi_parser(void *buf, int size)
{
         int word_count = size >> 2;
         uint32_t *my_word = (uint32_t*)buf, *data;

         printf("Size %d word_count %d\n", size, word_count);

         while (word_count > 1) {
                 data = my_word + 1;
                 printf("Myword %d == 0x%08x data=0x%08x\n", word_count, 
*my_word, *data);
                 my_word++;
                 word_count--;
         }
}

int main(int argc, char *argv[])
{
         int i;

         init(somebuf, sizeof(somebuf));
         for (i = 0; i < sizeof(somebuf); i++)
                 printf("%x = %x\n", i, somebuf[i]);

         hfi_parser(somebuf, sizeof(somebuf));

         return 0;
}

0 = 0
1 = 1
2 = 2
3 = 3
4 = 4
5 = 5
6 = 6
7 = 7
8 = 8
9 = 9
a = a
b = b
c = c
d = d
e = e
f = f
Size 16 word_count 4
Myword 4 == 0x03020100 data=0x07060504
Myword 3 == 0x07060504 data=0x0b0a0908
Myword 2 == 0x0b0a0908 data=0x0f0e0d0c

---
bod

Re: [PATCH 2/4] media: venus: hfi_parser: avoid OOB access beyond payload word count

[Vikash Garodia]

On 11/12/2024 5:13 AM, Bryan O'Donoghue wrote:
> On 11/11/2024 14:36, Vikash Garodia wrote:
>>> int hfi_parser(void *buf, int size)
>>> {
>>>          int word_count = size >> 2;
>>>          uint32_t*my_word = (uint32_t*)buf;
>> Make this as below and it should lead to OOB
>> uint32_t*my_word = (uint32_t*)buf + 1
>>
>> Regards,
>> Vikash
> 
> How does this code make sense ?
> 
> 
>         while (words_count) {
>                 data = word + 1;
> 
>                 switch (*word) {
>                 case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
>                         parse_codecs(core, data);
>                         init_codecs(core);
>                         break;
>                 case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
>                         parse_max_sessions(core, data);
>                         break;
>                 case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
>                         parse_codecs_mask(&codecs, &domain, data);
>                         break;
>                 case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
>                         parse_raw_formats(core, codecs, domain, data);
>                         break;
>                 case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
>                         parse_caps(core, codecs, domain, data);
>                         break;
>                 case HFI_PROPERTY_PARAM_PROFILE_LEVEL_SUPPORTED:
>                         parse_profile_level(core, codecs, domain, data);
>                         break;
>                 case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE_SUPPORTED:
>                         parse_alloc_mode(core, codecs, domain, data);
>                         break;
>                 default:
>                         break;
>                 }
> 
>                 word++;
>                 words_count--;
>         }
> 
> 
> word[] = { 0, 1, 2, 3 };
> 
> words_count = 4;
> 
> while(words_count);
> 
>     data = word + 1;
> 
>     switch(*word) {
>     case WHATEVER:
>         do_something(param, data);
>     }
> 
>     word++;
>     words_count--;
> }
> 
> // iteration 0
> data = 1;
> *word = 0;
> 
> // iteration 1
> data = 2;
> *word = 1;
> 
> ????
> 
> How can the step size of word be correct ?
> 
> Do we ever actually process more than one pair here ?
> 
> #include <stdio.h>
> #include <stdint.h>
> 
> char somebuf[16];
> 
> void init(char *buf, int len)
> {
>         int i;
>         char c = 0;
> 
>         for (i = 0; i < len; i++)
>                 buf[i] = c++;
> }
> 
> int hfi_parser(void *buf, int size)
> {
>         int word_count = size >> 2;
>         uint32_t *my_word = (uint32_t*)buf, *data;
> 
>         printf("Size %d word_count %d\n", size, word_count);
> 
>         while (word_count > 1) {
>                 data = my_word + 1;
>                 printf("Myword %d == 0x%08x data=0x%08x\n", word_count,
> *my_word, *data);
>                 my_word++;
>                 word_count--;
>         }
> }
> 
> int main(int argc, char *argv[])
> {
>         int i;
> 
>         init(somebuf, sizeof(somebuf));
>         for (i = 0; i < sizeof(somebuf); i++)
>                 printf("%x = %x\n", i, somebuf[i]);
> 
>         hfi_parser(somebuf, sizeof(somebuf));
> 
>         return 0;
> }
> 
> 0 = 0
> 1 = 1
> 2 = 2
> 3 = 3
> 4 = 4
> 5 = 5
> 6 = 6
> 7 = 7
> 8 = 8
> 9 = 9
> a = a
> b = b
> c = c
> d = d
> e = e
> f = f
> Size 16 word_count 4
> Myword 4 == 0x03020100 data=0x07060504
> Myword 3 == 0x07060504 data=0x0b0a0908
> Myword 2 == 0x0b0a0908 data=0x0f0e0d0c
You did not printed the last iteration without the proposed fix. In the last
iteration (Myword 1), it would access the data beyond allocated size of somebuf.
So we can see how the fix protects from OOB situation.

For the functionality part, packet from firmware would come as <prop type>
followed by <payload for that prop> i.e
*word = HFI_PROPERTY_PARAM_CODEC_SUPPORTED
*data = payload --> hence here data is pointed to next u32 to point and parse
payload for HFI_PROPERTY_PARAM_CODEC_SUPPORTED.
likewise for other properties in the same packet

Regards
Vikash

Re: [PATCH 2/4] media: venus: hfi_parser: avoid OOB access beyond payload word count

[Bryan O'Donoghue]

On 12/11/2024 08:05, Vikash Garodia wrote:
> You did not printed the last iteration without the proposed fix. In the last
> iteration (Myword 1), it would access the data beyond allocated size of somebuf.
> So we can see how the fix protects from OOB situation.

Right but the loop _can't_ be correct. What's the point in fixing an OOB 
in a loop that doesn't work ?

This is the loop:

#define BUF_SIZE 0x20  // BUF_SIZE doesn't really matter

char somebuf[BUF_SIZE];
u32 *word = somebuf[0];
u32 words = ARRAY_SIZE(somebuf);

while (words > 1) {
     data = word + 1;  // this
     word++;           // and this
     words--;
}

On the first loop
word = somebuf[0];
data = somebuf[3];

On the second loop
word = somebuf[3]; // the same value as *data in the previous loop

and that's just broken because on the second loop *word == *data in the 
first loop !

That's what my program showed you

word 4 == 0x03020100 data=0x07060504

// word == data from previous loop
word 3 == 0x07060504 data=0x0b0a0908

// word == data from previous loop
word 2 == 0x0b0a0908 data=0x0f0e0d0c

The step size, the number of bytes this loop increments is fundamentally 
wrong because

a) Its a fixed size [1]
b) *word in loop(n+1) == *data in loop(n)

Which cannot ever parse more than one data item - in effect never loop - 
in one go.

> For the functionality part, packet from firmware would come as <prop type>
> followed by <payload for that prop> i.e
> *word = HFI_PROPERTY_PARAM_CODEC_SUPPORTED
> *data = payload --> hence here data is pointed to next u32 to point and parse
> payload for HFI_PROPERTY_PARAM_CODEC_SUPPORTED.
> likewise for other properties in the same packet

[1]

But we've established that word increments by one word.
We wouldn't fix this loop by just making it into

while (words > 1) {
     data = word + 1;
     word = data + 1;
     words -= 2;
}

Because the consumers of the data have different step sizes, different 
number of bytes they consume for the structs they cast.

=>

case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
	parse_codecs(core, data);
	// consumes sizeof(struct hfi_codec_supported)
	struct hfi_codec_supported {
		u32 dec_codecs;
		u32 enc_codecs;
	};


case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
	parse_max_sessions(core, data);
	// consumes sizeof(struct hfi_max_sessions_supported)
	struct hfi_max_sessions_supported {
		u32 max_sessions;
	};

case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
	parse_codecs_mask(&codecs, &domain, data);
	// consumes sizeof(struct hfi_codec_mask_supported)
	struct hfi_codec_mask_supported {
         	u32 codecs;
	        u32 video_domains;
	};

case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
	parse_raw_formats(core, codecs, domain, data);
	// consumes sizeof(struct hfi_uncompressed_format_supported)
	struct hfi_uncompressed_format_supported {
		u32 buffer_type;
		u32 format_entries;
		struct hfi_uncompressed_plane_info plane_info;
	};

case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
	parse_caps(core, codecs, domain, data);
	
	struct hfi_capabilities {
		u32 num_capabilities;
		struct hfi_capability data[];
	};

	where
	hfi_platform.h:#define MAX_CAP_ENTRIES		32

I'll stop there.

This routine needs a rewrite.

---
bod

Re: [PATCH 2/4] media: venus: hfi_parser: avoid OOB access beyond payload word count

[Vikash Garodia]

On 11/12/2024 4:47 PM, Bryan O'Donoghue wrote:
> On 12/11/2024 08:05, Vikash Garodia wrote:
>> You did not printed the last iteration without the proposed fix. In the last
>> iteration (Myword 1), it would access the data beyond allocated size of somebuf.
>> So we can see how the fix protects from OOB situation.
> 
> Right but the loop _can't_ be correct. What's the point in fixing an OOB in a
> loop that doesn't work ?
> 
> This is the loop:
> 
> #define BUF_SIZE 0x20  // BUF_SIZE doesn't really matter
> 
> char somebuf[BUF_SIZE];
> u32 *word = somebuf[0];
> u32 words = ARRAY_SIZE(somebuf);
> 
> while (words > 1) {
>     data = word + 1;  // this
>     word++;           // and this
>     words--;
> }
> 
> On the first loop
> word = somebuf[0];
> data = somebuf[3];
> 
> On the second loop
> word = somebuf[3]; // the same value as *data in the previous loop
> 
> and that's just broken because on the second loop *word == *data in the first
> loop !
> 
> That's what my program showed you
> 
> word 4 == 0x03020100 data=0x07060504
> 
> // word == data from previous loop
> word 3 == 0x07060504 data=0x0b0a0908
> 
> // word == data from previous loop
> word 2 == 0x0b0a0908 data=0x0f0e0d0c
> 
> The step size, the number of bytes this loop increments is fundamentally wrong
> because
> 
> a) Its a fixed size [1]
> b) *word in loop(n+1) == *data in loop(n)
> 
> Which cannot ever parse more than one data item - in effect never loop - in one go.
In the second iteration, the loop would not match with any case and would try to
match the case by incrementing word. Let say the first word is
"HFI_PROPERTY_PARAM_CODEC_SUPPORTED" followed by 2 words (second and third word)
of payload step size. At this point, now when the loop runs again with second
word and third word, it would not match any case. Again at 4th word, it would
match a case and process the payload.
One thing that we can do here is to increment the word count with the step size
of the data consumed ? This way 2nd and 3rd iteration can be skipped as we know
that there would not be any case in those words.

Regards,
Vikash
> 
>> For the functionality part, packet from firmware would come as <prop type>
>> followed by <payload for that prop> i.e
>> *word = HFI_PROPERTY_PARAM_CODEC_SUPPORTED
>> *data = payload --> hence here data is pointed to next u32 to point and parse
>> payload for HFI_PROPERTY_PARAM_CODEC_SUPPORTED.
>> likewise for other properties in the same packet
> 
> [1]
> 
> But we've established that word increments by one word.
> We wouldn't fix this loop by just making it into
> 
> while (words > 1) {
>     data = word + 1;
>     word = data + 1;
>     words -= 2;
> }
> 
> Because the consumers of the data have different step sizes, different number of
> bytes they consume for the structs they cast.
> 
> =>
> 
> case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
>     parse_codecs(core, data);
>     // consumes sizeof(struct hfi_codec_supported)
>     struct hfi_codec_supported {
>         u32 dec_codecs;
>         u32 enc_codecs;
>     };
> 
> 
> case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
>     parse_max_sessions(core, data);
>     // consumes sizeof(struct hfi_max_sessions_supported)
>     struct hfi_max_sessions_supported {
>         u32 max_sessions;
>     };
> 
> case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
>     parse_codecs_mask(&codecs, &domain, data);
>     // consumes sizeof(struct hfi_codec_mask_supported)
>     struct hfi_codec_mask_supported {
>             u32 codecs;
>             u32 video_domains;
>     };
> 
> case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
>     parse_raw_formats(core, codecs, domain, data);
>     // consumes sizeof(struct hfi_uncompressed_format_supported)
>     struct hfi_uncompressed_format_supported {
>         u32 buffer_type;
>         u32 format_entries;
>         struct hfi_uncompressed_plane_info plane_info;
>     };
> 
> case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
>     parse_caps(core, codecs, domain, data);
>     
>     struct hfi_capabilities {
>         u32 num_capabilities;
>         struct hfi_capability data[];
>     };
> 
>     where
>     hfi_platform.h:#define MAX_CAP_ENTRIES        32
> 
> I'll stop there.
> 
> This routine needs a rewrite.
> 
> ---
> bod

Re: [PATCH 2/4] media: venus: hfi_parser: avoid OOB access beyond payload word count

[Bryan O'Donoghue]

On 12/11/2024 12:58, Vikash Garodia wrote:
> One thing that we can do here is to increment the word count with the step size
> of the data consumed ?

I think that's the right thing to do.

---
bod

[PATCH 3/4] media: venus: hfi: add check to handle incorrect queue size

[Vikash Garodia]

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();
 

-- 
2.34.1

Re: [PATCH 3/4] media: venus: hfi: add check to handle incorrect queue size

[Bryan O'Donoghue]

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>

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

[Vikash Garodia]

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;
+
+	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 4/4] media: venus: hfi: add a check to handle OOB in sfr region

[Bryan O'Donoghue]

On 05/11/2024 08:54, 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;
> +
> +	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);
>   }
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

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

[Dmitry Baryshkov]

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?

> +
> +	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
> 

-- 
With best wishes
Dmitry

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

[Vikash Garodia]

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);
>>  	/*
>>  	 * 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
>>
>