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

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

[Vikash Garodia]

v1 -> v2:
- Address the comment to reduce size of queue pointer from queue size
- Consider the data size during memcpy to avoid OOB write
- Use hweight_long() to count the setbits representing the supported codecs

v1: https://lore.kernel.org/all/1690432469-14803-1-git-send-email-quic_vgarodia@quicinc.com/

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.

Vikash Garodia (4):
  venus: hfi: add checks to perform sanity on queue pointers
  venus: hfi: fix the check to handle session buffer requirement
  venus: hfi: add checks to handle capabilities from firmware
  venus: hfi_parser: Add check to keep the number of codecs within range

 drivers/media/platform/qcom/venus/hfi_msgs.c   |  2 +-
 drivers/media/platform/qcom/venus/hfi_parser.c | 15 +++++++++++++++
 drivers/media/platform/qcom/venus/hfi_venus.c  | 10 ++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH v2 1/4] venus: hfi: add checks to perform sanity on queue pointers

[Vikash Garodia]

Read and write pointers are used to track the packet index in the memory
shared between video driver and firmware. There is a possibility of OOB
access if the read or write pointer goes beyond the queue memory size.
Add checks for the read and write pointer to avoid OOB access.

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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index f0b4638..4ddabb1 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -206,6 +206,11 @@ static int venus_write_queue(struct venus_hfi_device *hdev,
 
 	new_wr_idx = wr_idx + dwords;
 	wr_ptr = (u32 *)(queue->qmem.kva + (wr_idx << 2));
+
+	if (wr_ptr < (u32 *)queue->qmem.kva ||
+	    wr_ptr > (u32 *)(queue->qmem.kva + queue->qmem.size - sizeof(*wr_ptr)))
+		return -EINVAL;
+
 	if (new_wr_idx < qsize) {
 		memcpy(wr_ptr, packet, dwords << 2);
 	} else {
@@ -273,6 +278,11 @@ static int venus_read_queue(struct venus_hfi_device *hdev,
 	}
 
 	rd_ptr = (u32 *)(queue->qmem.kva + (rd_idx << 2));
+
+	if (rd_ptr < (u32 *)queue->qmem.kva ||
+	    rd_ptr > (u32 *)(queue->qmem.kva + queue->qmem.size - sizeof(*rd_ptr)))
+		return -EINVAL;
+
 	dwords = *rd_ptr >> 2;
 	if (!dwords)
 		return -EINVAL;
-- 
2.7.4

[PATCH v2 2/4] venus: hfi: fix the check to handle session buffer requirement

[Vikash Garodia]

Buffer requirement, for different buffer type, comes from video firmware.
While copying these requirements, there is an OOB possibility when the
payload from firmware is more than expected size. Fix the check to avoid
the OOB possibility.

Cc: stable@vger.kernel.org
Fixes: 09c2845e8fe4 ("[media] media: venus: hfi: add Host Firmware Interface (HFI)")
Reviewed-by: Nathan Hebert <nhebert@chromium.org>
Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
 drivers/media/platform/qcom/venus/hfi_msgs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
index 3d5dadf..3e85bd8 100644
--- a/drivers/media/platform/qcom/venus/hfi_msgs.c
+++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
@@ -398,7 +398,7 @@ session_get_prop_buf_req(struct hfi_msg_session_property_info_pkt *pkt,
 		memcpy(&bufreq[idx], buf_req, sizeof(*bufreq));
 		idx++;
 
-		if (idx > HFI_BUFFER_TYPE_MAX)
+		if (idx >= HFI_BUFFER_TYPE_MAX)
 			return HFI_ERR_SESSION_INVALID_PARAMETER;
 
 		req_bytes -= sizeof(struct hfi_buffer_requirements);
-- 
2.7.4

[PATCH v2 3/4] venus: hfi: add checks to handle capabilities from firmware

[Vikash Garodia]

The hfi parser, parses the capabilities received from venus firmware and
copies them to core capabilities. Consider below api, for example,
fill_caps - In this api, caps in core structure gets updated with the
number of capabilities received in firmware data payload. If the same api
is called multiple times, there is a possibility of copying beyond the max
allocated size in core caps.
Similar possibilities in fill_raw_fmts and fill_profile_level functions.

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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
index 6cf74b2..9d6ba22 100644
--- a/drivers/media/platform/qcom/venus/hfi_parser.c
+++ b/drivers/media/platform/qcom/venus/hfi_parser.c
@@ -86,6 +86,9 @@ static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
 {
 	const struct hfi_profile_level *pl = data;
 
+	if (cap->num_pl + num >= HFI_MAX_PROFILE_COUNT)
+		return;
+
 	memcpy(&cap->pl[cap->num_pl], pl, num * sizeof(*pl));
 	cap->num_pl += num;
 }
@@ -111,6 +114,9 @@ fill_caps(struct hfi_plat_caps *cap, const void *data, unsigned int num)
 {
 	const struct hfi_capability *caps = data;
 
+	if (cap->num_caps + num >= MAX_CAP_ENTRIES)
+		return;
+
 	memcpy(&cap->caps[cap->num_caps], caps, num * sizeof(*caps));
 	cap->num_caps += num;
 }
@@ -137,6 +143,9 @@ static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
 {
 	const struct raw_formats *formats = fmts;
 
+	if (cap->num_fmts + num_fmts >= MAX_FMT_ENTRIES)
+		return;
+
 	memcpy(&cap->fmts[cap->num_fmts], formats, num_fmts * sizeof(*formats));
 	cap->num_fmts += num_fmts;
 }
@@ -159,6 +168,9 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
 		rawfmts[i].buftype = fmt->buffer_type;
 		i++;
 
+		if (i >= MAX_FMT_ENTRIES)
+			return;
+
 		if (pinfo->num_planes > MAX_PLANES)
 			break;
 
-- 
2.7.4

[PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Vikash Garodia]

Supported codec bitmask is populated from the payload from venus firmware.
There is a possible case when all the bits in the codec bitmask is set. In
such case, core cap for decoder is filled  and MAX_CODEC_NUM is utilized.
Now while filling the caps for encoder, it can lead to access the caps
array beyong 32 index. Hence leading to OOB write.
The fix counts the supported encoder and decoder. If the count is more than
max, then it skips accessing the caps.

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
index 9d6ba22..c438395 100644
--- a/drivers/media/platform/qcom/venus/hfi_parser.c
+++ b/drivers/media/platform/qcom/venus/hfi_parser.c
@@ -19,6 +19,9 @@ static void init_codecs(struct venus_core *core)
 	struct hfi_plat_caps *caps = core->caps, *cap;
 	unsigned long bit;
 
+	if (hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) > MAX_CODEC_NUM)
+		return;
+
 	for_each_set_bit(bit, &core->dec_codecs, MAX_CODEC_NUM) {
 		cap = &caps[core->codecs_count++];
 		cap->codec = BIT(bit);
-- 
2.7.4

Re: [PATCH v2 1/4] venus: hfi: add checks to perform sanity on queue pointers

[Bryan O'Donoghue]

On 10/08/2023 03:25, Vikash Garodia wrote:
> Read and write pointers are used to track the packet index in the memory
> shared between video driver and firmware. There is a possibility of OOB
> access if the read or write pointer goes beyond the queue memory size.
> Add checks for the read and write pointer to avoid OOB access.
> 
> 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 | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index f0b4638..4ddabb1 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -206,6 +206,11 @@ static int venus_write_queue(struct venus_hfi_device *hdev,
>   
>   	new_wr_idx = wr_idx + dwords;
>   	wr_ptr = (u32 *)(queue->qmem.kva + (wr_idx << 2));
> +
> +	if (wr_ptr < (u32 *)queue->qmem.kva ||
> +	    wr_ptr > (u32 *)(queue->qmem.kva + queue->qmem.size - sizeof(*wr_ptr)))
> +		return -EINVAL;
> +
>   	if (new_wr_idx < qsize) {
>   		memcpy(wr_ptr, packet, dwords << 2);
>   	} else {
> @@ -273,6 +278,11 @@ static int venus_read_queue(struct venus_hfi_device *hdev,
>   	}
>   
>   	rd_ptr = (u32 *)(queue->qmem.kva + (rd_idx << 2));
> +
> +	if (rd_ptr < (u32 *)queue->qmem.kva ||
> +	    rd_ptr > (u32 *)(queue->qmem.kva + queue->qmem.size - sizeof(*rd_ptr)))
> +		return -EINVAL;
> +
>   	dwords = *rd_ptr >> 2;
>   	if (!dwords)
>   		return -EINVAL;

What is the bit-shifting for ?

Re: [PATCH v2 2/4] venus: hfi: fix the check to handle session buffer requirement

[Bryan O'Donoghue]

On 10/08/2023 03:25, Vikash Garodia wrote:
> Buffer requirement, for different buffer type, comes from video firmware.
> While copying these requirements, there is an OOB possibility when the
> payload from firmware is more than expected size. Fix the check to avoid
> the OOB possibility.
> 
> Cc: stable@vger.kernel.org
> Fixes: 09c2845e8fe4 ("[media] media: venus: hfi: add Host Firmware Interface (HFI)")
> Reviewed-by: Nathan Hebert <nhebert@chromium.org>
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>   drivers/media/platform/qcom/venus/hfi_msgs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
> index 3d5dadf..3e85bd8 100644
> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
> @@ -398,7 +398,7 @@ session_get_prop_buf_req(struct hfi_msg_session_property_info_pkt *pkt,
>   		memcpy(&bufreq[idx], buf_req, sizeof(*bufreq));
>   		idx++;
>   
> -		if (idx > HFI_BUFFER_TYPE_MAX)
> +		if (idx >= HFI_BUFFER_TYPE_MAX)
>   			return HFI_ERR_SESSION_INVALID_PARAMETER;
>   
>   		req_bytes -= sizeof(struct hfi_buffer_requirements);

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

Re: [PATCH v2 3/4] venus: hfi: add checks to handle capabilities from firmware

[Bryan O'Donoghue]

On 10/08/2023 03:25, Vikash Garodia wrote:
> The hfi parser, parses the capabilities received from venus firmware and
> copies them to core capabilities. Consider below api, for example,
> fill_caps - In this api, caps in core structure gets updated with the
> number of capabilities received in firmware data payload. If the same api
> is called multiple times, there is a possibility of copying beyond the max
> allocated size in core caps.
> Similar possibilities in fill_raw_fmts and fill_profile_level functions.
> 
> 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 | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 6cf74b2..9d6ba22 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -86,6 +86,9 @@ static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
>   {
>   	const struct hfi_profile_level *pl = data;
>   
> +	if (cap->num_pl + num >= HFI_MAX_PROFILE_COUNT)
> +		return;
> +
>   	memcpy(&cap->pl[cap->num_pl], pl, num * sizeof(*pl));
>   	cap->num_pl += num;
>   }

Why append and discard though ?

Couldn't we reset/reinitalise the relevant indexes in hfi_sys_init_done() ?

Can subsequent notifications from the firmware give a new capability set 
? Presumably not.

IMO though instead of throwing away the new data, we should throw away 
the old data, no ?

---
bod

Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Bryan O'Donoghue]

On 10/08/2023 03:25, Vikash Garodia wrote:
> +	if (hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) > MAX_CODEC_NUM)
> +		return;
> +

Shouldn't this be >= ?

struct hfi_plat_caps caps[MAX_CODEC_NUM];

---
bod

Re: [PATCH v2 1/4] venus: hfi: add checks to perform sanity on queue pointers

[Vikash Garodia]

On 8/10/2023 4:54 PM, Bryan O'Donoghue wrote:
> On 10/08/2023 03:25, Vikash Garodia wrote:
>> Read and write pointers are used to track the packet index in the memory
>> shared between video driver and firmware. There is a possibility of OOB
>> access if the read or write pointer goes beyond the queue memory size.
>> Add checks for the read and write pointer to avoid OOB access.
>>
>> 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 | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c
>> b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index f0b4638..4ddabb1 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -206,6 +206,11 @@ static int venus_write_queue(struct venus_hfi_device *hdev,
>>         new_wr_idx = wr_idx + dwords;
>>       wr_ptr = (u32 *)(queue->qmem.kva + (wr_idx << 2));
>> +
>> +    if (wr_ptr < (u32 *)queue->qmem.kva ||
>> +        wr_ptr > (u32 *)(queue->qmem.kva + queue->qmem.size - sizeof(*wr_ptr)))
>> +        return -EINVAL;
>> +
>>       if (new_wr_idx < qsize) {
>>           memcpy(wr_ptr, packet, dwords << 2);
>>       } else {
>> @@ -273,6 +278,11 @@ static int venus_read_queue(struct venus_hfi_device *hdev,
>>       }
>>         rd_ptr = (u32 *)(queue->qmem.kva + (rd_idx << 2));
>> +
>> +    if (rd_ptr < (u32 *)queue->qmem.kva ||
>> +        rd_ptr > (u32 *)(queue->qmem.kva + queue->qmem.size - sizeof(*rd_ptr)))
>> +        return -EINVAL;
>> +
>>       dwords = *rd_ptr >> 2;
>>       if (!dwords)
>>           return -EINVAL;
> 
> What is the bit-shifting for ?
If the query is related to dwords, the bit shift is to convert the size of
packet in words.

Regards,
Vikash

Re: [PATCH v2 3/4] venus: hfi: add checks to handle capabilities from firmware

[Vikash Garodia]

On 8/10/2023 5:01 PM, Bryan O'Donoghue wrote:
> On 10/08/2023 03:25, Vikash Garodia wrote:
>> The hfi parser, parses the capabilities received from venus firmware and
>> copies them to core capabilities. Consider below api, for example,
>> fill_caps - In this api, caps in core structure gets updated with the
>> number of capabilities received in firmware data payload. If the same api
>> is called multiple times, there is a possibility of copying beyond the max
>> allocated size in core caps.
>> Similar possibilities in fill_raw_fmts and fill_profile_level functions.
>>
>> 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 | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c
>> b/drivers/media/platform/qcom/venus/hfi_parser.c
>> index 6cf74b2..9d6ba22 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>> @@ -86,6 +86,9 @@ static void fill_profile_level(struct hfi_plat_caps *cap,
>> const void *data,
>>   {
>>       const struct hfi_profile_level *pl = data;
>>   +    if (cap->num_pl + num >= HFI_MAX_PROFILE_COUNT)
>> +        return;
>> +
>>       memcpy(&cap->pl[cap->num_pl], pl, num * sizeof(*pl));
>>       cap->num_pl += num;
>>   }
> 
> Why append and discard though ?
> 
> Couldn't we reset/reinitalise the relevant indexes in hfi_sys_init_done() ?
> 
> Can subsequent notifications from the firmware give a new capability set ?
> Presumably not.
> 
> IMO though instead of throwing away the new data, we should throw away the old
> data, no ?
The case is all about rogue firmware. If there is a need to fill the same cap
again, that itself indicates that the payload from firmware is not correct. In
such cases, the old as well as new cap data are not reliable. Though the
authenticity of the data cannot be ensured, the check would avoid any OOB during
such rogue firmware case.

Regards,
Vikash

> 
> ---
> bod

Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Vikash Garodia]

On 8/10/2023 5:03 PM, Bryan O'Donoghue wrote:
> On 10/08/2023 03:25, Vikash Garodia wrote:
>> +    if (hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) >
>> MAX_CODEC_NUM)
>> +        return;
>> +
> 
> Shouldn't this be >= ?
Not needed. Lets take a hypothetical case when core->dec_codecs has initial 16
(0-15) bits set and core->enc_codecs has next 16 bits (16-31) set. The bit count
would be 32. The codec loop after this check would run on caps array index 0-31.
I do not see a possibility for OOB access in this case.

> 
> struct hfi_plat_caps caps[MAX_CODEC_NUM];
> 
> ---
> bod
> 

Re: [PATCH v2 3/4] venus: hfi: add checks to handle capabilities from firmware

[Bryan O'Donoghue]

On 11/08/2023 06:54, Vikash Garodia wrote:
> The case is all about rogue firmware. If there is a need to fill the same cap
> again, that itself indicates that the payload from firmware is not correct. In
> such cases, the old as well as new cap data are not reliable. Though the
> authenticity of the data cannot be ensured, the check would avoid any OOB during
> such rogue firmware case.

Then why favour the old cap report over the new ?

Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Bryan O'Donoghue]

On 11/08/2023 07:04, Vikash Garodia wrote:
> 
> On 8/10/2023 5:03 PM, Bryan O'Donoghue wrote:
>> On 10/08/2023 03:25, Vikash Garodia wrote:
>>> +    if (hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) >
>>> MAX_CODEC_NUM)
>>> +        return;
>>> +
>>
>> Shouldn't this be >= ?
> Not needed. Lets take a hypothetical case when core->dec_codecs has initial 16
> (0-15) bits set and core->enc_codecs has next 16 bits (16-31) set. The bit count
> would be 32. The codec loop after this check would run on caps array index 0-31.
> I do not see a possibility for OOB access in this case.
> 
>>
>> struct hfi_plat_caps caps[MAX_CODEC_NUM];
>>
>> ---
>> bod
>>

Are you not doing a general defensive coding pass in this series ie

"[PATCH v2 2/4] venus: hfi: fix the check to handle session buffer 
requirement"

---
bod

Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Vikash Garodia]

On 8/11/2023 2:12 PM, Bryan O'Donoghue wrote:
> On 11/08/2023 07:04, Vikash Garodia wrote:
>>
>> On 8/10/2023 5:03 PM, Bryan O'Donoghue wrote:
>>> On 10/08/2023 03:25, Vikash Garodia wrote:
>>>> +    if (hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) >
>>>> MAX_CODEC_NUM)
>>>> +        return;
>>>> +
>>>
>>> Shouldn't this be >= ?
>> Not needed. Lets take a hypothetical case when core->dec_codecs has initial 16
>> (0-15) bits set and core->enc_codecs has next 16 bits (16-31) set. The bit count
>> would be 32. The codec loop after this check would run on caps array index 0-31.
>> I do not see a possibility for OOB access in this case.
>>
>>>
>>> struct hfi_plat_caps caps[MAX_CODEC_NUM];
>>>
>>> ---
>>> bod
>>>
> 
> Are you not doing a general defensive coding pass in this series ie
> 
> "[PATCH v2 2/4] venus: hfi: fix the check to handle session buffer requirement"

In "PATCH v2 2/4", there is a possibility if the check does not consider "=".
Here in this patch, I do not see a possibility.

> 
> ---
> bod

Re: [PATCH v2 3/4] venus: hfi: add checks to handle capabilities from firmware

[Vikash Garodia]

On 8/11/2023 2:11 PM, Bryan O'Donoghue wrote:
> On 11/08/2023 06:54, Vikash Garodia wrote:
>> The case is all about rogue firmware. If there is a need to fill the same cap
>> again, that itself indicates that the payload from firmware is not correct. In
>> such cases, the old as well as new cap data are not reliable. Though the
>> authenticity of the data cannot be ensured, the check would avoid any OOB during
>> such rogue firmware case.
> 
> Then why favour the old cap report over the new ?

When the driver hits the case for OOB, thats when it knows that something has
gone wrong. Keeping old or new, both are invalid values in such case, nothing to
favor any value.

Regards,
Vikash

Re: [PATCH v2 3/4] venus: hfi: add checks to handle capabilities from firmware

[Bryan O'Donoghue]

On 11/08/2023 09:51, Vikash Garodia wrote:
> 
> On 8/11/2023 2:11 PM, Bryan O'Donoghue wrote:
>> On 11/08/2023 06:54, Vikash Garodia wrote:
>>> The case is all about rogue firmware. If there is a need to fill the same cap
>>> again, that itself indicates that the payload from firmware is not correct. In
>>> such cases, the old as well as new cap data are not reliable. Though the
>>> authenticity of the data cannot be ensured, the check would avoid any OOB during
>>> such rogue firmware case.
>>
>> Then why favour the old cap report over the new ?
> 
> When the driver hits the case for OOB, thats when it knows that something has
> gone wrong. Keeping old or new, both are invalid values in such case, nothing to
> favor any value.
> 
> Regards,
> Vikash

Is this hypothetical or a real bug you are actually working to mitigate ?

---
bod

Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Bryan O'Donoghue]

On 11/08/2023 09:49, Vikash Garodia wrote:
> 
> On 8/11/2023 2:12 PM, Bryan O'Donoghue wrote:
>> On 11/08/2023 07:04, Vikash Garodia wrote:
>>>
>>> On 8/10/2023 5:03 PM, Bryan O'Donoghue wrote:
>>>> On 10/08/2023 03:25, Vikash Garodia wrote:
>>>>> +    if (hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) >
>>>>> MAX_CODEC_NUM)
>>>>> +        return;
>>>>> +
>>>>
>>>> Shouldn't this be >= ?
>>> Not needed. Lets take a hypothetical case when core->dec_codecs has initial 16
>>> (0-15) bits set and core->enc_codecs has next 16 bits (16-31) set. The bit count
>>> would be 32. The codec loop after this check would run on caps array index 0-31.
>>> I do not see a possibility for OOB access in this case.
>>>
>>>>
>>>> struct hfi_plat_caps caps[MAX_CODEC_NUM];
>>>>
>>>> ---
>>>> bod
>>>>
>>
>> Are you not doing a general defensive coding pass in this series ie
>>
>> "[PATCH v2 2/4] venus: hfi: fix the check to handle session buffer requirement"
> 
> In "PATCH v2 2/4", there is a possibility if the check does not consider "=".
> Here in this patch, I do not see a possibility.
> 
>>
>> ---
>> bod

But surely hweight_long(core->dec_codecs) + 
hweight_long(core->enc_codecs) == MAX_CODEC_NUM is an invalid offset ?

---
bod

Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Vikash Garodia]

On 8/11/2023 4:11 PM, Bryan O'Donoghue wrote:
> On 11/08/2023 09:49, Vikash Garodia wrote:
>>
>> On 8/11/2023 2:12 PM, Bryan O'Donoghue wrote:
>>> On 11/08/2023 07:04, Vikash Garodia wrote:
>>>>
>>>> On 8/10/2023 5:03 PM, Bryan O'Donoghue wrote:
>>>>> On 10/08/2023 03:25, Vikash Garodia wrote:
>>>>>> +    if (hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) >
>>>>>> MAX_CODEC_NUM)
>>>>>> +        return;
>>>>>> +
>>>>>
>>>>> Shouldn't this be >= ?
>>>> Not needed. Lets take a hypothetical case when core->dec_codecs has initial 16
>>>> (0-15) bits set and core->enc_codecs has next 16 bits (16-31) set. The bit
>>>> count
>>>> would be 32. The codec loop after this check would run on caps array index
>>>> 0-31.
>>>> I do not see a possibility for OOB access in this case.
>>>>
>>>>>
>>>>> struct hfi_plat_caps caps[MAX_CODEC_NUM];
>>>>>
>>>>> ---
>>>>> bod
>>>>>
>>>
>>> Are you not doing a general defensive coding pass in this series ie
>>>
>>> "[PATCH v2 2/4] venus: hfi: fix the check to handle session buffer requirement"
>>
>> In "PATCH v2 2/4", there is a possibility if the check does not consider "=".
>> Here in this patch, I do not see a possibility.
>>
>>>
>>> ---
>>> bod
> 
> But surely hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) ==
> MAX_CODEC_NUM is an invalid offset ?

No, it isn't. Please run through the loop with the bitmasks added upto 32 and
see if there is a possibility of OOB.

> 
> ---
> bod

Re: [PATCH v2 3/4] venus: hfi: add checks to handle capabilities from firmware

[Vikash Garodia]

On 8/11/2023 4:09 PM, Bryan O'Donoghue wrote:
> On 11/08/2023 09:51, Vikash Garodia wrote:
>>
>> On 8/11/2023 2:11 PM, Bryan O'Donoghue wrote:
>>> On 11/08/2023 06:54, Vikash Garodia wrote:
>>>> The case is all about rogue firmware. If there is a need to fill the same cap
>>>> again, that itself indicates that the payload from firmware is not correct. In
>>>> such cases, the old as well as new cap data are not reliable. Though the
>>>> authenticity of the data cannot be ensured, the check would avoid any OOB
>>>> during
>>>> such rogue firmware case.
>>>
>>> Then why favour the old cap report over the new ?
>>
>> When the driver hits the case for OOB, thats when it knows that something has
>> gone wrong. Keeping old or new, both are invalid values in such case, nothing to
>> favor any value.
>>
>> Regards,
>> Vikash
> 
> Is this hypothetical or a real bug you are actually working to mitigate ?

These are theoretical bugs, not reported during any video usecase so far. At the
same time, these are quite possible when the packets from firmware goes
different than expected.

> ---
> bod

Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Bryan O'Donoghue]

On 11/08/2023 17:02, Vikash Garodia wrote:
> 
> 
> On 8/11/2023 4:11 PM, Bryan O'Donoghue wrote:
>> On 11/08/2023 09:49, Vikash Garodia wrote:
>>>
>>> On 8/11/2023 2:12 PM, Bryan O'Donoghue wrote:
>>>> On 11/08/2023 07:04, Vikash Garodia wrote:
>>>>>
>>>>> On 8/10/2023 5:03 PM, Bryan O'Donoghue wrote:
>>>>>> On 10/08/2023 03:25, Vikash Garodia wrote:
>>>>>>> +    if (hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) >
>>>>>>> MAX_CODEC_NUM)
>>>>>>> +        return;
>>>>>>> +
>>>>>>
>>>>>> Shouldn't this be >= ?
>>>>> Not needed. Lets take a hypothetical case when core->dec_codecs has initial 16
>>>>> (0-15) bits set and core->enc_codecs has next 16 bits (16-31) set. The bit
>>>>> count
>>>>> would be 32. The codec loop after this check would run on caps array index
>>>>> 0-31.
>>>>> I do not see a possibility for OOB access in this case.
>>>>>
>>>>>>
>>>>>> struct hfi_plat_caps caps[MAX_CODEC_NUM];
>>>>>>
>>>>>> ---
>>>>>> bod
>>>>>>
>>>>
>>>> Are you not doing a general defensive coding pass in this series ie
>>>>
>>>> "[PATCH v2 2/4] venus: hfi: fix the check to handle session buffer requirement"
>>>
>>> In "PATCH v2 2/4", there is a possibility if the check does not consider "=".
>>> Here in this patch, I do not see a possibility.
>>>
>>>>
>>>> ---
>>>> bod
>>
>> But surely hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) ==
>> MAX_CODEC_NUM is an invalid offset ?
> 
> No, it isn't. Please run through the loop with the bitmasks added upto 32 and
> see if there is a possibility of OOB.

IDK Vikash, the logic here seems suspect.

We have two loops that check for up to 32 indexes per loop. Why not have 
a capabilities index that can accommodate all 64 bits ?

Why is it valid to have 16 encoder bits and 16 decoder bits but invalid 
to have 16 encoder bits with 17 decoder bits ? While at the same time 
valid to have 0 encoder bits but 17 decoder bits ?

---
bod

Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Vikash Garodia]

On 8/12/2023 12:21 AM, Bryan O'Donoghue wrote:
> On 11/08/2023 17:02, Vikash Garodia wrote:
>>
>>
>> On 8/11/2023 4:11 PM, Bryan O'Donoghue wrote:
>>> On 11/08/2023 09:49, Vikash Garodia wrote:
>>>>
>>>> On 8/11/2023 2:12 PM, Bryan O'Donoghue wrote:
>>>>> On 11/08/2023 07:04, Vikash Garodia wrote:
>>>>>>
>>>>>> On 8/10/2023 5:03 PM, Bryan O'Donoghue wrote:
>>>>>>> On 10/08/2023 03:25, Vikash Garodia wrote:
>>>>>>>> +    if (hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) >
>>>>>>>> MAX_CODEC_NUM)
>>>>>>>> +        return;
>>>>>>>> +
>>>>>>>
>>>>>>> Shouldn't this be >= ?
>>>>>> Not needed. Lets take a hypothetical case when core->dec_codecs has
>>>>>> initial 16
>>>>>> (0-15) bits set and core->enc_codecs has next 16 bits (16-31) set. The bit
>>>>>> count
>>>>>> would be 32. The codec loop after this check would run on caps array index
>>>>>> 0-31.
>>>>>> I do not see a possibility for OOB access in this case.
>>>>>>
>>>>>>>
>>>>>>> struct hfi_plat_caps caps[MAX_CODEC_NUM];
>>>>>>>
>>>>>>> ---
>>>>>>> bod
>>>>>>>
>>>>>
>>>>> Are you not doing a general defensive coding pass in this series ie
>>>>>
>>>>> "[PATCH v2 2/4] venus: hfi: fix the check to handle session buffer
>>>>> requirement"
>>>>
>>>> In "PATCH v2 2/4", there is a possibility if the check does not consider "=".
>>>> Here in this patch, I do not see a possibility.
>>>>
>>>>>
>>>>> ---
>>>>> bod
>>>
>>> But surely hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) ==
>>> MAX_CODEC_NUM is an invalid offset ?
>>
>> No, it isn't. Please run through the loop with the bitmasks added upto 32 and
>> see if there is a possibility of OOB.
> 
> IDK Vikash, the logic here seems suspect.
> 
> We have two loops that check for up to 32 indexes per loop. Why not have a
> capabilities index that can accommodate all 64 bits ?
Max codecs supported can be 32, which is also a very high number. At max the
hardware supports 5-6 codecs, including both decoder and encoder. 64 indices is
would not be needed.

