[PATCH 0/2] venus driver fixes to avoid possible OOB read access

[PATCH 0/2] venus driver fixes to avoid possible OOB read access

[Vedang Nagar]

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.

Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
---
Vedang Nagar (2):
      media: venus: fix OOB read issue due to double read
      media: venus: fix OOB access issue while reading sequence changed events

 drivers/media/platform/qcom/venus/hfi_msgs.c  | 62 ++++++++++++++++++++++++++-
 drivers/media/platform/qcom/venus/hfi_venus.c |  1 +
 2 files changed, 62 insertions(+), 1 deletion(-)
---
base-commit: 91e71d606356e50f238d7a87aacdee4abc427f07
change-id: 20241211-venus-security-fixes-50c22e2564d5

Best regards,
-- 
Vedang Nagar <quic_vnagar@quicinc.com>

[PATCH 1/2] media: venus: fix OOB read issue due to double read

[Vedang Nagar]

During message queue read, the address is being read twice
from the shared memory. The first read is validated against
the size of the packet, however the second read is not
being validated. Therefore, it's possible for firmware to
modify the value to a bigger invalid value which can lead
to OOB read access issue while reading the packet.
Added fix to reupdate the size of the packet which was
read for the first time.

Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index f9437b6412b91c2483670a2b11f4fd43f3206404..64cc9e916f53e5a9c82b1ff25c4475d622ebc321 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -298,6 +298,7 @@ static int venus_read_queue(struct venus_hfi_device *hdev,
 			memcpy(pkt, rd_ptr, len);
 			memcpy(pkt + len, queue->qmem.kva, new_rd_idx << 2);
 		}
+		memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));
 	} else {
 		/* bad packet received, dropping */
 		new_rd_idx = qhdr->write_idx;

-- 
2.34.1

Re: [PATCH 1/2] media: venus: fix OOB read issue due to double read

[Bryan O'Donoghue]

On 04/01/2025 05:41, Vedang Nagar wrote:
> During message queue read, the address is being read twice
> from the shared memory. The first read is validated against
> the size of the packet, however the second read is not
> being validated.

A brief scan of this code doesn't really show the base case you assert here.

Could you be a bit more explicit.

  Therefore, it's possible for firmware to
> modify the value to a bigger invalid value which can lead
> to OOB read access issue while reading the packet.
> Added fix to reupdate the size of the packet which was
> read for the first time.
> 
> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
> ---
>   drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index f9437b6412b91c2483670a2b11f4fd43f3206404..64cc9e916f53e5a9c82b1ff25c4475d622ebc321 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -298,6 +298,7 @@ static int venus_read_queue(struct venus_hfi_device *hdev,
>   			memcpy(pkt, rd_ptr, len);
>   			memcpy(pkt + len, queue->qmem.kva, new_rd_idx << 2);
>   		}
> +		memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));

I'm not entirely following your reasoning here.

Here's how the code looks after your change:

if (new_rd_idx < qsize) {
	memcpy(pkt, rd_ptr, dwords << 2);
} else {
	size_t len;
	
	new_rd_idx -= qsize;
	len = (dwords - new_rd_idx) << 2;
	memcpy(pkt, rd_ptr, len);
	memcpy(pkt + len, queue->qmem.kva, new_rd_idx << 2);
}

memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));

Which of the above memcpy() operations is subject to a pointer that 
firmware can influence exactly ?

Is this a real problem you've seen if so please add a backtrace to this 
commit log.

>   	} else {
>   		/* bad packet received, dropping */
>   		new_rd_idx = qhdr->write_idx;
> 

If this is a fix it requires a Fixes: tag.

Please add.

Still finding the reasoning you are outlining here not obvious.

---
bod

Re: [PATCH 1/2] media: venus: fix OOB read issue due to double read

[Vedang Nagar]

Hi Bryan,

On 1/6/2025 5:28 AM, Bryan O'Donoghue wrote:
> On 04/01/2025 05:41, Vedang Nagar wrote:
>> During message queue read, the address is being read twice
>> from the shared memory. The first read is validated against
>> the size of the packet, however the second read is not
>> being validated.
> 
> A brief scan of this code doesn't really show the base case you assert here.
> 
> Could you be a bit more explicit.
> 
>  Therefore, it's possible for firmware to
>> modify the value to a bigger invalid value which can lead
>> to OOB read access issue while reading the packet.
>> Added fix to reupdate the size of the packet which was
>> read for the first time.
>>
>> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_venus.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index f9437b6412b91c2483670a2b11f4fd43f3206404..64cc9e916f53e5a9c82b1ff25c4475d622ebc321 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -298,6 +298,7 @@ static int venus_read_queue(struct venus_hfi_device *hdev,
>>               memcpy(pkt, rd_ptr, len);
>>               memcpy(pkt + len, queue->qmem.kva, new_rd_idx << 2);
>>           }
>> +        memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));
> 
> I'm not entirely following your reasoning here.
> 
> Here's how the code looks after your change:
> 
> if (new_rd_idx < qsize) {
>     memcpy(pkt, rd_ptr, dwords << 2);
> } else {
>     size_t len;
>     
>     new_rd_idx -= qsize;
>     len = (dwords - new_rd_idx) << 2;
>     memcpy(pkt, rd_ptr, len);
>     memcpy(pkt + len, queue->qmem.kva, new_rd_idx << 2);
> }
> 
> memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));
> 
> Which of the above memcpy() operations is subject to a pointer that firmware can influence exactly ?
Below is the first read where dwords is being validated properly with the checks.
dwords = *rd_ptr >> 2;