> Why is it valid to have 16 encoder bits and 16 decoder bits but invalid to have
> 16 encoder bits with 17 decoder bits ? While at the same time valid to have 0
> encoder bits but 17 decoder bits ?
The addition of the encoder and decoder should be 32. Any combination which adds
to it, would go through. For ex, (17 dec + 15 enc) OR (32 dec + 0 enc) OR (0 dec
+ 32 enc) etc are valid combination theoretically, though there are only few
decoders and encoders actually supported by hardware.

Regards,
Vikash

Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Bryan O'Donoghue]

On 14/08/2023 07:34, Vikash Garodia wrote:
>> We have two loops that check for up to 32 indexes per loop. Why not have a
>> capabilities index that can accommodate all 64 bits ?
> Max codecs supported can be 32, which is also a very high number. At max the
> hardware supports 5-6 codecs, including both decoder and encoder. 64 indices is
> would not be needed.
> 

But the bug you are fixing here is an overflow where we have received a 
full range 32 bit for each decode and encode.

How is the right fix not to extend the storage to the maximum possible 2 
x 32 ? Or indeed why not constrain the input data to 32/2 for each 
encode/decode path ?

The bug here is that we can copy two arrays of size X into one array of 
size X.

Please consider expanding the size of the storage array to accommodate 
the full size the protocol supports 2 x 32.

---
bod

Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Vikash Garodia]

Hi Bryan,

On 8/14/2023 7:45 PM, Bryan O'Donoghue wrote:
> On 14/08/2023 07:34, Vikash Garodia wrote:
>>> We have two loops that check for up to 32 indexes per loop. Why not have a
>>> capabilities index that can accommodate all 64 bits ?
>> Max codecs supported can be 32, which is also a very high number. At max the
>> hardware supports 5-6 codecs, including both decoder and encoder. 64 indices is
>> would not be needed.
>>
> 
> But the bug you are fixing here is an overflow where we have received a full
> range 32 bit for each decode and encode.
> 
> How is the right fix not to extend the storage to the maximum possible 2 x 32 ?
> Or indeed why not constrain the input data to 32/2 for each encode/decode path ?
At this point, we agree that there is very less or no possibility to have this
as a real usecase i.e having 64 (or more than 32) codecs supported in video
hardware. There seem to be no value add if we are extending the cap array from
32 to 64, as anything beyond 32 itself indicates rogue firmware. The idea here
is to gracefully come out of such case when firmware is responding with such
data payload.
Again, lets think of constraining the data to 32/2. We have 2 32 bit masks for
decoder and encoder. Malfunctioning firmware could still send payload with all
bits enabled in those masks. Then the driver needs to add same check to avoid
the memcpy in such case.