Whereas the same address is being read for the second time:
memcpy(pkt, rd_ptr, dwords << 2);

For the second read the value is not validated which may get updated from the firmware
leading to incorrect memcpy into the packet and may lead to OOB read access while accessing
the packet.

To avoid the issue, planning to repopulate the pkt with the first read like below:
 *(u32 *)pkt = dwords << 2;
Pls ignore the original solution which seems to be not correct.
Pls let me know if it looks good, will fix it in next version.

Regards,
Vedang Nagar
> 
> Is this a real problem you've seen if so please add a backtrace to this commit log.
> 
>>       } else {
>>           /* bad packet received, dropping */
>>           new_rd_idx = qhdr->write_idx;
>>
> 
> If this is a fix it requires a Fixes: tag.
> 
> Please add.
> 
> Still finding the reasoning you are outlining here not obvious.
> 
> ---
> bod

Re: [PATCH 1/2] media: venus: fix OOB read issue due to double read

[Bryan O'Donoghue]

On 17/01/2025 08:39, Vedang Nagar wrote:
> Below is the first read where dwords is being validated properly with the checks.
> dwords = *rd_ptr >> 2;
> 
> Whereas the same address is being read for the second time:
> memcpy(pkt, rd_ptr, dwords << 2);
> 
> For the second read the value is not validated which may get updated from the firmware
> leading to incorrect memcpy into the packet and may lead to OOB read access while accessing
> the packet.

So you are saying that pkt points to memory that the firmware and host 
can simultaneously access.

The question is - if the length value can change between one read and 
another read - how do you trust the _content_ of the packet ?

Surely the right thing to do is to take a _copy_ of the entire frame and 
act on that frame exclusively on the host side ?

If I receive a frame, and read length X.

Then I need to re-read that frame because length may now by X+3.

This implies the _data_ in the frame has changed.

What exactly is the valid lifetime of this data from HFI RX interrupt ?

---
bod

Re: [PATCH 1/2] media: venus: fix OOB read issue due to double read

[Vedang Nagar]

Hi Bryan,

On 1/17/2025 3:55 PM, Bryan O'Donoghue wrote:
> On 17/01/2025 08:39, Vedang Nagar wrote:
>> Below is the first read where dwords is being validated properly with the checks.
>> dwords = *rd_ptr >> 2;
>>
>> Whereas the same address is being read for the second time:
>> memcpy(pkt, rd_ptr, dwords << 2);
>>
>> For the second read the value is not validated which may get updated from the firmware
>> leading to incorrect memcpy into the packet and may lead to OOB read access while accessing
>> the packet.
> 
> So you are saying that pkt points to memory that the firmware and host can simultaneously access.
> 
> The question is - if the length value can change between one read and another read - how do you trust the _content_ of the packet ?
Original content of the packet is validated while reading the packet in hfi_process_msg_packet function.
Whereas the current change is just to validate the size of the packet to avoid the Out of bound read access.
> 
> Surely the right thing to do is to take a _copy_ of the entire frame and act on that frame exclusively on the host side ?
> 
> If I receive a frame, and read length X.
> 
> Then I need to re-read that frame because length may now by X+3.
> 
> This implies the _data_ in the frame has changed.
Yes, the _data_ in the frame has changed and will get rejected while parsing that data.
So I think it's okay to no read or copy the entire frame again.
> 
> What exactly is the valid lifetime of this data from HFI RX interrupt ?
There is no as such lifetime of the interrupt, but any rogue firmware can corrupt the data in the packet.

Regards,
Vedang Nagar
> 
> ---
> bod

[PATCH 2/2] media: venus: fix OOB access issue while reading sequence changed events

[Vedang Nagar]

num_properties_changed is being read from the message queue but is
not validated. Value can be corrupted from the firmware leading to
OOB read access issues. Add fix to read the size of the packets as
well and crosscheck before reading from the packet.

Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
---
 drivers/media/platform/qcom/venus/hfi_msgs.c | 62 +++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
index 0a041b4db9efc549621de07dd13b4a3a37a70d11..3fff21ea744b0171e204dd0851fc46cb468e1979 100644
--- a/drivers/media/platform/qcom/venus/hfi_msgs.c
+++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
@@ -33,8 +33,8 @@ static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
 	struct hfi_buffer_requirements *bufreq;
 	struct hfi_extradata_input_crop *crop;
 	struct hfi_dpb_counts *dpb_count;
+	u32 ptype, rem_bytes;
 	u8 *data_ptr;
-	u32 ptype;
 
 	inst->error = HFI_ERR_NONE;
 
@@ -56,66 +56,126 @@ static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
 	}
 
 	data_ptr = (u8 *)&pkt->ext_event_data[0];
+	rem_bytes = pkt->shdr.hdr.size - sizeof(*pkt);
+	if (rem_bytes - 4 < 0) {
+		inst->error = HFI_ERR_SESSION_INSUFFICIENT_RESOURCES;
+		goto done;
+	}
+
 	do {
 		ptype = *((u32 *)data_ptr);
 		switch (ptype) {
 		case HFI_PROPERTY_PARAM_FRAME_SIZE:
+			if (rem_bytes < sizeof(u32))
+				break;
 			data_ptr += sizeof(u32);
+			rem_bytes -= sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_framesize))
+				break;
 			frame_sz = (struct hfi_framesize *)data_ptr;
 			event.width = frame_sz->width;
 			event.height = frame_sz->height;
 			data_ptr += sizeof(*frame_sz);
+			rem_bytes -= sizeof(struct hfi_framesize);
 			break;
 		case HFI_PROPERTY_PARAM_PROFILE_LEVEL_CURRENT:
+			if (rem_bytes < sizeof(u32))
+				break;
 			data_ptr += sizeof(u32);
+			rem_bytes -= sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_profile_level))
+				break;
 			profile_level = (struct hfi_profile_level *)data_ptr;
 			event.profile = profile_level->profile;
 			event.level = profile_level->level;
 			data_ptr += sizeof(*profile_level);