> The bug here is that we can copy two arrays of size X into one array of size X.
> 
> Please consider expanding the size of the storage array to accommodate the full
> size the protocol supports 2 x 32.
I see this as an alternate implementation to existing handling. 64 index would
never exist practically, so accommodating it only implies to store the data for
invalid response and gracefully close the session.

Thanks,
Vikash

Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Bryan O'Donoghue]

On 29/08/2023 09:00, Vikash Garodia wrote:
> Hi Bryan,
> 
> On 8/14/2023 7:45 PM, Bryan O'Donoghue wrote:
>> On 14/08/2023 07:34, Vikash Garodia wrote:
>>>> We have two loops that check for up to 32 indexes per loop. Why not have a
>>>> capabilities index that can accommodate all 64 bits ?
>>> Max codecs supported can be 32, which is also a very high number. At max the
>>> hardware supports 5-6 codecs, including both decoder and encoder. 64 indices is
>>> would not be needed.
>>>
>>
>> But the bug you are fixing here is an overflow where we have received a full
>> range 32 bit for each decode and encode.
>>
>> How is the right fix not to extend the storage to the maximum possible 2 x 32 ?
>> Or indeed why not constrain the input data to 32/2 for each encode/decode path ?
> At this point, we agree that there is very less or no possibility to have this
> as a real usecase i.e having 64 (or more than 32) codecs supported in video
> hardware. There seem to be no value add if we are extending the cap array from
> 32 to 64, as anything beyond 32 itself indicates rogue firmware. The idea here
> is to gracefully come out of such case when firmware is responding with such
> data payload.
> Again, lets think of constraining the data to 32/2. We have 2 32 bit masks for
> decoder and encoder. Malfunctioning firmware could still send payload with all
> bits enabled in those masks. Then the driver needs to add same check to avoid
> the memcpy in such case.
> 
>> The bug here is that we can copy two arrays of size X into one array of size X.
>>
>> Please consider expanding the size of the storage array to accommodate the full
>> size the protocol supports 2 x 32.
> I see this as an alternate implementation to existing handling. 64 index would
> never exist practically, so accommodating it only implies to store the data for
> invalid response and gracefully close the session.

What's the contractual definition of "this many bits per encoder and 
decoder" between firmware and APSS in that case ?

Where do we get the idea that 32/2 per encoder/decoder is valid but 32 
per encoder decoder is invalid ?

At this moment in time 16 encoder/decoder bits would be equally invalid.

I suggest the right answer is to buffer the protocol data unit - PDU 
maximum as an RX or constrain the maximum number of encoder/decoder bits 
based on HFI version.

ie.

- Either constrain on the PDU or
- Constrain on the known number of maximum bits per f/w version

---
bod

Re: [PATCH v2 4/4] venus: hfi_parser: Add check to keep the number of codecs within range

[Vikash Garodia]

On 8/29/2023 5:29 PM, Bryan O'Donoghue wrote:
> On 29/08/2023 09:00, Vikash Garodia wrote:
>> Hi Bryan,
>>
>> On 8/14/2023 7:45 PM, Bryan O'Donoghue wrote:
>>> On 14/08/2023 07:34, Vikash Garodia wrote:
>>>>> We have two loops that check for up to 32 indexes per loop. Why not have a
>>>>> capabilities index that can accommodate all 64 bits ?
>>>> Max codecs supported can be 32, which is also a very high number. At max the
>>>> hardware supports 5-6 codecs, including both decoder and encoder. 64 indices is
>>>> would not be needed.
>>>>
>>>
>>> But the bug you are fixing here is an overflow where we have received a full
>>> range 32 bit for each decode and encode.
>>>
>>> How is the right fix not to extend the storage to the maximum possible 2 x 32 ?
>>> Or indeed why not constrain the input data to 32/2 for each encode/decode path ?
>> At this point, we agree that there is very less or no possibility to have this
>> as a real usecase i.e having 64 (or more than 32) codecs supported in video
>> hardware. There seem to be no value add if we are extending the cap array from
>> 32 to 64, as anything beyond 32 itself indicates rogue firmware. The idea here
>> is to gracefully come out of such case when firmware is responding with such
>> data payload.
>> Again, lets think of constraining the data to 32/2. We have 2 32 bit masks for
>> decoder and encoder. Malfunctioning firmware could still send payload with all
>> bits enabled in those masks. Then the driver needs to add same check to avoid
>> the memcpy in such case.
>>
>>> The bug here is that we can copy two arrays of size X into one array of size X.
>>>
>>> Please consider expanding the size of the storage array to accommodate the full
>>> size the protocol supports 2 x 32.
>> I see this as an alternate implementation to existing handling. 64 index would
>> never exist practically, so accommodating it only implies to store the data for
>> invalid response and gracefully close the session.
> 
> What's the contractual definition of "this many bits per encoder and decoder"
> between firmware and APSS in that case ?
> 
> Where do we get the idea that 32/2 per encoder/decoder is valid but 32 per
> encoder decoder is invalid ?
> 
> At this moment in time 16 encoder/decoder bits would be equally invalid.
> 
> I suggest the right answer is to buffer the protocol data unit - PDU maximum as
> an RX or constrain the maximum number of encoder/decoder bits based on HFI version.
> 
> ie.
> 
> - Either constrain on the PDU or
> - Constrain on the known number of maximum bits per f/w version

Let me simply ask this - What benefit we will be getting with above approaches
over the existing handling ?

Thanks,
Vikash
> ---
> bod
>