+			rem_bytes -= sizeof(struct hfi_profile_level);
 			break;
 		case HFI_PROPERTY_PARAM_VDEC_PIXEL_BITDEPTH:
+			if (rem_bytes < sizeof(u32))
+				break;
 			data_ptr += sizeof(u32);
+			rem_bytes -= sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_bit_depth))
+				break;
 			pixel_depth = (struct hfi_bit_depth *)data_ptr;
 			event.bit_depth = pixel_depth->bit_depth;
 			data_ptr += sizeof(*pixel_depth);
+			rem_bytes -= sizeof(struct hfi_bit_depth);
 			break;
 		case HFI_PROPERTY_PARAM_VDEC_PIC_STRUCT:
+			if (rem_bytes < sizeof(u32))
+				break;
 			data_ptr += sizeof(u32);
+			rem_bytes -= sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_pic_struct))
+				break;
 			pic_struct = (struct hfi_pic_struct *)data_ptr;
 			event.pic_struct = pic_struct->progressive_only;
 			data_ptr += sizeof(*pic_struct);
+			rem_bytes -= sizeof(struct hfi_pic_struct);
 			break;
 		case HFI_PROPERTY_PARAM_VDEC_COLOUR_SPACE:
+			if (rem_bytes < sizeof(u32))
+				break;
 			data_ptr += sizeof(u32);
+			rem_bytes -= sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_colour_space))
+				break;
 			colour_info = (struct hfi_colour_space *)data_ptr;
 			event.colour_space = colour_info->colour_space;
 			data_ptr += sizeof(*colour_info);
+			rem_bytes -= sizeof(struct hfi_colour_space);
 			break;
 		case HFI_PROPERTY_CONFIG_VDEC_ENTROPY:
+			if (rem_bytes < sizeof(u32))
+				break;
 			data_ptr += sizeof(u32);
+			rem_bytes -= sizeof(u32);
+			if (rem_bytes < sizeof(u32))
+				break;
 			event.entropy_mode = *(u32 *)data_ptr;
 			data_ptr += sizeof(u32);
+			rem_bytes -= sizeof(u32);
 			break;
 		case HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS:
+			if (rem_bytes < sizeof(u32))
+				break;
 			data_ptr += sizeof(u32);
+			rem_bytes -= sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_buffer_requirements))
+				break;
 			bufreq = (struct hfi_buffer_requirements *)data_ptr;
 			event.buf_count = hfi_bufreq_get_count_min(bufreq, ver);
 			data_ptr += sizeof(*bufreq);
+			rem_bytes -= sizeof(struct hfi_buffer_requirements);
 			break;
 		case HFI_INDEX_EXTRADATA_INPUT_CROP:
+			if (rem_bytes < sizeof(u32))
+				break;
 			data_ptr += sizeof(u32);
+			rem_bytes -= sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_extradata_input_crop))
+				break;
 			crop = (struct hfi_extradata_input_crop *)data_ptr;
 			event.input_crop.left = crop->left;
 			event.input_crop.top = crop->top;
 			event.input_crop.width = crop->width;
 			event.input_crop.height = crop->height;
 			data_ptr += sizeof(*crop);
+			rem_bytes -= sizeof(struct hfi_extradata_input_crop);
 			break;
 		case HFI_PROPERTY_PARAM_VDEC_DPB_COUNTS:
+			if (rem_bytes < sizeof(u32))
+				break;
 			data_ptr += sizeof(u32);
+			rem_bytes -= sizeof(u32);
+			if (rem_bytes < sizeof(struct hfi_dpb_counts))
+				break;
 			dpb_count = (struct hfi_dpb_counts *)data_ptr;
 			event.buf_count = dpb_count->fw_min_cnt;
 			data_ptr += sizeof(*dpb_count);
+			rem_bytes -= sizeof(struct hfi_dpb_counts);
 			break;
 		default:
 			break;

-- 
2.34.1

Re: [PATCH 2/2] media: venus: fix OOB access issue while reading sequence changed events

[Bryan O'Donoghue]

On 04/01/2025 05:41, Vedang Nagar wrote:
> num_properties_changed is being read from the message queue but is
> not validated. Value can be corrupted from the firmware leading to
> OOB read access issues. Add fix to read the size of the packets as
> well and crosscheck before reading from the packet.
> 
> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
Please see Vikash's series on this.

https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-2-483ae0a464b8@quicinc.com/

it seems to have exactly the same patch title ?

Is this patch supposed to be a follow-up to that patch ?

https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-0-483ae0a464b8@quicinc.com/

Expecting to see a V3 of the above. If the intention is to supersede 
that patch or some of those patches you should make clear here.

On the switch statement I'd have two comments.

#1 is everything really a " -= sizeof(u32)" ?
#2 if so then this ought to be factored out into a function
    => functional decomposition

---
bod

Re: [PATCH 2/2] media: venus: fix OOB access issue while reading sequence changed events

[Vedang Nagar]

Hi Bryan,

On 1/6/2025 5:36 AM, Bryan O'Donoghue wrote:
> On 04/01/2025 05:41, Vedang Nagar wrote:
>> num_properties_changed is being read from the message queue but is
>> not validated. Value can be corrupted from the firmware leading to
>> OOB read access issues. Add fix to read the size of the packets as
>> well and crosscheck before reading from the packet.
>>
>> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
> Please see Vikash's series on this.
> 
> https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-2-483ae0a464b8@quicinc.com/
> 
> it seems to have exactly the same patch title ?
> 
> Is this patch supposed to be a follow-up to that patch ?
> 
> https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-0-483ae0a464b8@quicinc.com/
> 
> Expecting to see a V3 of the above. If the intention is to supersede that patch or some of those patches you should make clear here.
No, this is a different series having OOB fixes similar to ones posted by Vikash.
> 
> On the switch statement I'd have two comments.
> 
> #1 is everything really a " -= sizeof(u32)" ?
Yes, it's everytime " -= sizeof(u32) " since the first the first word read is ptype of size u32
> #2 if so then this ought to be factored out into a function
>    => functional decomposition
Sure, will fix this with decomposition into functions.

Regards,
Vedang Nagar
> 
> ---
> bod

Re: [PATCH 2/2] media: venus: fix OOB access issue while reading sequence changed events

[Bryan O'Donoghue]

On 17/01/2025 08:39, Vedang Nagar wrote:
> Hi Bryan,
> 
> On 1/6/2025 5:36 AM, Bryan O'Donoghue wrote:
>> On 04/01/2025 05:41, Vedang Nagar wrote:
>>> num_properties_changed is being read from the message queue but is
>>> not validated. Value can be corrupted from the firmware leading to
>>> OOB read access issues. Add fix to read the size of the packets as
>>> well and crosscheck before reading from the packet.
>>>
>>> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
>> Please see Vikash's series on this.
>>
>> https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-2-483ae0a464b8@quicinc.com/
>>
>> it seems to have exactly the same patch title ?
>>
>> Is this patch supposed to be a follow-up to that patch ?
>>
>> https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-0-483ae0a464b8@quicinc.com/
>>
>> Expecting to see a V3 of the above. If the intention is to supersede that patch or some of those patches you should make clear here.
> No, this is a different series having OOB fixes similar to ones posted by Vikash.

OK, please use a different patch title.

I understand the motive to repeat the patch title but, its confusing. If 
you added some text to make the OOB more specific then it would be 
possible to differentiate between.

"fix OOB access issue while reading sequence changed events 'in some 
location' || 'on some path'"


>>
>> On the switch statement I'd have two comments.
>>
>> #1 is everything really a " -= sizeof(u32)" ?
> Yes, it's everytime " -= sizeof(u32) " since the first the first word read is ptype of size u32
>> #2 if so then this ought to be factored out into a function
>>     => functional decomposition
> Sure, will fix this with decomposition into functions.

Is firmware sending a change event or updating a packet already in memory ?

What is the nature of the change event and how do you guarantee the 
second read is valid when the first read can be considered invalid ?

i.e.

- Read - derive read value X.
- Do some stuff.
- Read - again to make sure length value is still X.
- Do all sorts of other processing.

At which point is the sequence considered complete and the data 
considered "locked" and valid ?

What happens if you get a subsequent change event once this procedure 
has completed ?

---
bod

Re: [PATCH 2/2] media: venus: fix OOB access issue while reading sequence changed events

[Vedang Nagar]

Hi Bryan,

On 1/17/2025 4:02 PM, Bryan O'Donoghue wrote:
> On 17/01/2025 08:39, Vedang Nagar wrote:
>> Hi Bryan,
>>
>> On 1/6/2025 5:36 AM, Bryan O'Donoghue wrote:
>>> On 04/01/2025 05:41, Vedang Nagar wrote:
>>>> num_properties_changed is being read from the message queue but is
>>>> not validated. Value can be corrupted from the firmware leading to
>>>> OOB read access issues. Add fix to read the size of the packets as
>>>> well and crosscheck before reading from the packet.
>>>>
>>>> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
>>> Please see Vikash's series on this.
>>>
>>> https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-2-483ae0a464b8@quicinc.com/
>>>
>>> it seems to have exactly the same patch title ?
>>>
>>> Is this patch supposed to be a follow-up to that patch ?
>>>
>>> https://lore.kernel.org/linux-arm-msm/20241128-venus_oob_2-v2-0-483ae0a464b8@quicinc.com/
>>>
>>> Expecting to see a V3 of the above. If the intention is to supersede that patch or some of those patches you should make clear here.
>> No, this is a different series having OOB fixes similar to ones posted by Vikash.
> 
> OK, please use a different patch title.
> 
> I understand the motive to repeat the patch title but, its confusing. If you added some text to make the OOB more specific then it would be possible to differentiate between.
> 
> "fix OOB access issue while reading sequence changed events 'in some location' || 'on some path'"
Got it, will update the patch title in the next version.
> 
> 
>>>
>>> On the switch statement I'd have two comments.
>>>
>>> #1 is everything really a " -= sizeof(u32)" ?
>> Yes, it's everytime " -= sizeof(u32) " since the first the first word read is ptype of size u32
>>> #2 if so then this ought to be factored out into a function
>>>     => functional decomposition
>> Sure, will fix this with decomposition into functions.
> 
> Is firmware sending a change event or updating a packet already in memory ?
Firmware is sending a change event in a new packet.
> 
> What is the nature of the change event and how do you guarantee the second read is valid when the first read can be considered invalid ?
> 
> i.e.
> 
> - Read - derive read value X.
> - Do some stuff.
> - Read - again to make sure length value is still X.
> - Do all sorts of other processing.
> 
> At which point is the sequence considered complete and the data considered "locked" and valid ?
With rouge firmware, the data can get invalid by the firmware during the second read but our intention is to avoid reading the out of bound data.
Whereas reading the invalid data will eventually lead to session_error at some point in future.
> 
> What happens if you get a subsequent change event once this procedure has completed ?
Either instance will go into error state or the same process will be repeated.
Please let us know if this is not a correct approach, or any other suggestions.

Regards,
Vedang Nagar
> 
> ---
> bod