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

[PATCH v2 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>
---
Changes in v2:
- Decompose sequence change event function. 
- Fix repopulating the packet .with the first read during read_queue.
- Link to v1: https://lore.kernel.org/r/20250104-venus-security-fixes-v1-0-9d0dd4594cb4@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  | 72 +++++++++++++++++++++++----
 drivers/media/platform/qcom/venus/hfi_venus.c |  1 +
 2 files changed, 63 insertions(+), 10 deletions(-)
---
base-commit: 91e71d606356e50f238d7a87aacdee4abc427f07
change-id: 20241211-venus-security-fixes-50c22e2564d5

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

[PATCH v2 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..c124db8ac79d18f32289a690ee82145dc93daee6 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);
 		}
+		*(u32 *)pkt = dwords << 2;
 	} else {
 		/* bad packet received, dropping */
 		new_rd_idx = qhdr->write_idx;

-- 
2.34.1

[PATCH v2 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 | 72 ++++++++++++++++++++++++----
 1 file changed, 62 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
index 0a041b4db9efc549621de07dd13b4a3a37a70d11..2ad60a3fbfe0286de09a44664fc3b30259aa0368 100644
--- a/drivers/media/platform/qcom/venus/hfi_msgs.c
+++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
@@ -19,6 +19,16 @@
 #define VER_STR_SZ		128
 #define SMEM_IMG_OFFSET_VENUS	(14 * 128)
 
+static inline int increment_data_ptr(u8 *data_ptr, u32 *rem_bytes)
+{
+	if (*rem_bytes < sizeof(u32))
+		return -EINVAL;
+	data_ptr += sizeof(u32);
+	*rem_bytes -= sizeof(u32);
+
+	return 0;
+}
+
 static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
 			      struct hfi_msg_event_notify_pkt *pkt)
 {
@@ -33,8 +43,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 +66,108 @@ 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:
-			data_ptr += sizeof(u32);
+			if (increment_data_ptr(data_ptr, &rem_bytes))
+				break;
+			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:
-			data_ptr += sizeof(u32);
+			if (increment_data_ptr(data_ptr, &rem_bytes))
+				break;
+			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:
-			data_ptr += sizeof(u32);
+			if (increment_data_ptr(data_ptr, &rem_bytes))
+				break;
+			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:
-			data_ptr += sizeof(u32);
+			if (increment_data_ptr(data_ptr, &rem_bytes))
+				break;
+			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:
-			data_ptr += sizeof(u32);
+			if (increment_data_ptr(data_ptr, &rem_bytes))
+				break;
+			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:
-			data_ptr += sizeof(u32);
+			if (increment_data_ptr(data_ptr, &rem_bytes))
+				break;
+			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:
-			data_ptr += sizeof(u32);
+			if (increment_data_ptr(data_ptr, &rem_bytes))
+				break;
+			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:
-			data_ptr += sizeof(u32);
+			if (increment_data_ptr(data_ptr, &rem_bytes))
+				break;
+			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:
-			data_ptr += sizeof(u32);
+			if (increment_data_ptr(data_ptr, &rem_bytes))
+				break;
+			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 v2 2/2] media: venus: fix OOB access issue while reading sequence changed events

[kernel test robot]

Hi Vedang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 91e71d606356e50f238d7a87aacdee4abc427f07]

url:    https://github.com/intel-lab-lkp/linux/commits/Vedang-Nagar/media-venus-fix-OOB-read-issue-due-to-double-read/20250216-012213
base:   91e71d606356e50f238d7a87aacdee4abc427f07
patch link:    https://lore.kernel.org/r/20250215-venus-security-fixes-v2-2-cfc7e4b87168%40quicinc.com
patch subject: [PATCH v2 2/2] media: venus: fix OOB access issue while reading sequence changed events
config: s390-randconfig-002-20250216 (https://download.01.org/0day-ci/archive/20250216/202502161251.lBVyP1dc-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250216/202502161251.lBVyP1dc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502161251.lBVyP1dc-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/media/platform/qcom/venus/hfi_msgs.c:22:42: warning: parameter 'data_ptr' set but not used [-Wunused-but-set-parameter]
      22 | static inline int increment_data_ptr(u8 *data_ptr, u32 *rem_bytes)
         |                                          ^
   1 warning generated.


vim +/data_ptr +22 drivers/media/platform/qcom/venus/hfi_msgs.c

    21	
  > 22	static inline int increment_data_ptr(u8 *data_ptr, u32 *rem_bytes)
    23	{
    24		if (*rem_bytes < sizeof(u32))
    25			return -EINVAL;
    26		data_ptr += sizeof(u32);
    27		*rem_bytes -= sizeof(u32);
    28	
    29		return 0;
    30	}
    31	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Bryan O'Donoghue]

On 15/02/2025 17:19, Vedang Nagar wrote:
> 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>
> ---
> Changes in v2:
> - Decompose sequence change event function.
> - Fix repopulating the packet .with the first read during read_queue.
> - Link to v1: https://lore.kernel.org/r/20250104-venus-security-fixes-v1-0-9d0dd4594cb4@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  | 72 +++++++++++++++++++++++----
>   drivers/media/platform/qcom/venus/hfi_venus.c |  1 +
>   2 files changed, 63 insertions(+), 10 deletions(-)
> ---
> base-commit: 91e71d606356e50f238d7a87aacdee4abc427f07
> change-id: 20241211-venus-security-fixes-50c22e2564d5
> 
> Best regards,

Could you please address the feedback I gave you / questions posited in 
these two messages ?

4cfc1fe1-2fab-4256-9ce2-b4a0aad1069e@linaro.org

0eab7323-ce86-40c7-9737-06eedcdf492d@linaro.org

The basic question : what is the lifetime of the data from RX interrupt 
to consumption by another system agent, DSP, userspace, whatever ?

Why is it in this small specific window that the data can change but not 
later ? What is the mechanism the data can change and how do the changes 
you propose here address the data lifetime problem ?

Without that context, I don't believe it is really possible to validate 
an additional memcpy() here and there in the code as fixing anything.

---
bod

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

[Vedang Nagar]

Hi Bryan,

On 2/16/2025 9:33 PM, Bryan O'Donoghue wrote:
> On 15/02/2025 17:19, Vedang Nagar wrote:
>> 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>
>> ---
>> Changes in v2:
>> - Decompose sequence change event function.
>> - Fix repopulating the packet .with the first read during read_queue.
>> - Link to v1: https://lore.kernel.org/r/20250104-venus-security-fixes- 
>> v1-0-9d0dd4594cb4@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  | 72 +++++++++++++++++ 
>> ++++++----
>>   drivers/media/platform/qcom/venus/hfi_venus.c |  1 +
>>   2 files changed, 63 insertions(+), 10 deletions(-)
>> ---
>> base-commit: 91e71d606356e50f238d7a87aacdee4abc427f07
>> change-id: 20241211-venus-security-fixes-50c22e2564d5
>>
>> Best regards,
> 
> Could you please address the feedback I gave you / questions posited in 
> these two messages ?
> 
> 4cfc1fe1-2fab-4256-9ce2-b4a0aad1069e@linaro.org
> 
> 0eab7323-ce86-40c7-9737-06eedcdf492d@linaro.org
> 
> The basic question : what is the lifetime of the data from RX interrupt 
> to consumption by another system agent, DSP, userspace, whatever ?
As mentioned in [1], With the regular firmware, after RX interrupt the 
data can be considered as valid until next interrupt is raised, but with 
the rouge firmware, data can get invalid during the second read and our 
intention is to avoid out of bound access read because of such issues.

[1]: 
https://lore.kernel.org/lkml/4cfc1fe1-2fab-4256-9ce2-b4a0aad1069e@linaro.org/T/#m5f1737b16e68f8b8fc1d75517356b6566d0ec619
> 
> Why is it in this small specific window that the data can change but not 
> later ? What is the mechanism the data can change and how do the changes 
> you propose here address the data lifetime problem ?
Currently this issue has been discovered by external researchers at this 
point, but if any such OOB issue is discovered at later point as well 
then we shall fix them as well.

Also, with rougue firmware we cannot fix the data lifetime problem in my 
opinion, but atleast we can fix the out of bound issues.
> 
> Without that context, I don't believe it is really possible to validate 
> an additional memcpy() here and there in the code as fixing anything.
There is no additional memcpy() now in the v2 patch, but as part of the 
fix, we are just trying to retain the length of the packet which was 
being read in the first memcpy() to avoid the OOB read access.

Please let me know if you have any other suggestions.

Regards,
Vedang Nagar
> 
> ---
> bod

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

[Bryan O'Donoghue]

On 02/03/2025 11:58, Vedang Nagar wrote:
>>
>> The basic question : what is the lifetime of the data from RX 
>> interrupt to consumption by another system agent, DSP, userspace, 
>> whatever ?
> As mentioned in [1], With the regular firmware, after RX interrupt the 
> data can be considered as valid until next interrupt is raised, but with 
> the rouge firmware, data can get invalid during the second read and our 
> intention is to avoid out of bound access read because of such issues.

This is definitely the part I don't compute.

1. RX interrupt
2. Frame#0 Some amount of time data is always valid
3. RX interrupt - new data
4. Frame#1 new data delivered into a buffer

Are you describing a case between RX interrupts 1-3 or a case after 1-4?

Why do we need to write code for rouge firmware anyway ?

And the real question - if the data can be invalidated in the 1-3 window 
above when is the safe time to snapshot that data ?

We seem to have alot of submissions to deal with 'rouge' firmware 
without I think properly describing the problem of the _expected_ data 
lifetime.

So

a) What is the expected data lifetime of an RX buffer between one
    RX IRQ and the next ?
    I hope the answer to this is - APSS owns the buffer.
    This is BTW usually the case in these types of asymmetric setups
    with a flag or some other kind of semaphore that indicates which
    side of the data-exchange owns the buffer.

b) In this rouge - buggy - firmware case what is the scope of the
    potential race condition ?

    What I'd really like to know here is why we have to seemingly
    memcpy() again and again in seemingly incongrous and not
    immediately obvious places in the code.

    Would we not be better advised to do a memcpy() of the entire
    RX frame in the RX IRQ handler path if as you appear to me
    suggesting - the firmware can "race" with the APSS
    i.e. the data-buffer ownership flag either doesn't work
    or isn't respected by one side in the data-exchange.

Can we please have a detailed description of the race condition here ?

I don't doubt the new memcpy() makes sense to you but without this 
detailed understanding of the underlying problem its virtually 
impossible to debate the appropriate remediation - perhaps this patch 
you've submitted - or some other solution.

Sorry to dig into my trench here but, way more detail is needed.

> [1]: https://lore.kernel.org/lkml/4cfc1fe1-2fab-4256-9ce2- 
> b4a0aad1069e@linaro.org/T/#m5f1737b16e68f8b8fc1d75517356b6566d0ec619
>>
>> Why is it in this small specific window that the data can change but 
>> not later ? What is the mechanism the data can change and how do the 
>> changes you propose here address the data lifetime problem ?
> Currently this issue has been discovered by external researchers at this 
> point, but if any such OOB issue is discovered at later point as well 
> then we shall fix them as well.

Right but, I'm looking for a detailed description of the problem.

Can you describe from RX interrupt again what the expected data lifetime 
of the RX frame is, which I hope we agree is until the next RX interrupt 
associated with a given buffer with an ownership flag shared between 
firmware and APSS - and then under what circumstances that "software 
contract" is being violated.

> Also, with rougue firmware we cannot fix the data lifetime problem in my 
> opinion, but atleast we can fix the out of bound issues.
>>
>> Without that context, I don't believe it is really possible to 
>> validate an additional memcpy() here and there in the code as fixing 
>> anything.
> There is no additional memcpy() now in the v2 patch, but as part of the 
> fix, we are just trying to retain the length of the packet which was 
> being read in the first memcpy() to avoid the OOB read access.

I can't make a suggestion because - personally speaking I still don't 
quite understand the data-race you are describing.

I get that you say the firmware is breaking the contract but, without 
more detail on _how_ it breaks that contract I don't think it's really 
possible to validate your fix here, fixes anything.

---
bod

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

[Vikash Garodia]

On 3/2/2025 9:26 PM, Bryan O'Donoghue wrote:
> On 02/03/2025 11:58, Vedang Nagar wrote:
>>>
>>> The basic question : what is the lifetime of the data from RX interrupt to
>>> consumption by another system agent, DSP, userspace, whatever ?
>> As mentioned in [1], With the regular firmware, after RX interrupt the data
>> can be considered as valid until next interrupt is raised, but with the rouge
>> firmware, data can get invalid during the second read and our intention is to
>> avoid out of bound access read because of such issues.
> 
> This is definitely the part I don't compute.
> 
> 1. RX interrupt
> 2. Frame#0 Some amount of time data is always valid
This is not correct. Its not the amount of time which determines the validity of
the data, its the possibility of rogue firmware which, if incase, puts up the
date in shared queue, would always be invalid, irrespective of time.

> 3. RX interrupt - new data
> 4. Frame#1 new data delivered into a buffer
> 
> Are you describing a case between RX interrupts 1-3 or a case after 1-4?
> 
> Why do we need to write code for rouge firmware anyway ?
It is a way to prevent any possibility of OOB, similar to how any API does check
for validity of any arguments passed to it, prior to processing.
> 
> And the real question - if the data can be invalidated in the 1-3 window above
> when is the safe time to snapshot that data ?
> 
> We seem to have alot of submissions to deal with 'rouge' firmware without I
> think properly describing the problem of the _expected_ data lifetime.
> 
> So
> 
> a) What is the expected data lifetime of an RX buffer between one
>    RX IRQ and the next ?
>    I hope the answer to this is - APSS owns the buffer.
>    This is BTW usually the case in these types of asymmetric setups
>    with a flag or some other kind of semaphore that indicates which
>    side of the data-exchange owns the buffer.
> 
> b) In this rouge - buggy - firmware case what is the scope of the
>    potential race condition ?
> 
>    What I'd really like to know here is why we have to seemingly
>    memcpy() again and again in seemingly incongrous and not
>    immediately obvious places in the code.
> 
>    Would we not be better advised to do a memcpy() of the entire
>    RX frame in the RX IRQ handler path if as you appear to me
>    suggesting - the firmware can "race" with the APSS
>    i.e. the data-buffer ownership flag either doesn't work
>    or isn't respected by one side in the data-exchange.
> 
> Can we please have a detailed description of the race condition here ?
Below is the report which the reporter reported leading to OOB, let me know if
you are unable to deduce the trail leading to OOB here.

OOB read issue is in function event_seq_changed, please reference below code
snippet:

Buggy code snippet:

static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
        struct hfi_msg_event_notify_pkt *pkt)
...
num_properties_changed = pkt->event_data2; //num_properties_changed is from
message and is not validated.
...
data_ptr = (u8 *)&pkt->ext_event_data[0];
do {
 ptype = *((u32 *)data_ptr);
 switch (ptype) {
 case HFI_PROPERTY_PARAM_FRAME_SIZE:
  data_ptr += sizeof(u32);
  frame_sz = (struct hfi_framesize *)data_ptr;
  event.width = frame_sz->width;
...
 }
 num_properties_changed--;
} while (num_properties_changed > 0);
```
There is no validation against `num_properties_changed = pkt->event_data2`, so
OOB read occurs.
> 
> I don't doubt the new memcpy() makes sense to you but without this detailed
> understanding of the underlying problem its virtually impossible to debate the
> appropriate remediation - perhaps this patch you've submitted - or some other
> solution.
> 
> Sorry to dig into my trench here but, way more detail is needed.
> 
>> [1]: https://lore.kernel.org/lkml/4cfc1fe1-2fab-4256-9ce2-
>> b4a0aad1069e@linaro.org/T/#m5f1737b16e68f8b8fc1d75517356b6566d0ec619
>>>
>>> Why is it in this small specific window that the data can change but not
>>> later ? What is the mechanism the data can change and how do the changes you
>>> propose here address the data lifetime problem ?
>> Currently this issue has been discovered by external researchers at this
>> point, but if any such OOB issue is discovered at later point as well then we
>> shall fix them as well.
> 
> Right but, I'm looking for a detailed description of the problem.
> 
> Can you describe from RX interrupt again what the expected data lifetime of the
> RX frame is, which I hope we agree is until the next RX interrupt associated
> with a given buffer with an ownership flag shared between firmware and APSS -
> and then under what circumstances that "software contract" is being violated.
> 
>> Also, with rougue firmware we cannot fix the data lifetime problem in my
>> opinion, but atleast we can fix the out of bound issues.
>>>
>>> Without that context, I don't believe it is really possible to validate an
>>> additional memcpy() here and there in the code as fixing anything.
>> There is no additional memcpy() now in the v2 patch, but as part of the fix,
>> we are just trying to retain the length of the packet which was being read in
>> the first memcpy() to avoid the OOB read access.
> 
> I can't make a suggestion because - personally speaking I still don't quite
> understand the data-race you are describing.
Go through the reports from the reporter, it was quite evident in leading upto
OOB case.
Putting up the sequence for you to go over the interrupt handling and message
queue parsing of the packets from firmware
1.
https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_venus.c#L1082
2.
https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L816
3. event handling (this particular case)
https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L658
4.
https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L22

the "struct hfi_msg_event_notify_pkt *pkt" pkt here is having the data read from
shared queue.

> 
> I get that you say the firmware is breaking the contract but, without more
> detail on _how_ it breaks that contract I don't think it's really possible to
> validate your fix here, fixes anything.
> 
> ---
> bod

Regards,
Vikash

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

[Bryan O'Donoghue]

On 03/03/2025 13:12, Vikash Garodia wrote:
> 
> On 3/2/2025 9:26 PM, Bryan O'Donoghue wrote:
>> On 02/03/2025 11:58, Vedang Nagar wrote:
>>>>
>>>> The basic question : what is the lifetime of the data from RX interrupt to
>>>> consumption by another system agent, DSP, userspace, whatever ?
>>> As mentioned in [1], With the regular firmware, after RX interrupt the data
>>> can be considered as valid until next interrupt is raised, but with the rouge
>>> firmware, data can get invalid during the second read and our intention is to
>>> avoid out of bound access read because of such issues.
>>
>> This is definitely the part I don't compute.
>>
>> 1. RX interrupt
>> 2. Frame#0 Some amount of time data is always valid
> This is not correct. Its not the amount of time which determines the validity of
> the data, its the possibility of rogue firmware which, if incase, puts up the
> date in shared queue, would always be invalid, irrespective of time.
> 
>> 3. RX interrupt - new data
>> 4. Frame#1 new data delivered into a buffer
>>
>> Are you describing a case between RX interrupts 1-3 or a case after 1-4?
>>
>> Why do we need to write code for rouge firmware anyway ?
> It is a way to prevent any possibility of OOB, similar to how any API does check
> for validity of any arguments passed to it, prior to processing.
>>
>> And the real question - if the data can be invalidated in the 1-3 window above
>> when is the safe time to snapshot that data ?
>>
>> We seem to have alot of submissions to deal with 'rouge' firmware without I
>> think properly describing the problem of the _expected_ data lifetime.
>>
>> So
>>
>> a) What is the expected data lifetime of an RX buffer between one
>>     RX IRQ and the next ?
>>     I hope the answer to this is - APSS owns the buffer.
>>     This is BTW usually the case in these types of asymmetric setups
>>     with a flag or some other kind of semaphore that indicates which
>>     side of the data-exchange owns the buffer.
>>
>> b) In this rouge - buggy - firmware case what is the scope of the
>>     potential race condition ?
>>
>>     What I'd really like to know here is why we have to seemingly
>>     memcpy() again and again in seemingly incongrous and not
>>     immediately obvious places in the code.
>>
>>     Would we not be better advised to do a memcpy() of the entire
>>     RX frame in the RX IRQ handler path if as you appear to me
>>     suggesting - the firmware can "race" with the APSS
>>     i.e. the data-buffer ownership flag either doesn't work
>>     or isn't respected by one side in the data-exchange.
>>
>> Can we please have a detailed description of the race condition here ?
> Below is the report which the reporter reported leading to OOB, let me know if
> you are unable to deduce the trail leading to OOB here.
> 
> OOB read issue is in function event_seq_changed, please reference below code
> snippet:
> 
> Buggy code snippet:
> 
> static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
>          struct hfi_msg_event_notify_pkt *pkt)
> ...
> num_properties_changed = pkt->event_data2; //num_properties_changed is from
> message and is not validated.
> ...
> data_ptr = (u8 *)&pkt->ext_event_data[0];
> do {
>   ptype = *((u32 *)data_ptr);
>   switch (ptype) {
>   case HFI_PROPERTY_PARAM_FRAME_SIZE:
>    data_ptr += sizeof(u32);
>    frame_sz = (struct hfi_framesize *)data_ptr;
>    event.width = frame_sz->width;
> ...
>   }
>   num_properties_changed--;
> } while (num_properties_changed > 0);
> ```
> There is no validation against `num_properties_changed = pkt->event_data2`, so
> OOB read occurs.
>>
>> I don't doubt the new memcpy() makes sense to you but without this detailed
>> understanding of the underlying problem its virtually impossible to debate the
>> appropriate remediation - perhaps this patch you've submitted - or some other
>> solution.
>>
>> Sorry to dig into my trench here but, way more detail is needed.
>>
>>> [1]: https://lore.kernel.org/lkml/4cfc1fe1-2fab-4256-9ce2-
>>> b4a0aad1069e@linaro.org/T/#m5f1737b16e68f8b8fc1d75517356b6566d0ec619
>>>>
>>>> Why is it in this small specific window that the data can change but not
>>>> later ? What is the mechanism the data can change and how do the changes you
>>>> propose here address the data lifetime problem ?
>>> Currently this issue has been discovered by external researchers at this
>>> point, but if any such OOB issue is discovered at later point as well then we
>>> shall fix them as well.
>>
>> Right but, I'm looking for a detailed description of the problem.
>>
>> Can you describe from RX interrupt again what the expected data lifetime of the
>> RX frame is, which I hope we agree is until the next RX interrupt associated
>> with a given buffer with an ownership flag shared between firmware and APSS -
>> and then under what circumstances that "software contract" is being violated.
>>
>>> Also, with rougue firmware we cannot fix the data lifetime problem in my
>>> opinion, but atleast we can fix the out of bound issues.
>>>>
>>>> Without that context, I don't believe it is really possible to validate an
>>>> additional memcpy() here and there in the code as fixing anything.
>>> There is no additional memcpy() now in the v2 patch, but as part of the fix,
>>> we are just trying to retain the length of the packet which was being read in
>>> the first memcpy() to avoid the OOB read access.
>>
>> I can't make a suggestion because - personally speaking I still don't quite
>> understand the data-race you are describing.
> Go through the reports from the reporter, it was quite evident in leading upto
> OOB case.
> Putting up the sequence for you to go over the interrupt handling and message
> queue parsing of the packets from firmware
> 1.
> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_venus.c#L1082
> 2.
> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L816
> 3. event handling (this particular case)
> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L658
> 4.
> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L22
> 
> the "struct hfi_msg_event_notify_pkt *pkt" pkt here is having the data read from
> shared queue.
> 
>>
>> I get that you say the firmware is breaking the contract but, without more
>> detail on _how_ it breaks that contract I don't think it's really possible to
>> validate your fix here, fixes anything.
>>
>> ---
>> bod
> 
> Regards,
> Vikash

I'll go through all of these links given here, thanks.

Whatever the result of the review, this detail needs to go into the 
commit log so that a reviewer can reasonably read the problem 
description and evaluate against submitted code as a fix.

---
bod

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

[Vikash Garodia]

On 3/3/2025 8:26 PM, Bryan O'Donoghue wrote:
> On 03/03/2025 13:12, Vikash Garodia wrote:
>>
>> On 3/2/2025 9:26 PM, Bryan O'Donoghue wrote:
>>> On 02/03/2025 11:58, Vedang Nagar wrote:
>>>>>
>>>>> The basic question : what is the lifetime of the data from RX interrupt to
>>>>> consumption by another system agent, DSP, userspace, whatever ?
>>>> As mentioned in [1], With the regular firmware, after RX interrupt the data
>>>> can be considered as valid until next interrupt is raised, but with the rouge
>>>> firmware, data can get invalid during the second read and our intention is to
>>>> avoid out of bound access read because of such issues.
>>>
>>> This is definitely the part I don't compute.
>>>
>>> 1. RX interrupt
>>> 2. Frame#0 Some amount of time data is always valid
>> This is not correct. Its not the amount of time which determines the validity of
>> the data, its the possibility of rogue firmware which, if incase, puts up the
>> date in shared queue, would always be invalid, irrespective of time.
>>
>>> 3. RX interrupt - new data
>>> 4. Frame#1 new data delivered into a buffer
>>>
>>> Are you describing a case between RX interrupts 1-3 or a case after 1-4?
>>>
>>> Why do we need to write code for rouge firmware anyway ?
>> It is a way to prevent any possibility of OOB, similar to how any API does check
>> for validity of any arguments passed to it, prior to processing.
>>>
>>> And the real question - if the data can be invalidated in the 1-3 window above
>>> when is the safe time to snapshot that data ?
>>>
>>> We seem to have alot of submissions to deal with 'rouge' firmware without I
>>> think properly describing the problem of the _expected_ data lifetime.
>>>
>>> So
>>>
>>> a) What is the expected data lifetime of an RX buffer between one
>>>     RX IRQ and the next ?
>>>     I hope the answer to this is - APSS owns the buffer.
>>>     This is BTW usually the case in these types of asymmetric setups
>>>     with a flag or some other kind of semaphore that indicates which
>>>     side of the data-exchange owns the buffer.
>>>
>>> b) In this rouge - buggy - firmware case what is the scope of the
>>>     potential race condition ?
>>>
>>>     What I'd really like to know here is why we have to seemingly
>>>     memcpy() again and again in seemingly incongrous and not
>>>     immediately obvious places in the code.
>>>
>>>     Would we not be better advised to do a memcpy() of the entire
>>>     RX frame in the RX IRQ handler path if as you appear to me
>>>     suggesting - the firmware can "race" with the APSS
>>>     i.e. the data-buffer ownership flag either doesn't work
>>>     or isn't respected by one side in the data-exchange.
>>>
>>> Can we please have a detailed description of the race condition here ?
>> Below is the report which the reporter reported leading to OOB, let me know if
>> you are unable to deduce the trail leading to OOB here.
>>
>> OOB read issue is in function event_seq_changed, please reference below code
>> snippet:
>>
>> Buggy code snippet:
>>
>> static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
>>          struct hfi_msg_event_notify_pkt *pkt)
>> ...
>> num_properties_changed = pkt->event_data2; //num_properties_changed is from
>> message and is not validated.
>> ...
>> data_ptr = (u8 *)&pkt->ext_event_data[0];
>> do {
>>   ptype = *((u32 *)data_ptr);
>>   switch (ptype) {
>>   case HFI_PROPERTY_PARAM_FRAME_SIZE:
>>    data_ptr += sizeof(u32);
>>    frame_sz = (struct hfi_framesize *)data_ptr;
>>    event.width = frame_sz->width;
>> ...
>>   }
>>   num_properties_changed--;
>> } while (num_properties_changed > 0);
>> ```
>> There is no validation against `num_properties_changed = pkt->event_data2`, so
>> OOB read occurs.
>>>
>>> I don't doubt the new memcpy() makes sense to you but without this detailed
>>> understanding of the underlying problem its virtually impossible to debate the
>>> appropriate remediation - perhaps this patch you've submitted - or some other
>>> solution.
>>>
>>> Sorry to dig into my trench here but, way more detail is needed.
>>>
>>>> [1]: https://lore.kernel.org/lkml/4cfc1fe1-2fab-4256-9ce2-
>>>> b4a0aad1069e@linaro.org/T/#m5f1737b16e68f8b8fc1d75517356b6566d0ec619
>>>>>
>>>>> Why is it in this small specific window that the data can change but not
>>>>> later ? What is the mechanism the data can change and how do the changes you
>>>>> propose here address the data lifetime problem ?
>>>> Currently this issue has been discovered by external researchers at this
>>>> point, but if any such OOB issue is discovered at later point as well then we
>>>> shall fix them as well.
>>>
>>> Right but, I'm looking for a detailed description of the problem.
>>>
>>> Can you describe from RX interrupt again what the expected data lifetime of the
>>> RX frame is, which I hope we agree is until the next RX interrupt associated
>>> with a given buffer with an ownership flag shared between firmware and APSS -
>>> and then under what circumstances that "software contract" is being violated.
>>>
>>>> Also, with rougue firmware we cannot fix the data lifetime problem in my
>>>> opinion, but atleast we can fix the out of bound issues.
>>>>>
>>>>> Without that context, I don't believe it is really possible to validate an
>>>>> additional memcpy() here and there in the code as fixing anything.
>>>> There is no additional memcpy() now in the v2 patch, but as part of the fix,
>>>> we are just trying to retain the length of the packet which was being read in
>>>> the first memcpy() to avoid the OOB read access.
>>>
>>> I can't make a suggestion because - personally speaking I still don't quite
>>> understand the data-race you are describing.
>> Go through the reports from the reporter, it was quite evident in leading upto
>> OOB case.
>> Putting up the sequence for you to go over the interrupt handling and message
>> queue parsing of the packets from firmware
>> 1.
>> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_venus.c#L1082
>> 2.
>> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L816
>> 3. event handling (this particular case)
>> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L658
>> 4.
>> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L22
>>
>> the "struct hfi_msg_event_notify_pkt *pkt" pkt here is having the data read from
>> shared queue.
>>
>>>
>>> I get that you say the firmware is breaking the contract but, without more
>>> detail on _how_ it breaks that contract I don't think it's really possible to
>>> validate your fix here, fixes anything.
>>>
>>> ---
>>> bod
>>
>> Regards,
>> Vikash
> 
> I'll go through all of these links given here, thanks.
I would request you to go through the description putup by the reporter of this
OOB as well, i added in my earlier response. It provided a good background of
how the firmware response can led to this particular OOB, atleast that was the
source of OOB info for us.
Regards,
Vikash
> 
> Whatever the result of the review, this detail needs to go into the commit log
> so that a reviewer can reasonably read the problem description and evaluate
> against submitted code as a fix.
> 
> ---
> bod

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

[Bryan O'Donoghue]

On 03/03/2025 15:01, Vikash Garodia wrote:
> 
> On 3/3/2025 8:26 PM, Bryan O'Donoghue wrote:
>> On 03/03/2025 13:12, Vikash Garodia wrote:
>>>
>>> On 3/2/2025 9:26 PM, Bryan O'Donoghue wrote:
>>>> On 02/03/2025 11:58, Vedang Nagar wrote:
>>>>>>
>>>>>> The basic question : what is the lifetime of the data from RX interrupt to
>>>>>> consumption by another system agent, DSP, userspace, whatever ?
>>>>> As mentioned in [1], With the regular firmware, after RX interrupt the data
>>>>> can be considered as valid until next interrupt is raised, but with the rouge
>>>>> firmware, data can get invalid during the second read and our intention is to
>>>>> avoid out of bound access read because of such issues.
>>>>
>>>> This is definitely the part I don't compute.
>>>>
>>>> 1. RX interrupt
>>>> 2. Frame#0 Some amount of time data is always valid
>>> This is not correct. Its not the amount of time which determines the validity of
>>> the data, its the possibility of rogue firmware which, if incase, puts up the
>>> date in shared queue, would always be invalid, irrespective of time.
>>>
>>>> 3. RX interrupt - new data
>>>> 4. Frame#1 new data delivered into a buffer
>>>>
>>>> Are you describing a case between RX interrupts 1-3 or a case after 1-4?
>>>>
>>>> Why do we need to write code for rouge firmware anyway ?
>>> It is a way to prevent any possibility of OOB, similar to how any API does check
>>> for validity of any arguments passed to it, prior to processing.
>>>>
>>>> And the real question - if the data can be invalidated in the 1-3 window above
>>>> when is the safe time to snapshot that data ?
>>>>
>>>> We seem to have alot of submissions to deal with 'rouge' firmware without I
>>>> think properly describing the problem of the _expected_ data lifetime.
>>>>
>>>> So
>>>>
>>>> a) What is the expected data lifetime of an RX buffer between one
>>>>      RX IRQ and the next ?
>>>>      I hope the answer to this is - APSS owns the buffer.
>>>>      This is BTW usually the case in these types of asymmetric setups
>>>>      with a flag or some other kind of semaphore that indicates which
>>>>      side of the data-exchange owns the buffer.
>>>>
>>>> b) In this rouge - buggy - firmware case what is the scope of the
>>>>      potential race condition ?
>>>>
>>>>      What I'd really like to know here is why we have to seemingly
>>>>      memcpy() again and again in seemingly incongrous and not
>>>>      immediately obvious places in the code.
>>>>
>>>>      Would we not be better advised to do a memcpy() of the entire
>>>>      RX frame in the RX IRQ handler path if as you appear to me
>>>>      suggesting - the firmware can "race" with the APSS
>>>>      i.e. the data-buffer ownership flag either doesn't work
>>>>      or isn't respected by one side in the data-exchange.
>>>>
>>>> Can we please have a detailed description of the race condition here ?
>>> Below is the report which the reporter reported leading to OOB, let me know if
>>> you are unable to deduce the trail leading to OOB here.
>>>
>>> OOB read issue is in function event_seq_changed, please reference below code
>>> snippet:
>>>
>>> Buggy code snippet:
>>>
>>> static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
>>>           struct hfi_msg_event_notify_pkt *pkt)
>>> ...
>>> num_properties_changed = pkt->event_data2; //num_properties_changed is from
>>> message and is not validated.
>>> ...
>>> data_ptr = (u8 *)&pkt->ext_event_data[0];
>>> do {
>>>    ptype = *((u32 *)data_ptr);
>>>    switch (ptype) {
>>>    case HFI_PROPERTY_PARAM_FRAME_SIZE:
>>>     data_ptr += sizeof(u32);
>>>     frame_sz = (struct hfi_framesize *)data_ptr;
>>>     event.width = frame_sz->width;
>>> ...
>>>    }
>>>    num_properties_changed--;
>>> } while (num_properties_changed > 0);
>>> ```
>>> There is no validation against `num_properties_changed = pkt->event_data2`, so
>>> OOB read occurs.
>>>>
>>>> I don't doubt the new memcpy() makes sense to you but without this detailed
>>>> understanding of the underlying problem its virtually impossible to debate the
>>>> appropriate remediation - perhaps this patch you've submitted - or some other
>>>> solution.
>>>>
>>>> Sorry to dig into my trench here but, way more detail is needed.
>>>>
>>>>> [1]: https://lore.kernel.org/lkml/4cfc1fe1-2fab-4256-9ce2-
>>>>> b4a0aad1069e@linaro.org/T/#m5f1737b16e68f8b8fc1d75517356b6566d0ec619
>>>>>>
>>>>>> Why is it in this small specific window that the data can change but not
>>>>>> later ? What is the mechanism the data can change and how do the changes you
>>>>>> propose here address the data lifetime problem ?
>>>>> Currently this issue has been discovered by external researchers at this
>>>>> point, but if any such OOB issue is discovered at later point as well then we
>>>>> shall fix them as well.
>>>>
>>>> Right but, I'm looking for a detailed description of the problem.
>>>>
>>>> Can you describe from RX interrupt again what the expected data lifetime of the
>>>> RX frame is, which I hope we agree is until the next RX interrupt associated
>>>> with a given buffer with an ownership flag shared between firmware and APSS -
>>>> and then under what circumstances that "software contract" is being violated.
>>>>
>>>>> Also, with rougue firmware we cannot fix the data lifetime problem in my
>>>>> opinion, but atleast we can fix the out of bound issues.
>>>>>>
>>>>>> Without that context, I don't believe it is really possible to validate an
>>>>>> additional memcpy() here and there in the code as fixing anything.
>>>>> There is no additional memcpy() now in the v2 patch, but as part of the fix,
>>>>> we are just trying to retain the length of the packet which was being read in
>>>>> the first memcpy() to avoid the OOB read access.
>>>>
>>>> I can't make a suggestion because - personally speaking I still don't quite
>>>> understand the data-race you are describing.
>>> Go through the reports from the reporter, it was quite evident in leading upto
>>> OOB case.
>>> Putting up the sequence for you to go over the interrupt handling and message
>>> queue parsing of the packets from firmware
>>> 1.
>>> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_venus.c#L1082
>>> 2.
>>> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L816
>>> 3. event handling (this particular case)
>>> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L658
>>> 4.
>>> https://elixir.bootlin.com/linux/v6.14-rc4/source/drivers/media/platform/qcom/venus/hfi_msgs.c#L22
>>>
>>> the "struct hfi_msg_event_notify_pkt *pkt" pkt here is having the data read from
>>> shared queue.
>>>
>>>>
>>>> I get that you say the firmware is breaking the contract but, without more
>>>> detail on _how_ it breaks that contract I don't think it's really possible to
>>>> validate your fix here, fixes anything.
>>>>
>>>> ---
>>>> bod
>>>
>>> Regards,
>>> Vikash
>>
>> I'll go through all of these links given here, thanks.
> I would request you to go through the description putup by the reporter of this
> OOB as well, i added in my earlier response. It provided a good background of
> how the firmware response can led to this particular OOB, atleast that was the
> source of OOB info for us.
> Regards,
> Vikash
>>
>> Whatever the result of the review, this detail needs to go into the commit log
>> so that a reviewer can reasonably read the problem description and evaluate
>> against submitted code as a fix.
>>
>> ---
>> bod

Replied to the wrong patch.

There is no memcpy() in this patch - there was in patch #1 which has 
subsequently been dropped.

Anyway I'll address my comment there.

---
bod

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

[Bryan O'Donoghue]

On 15/02/2025 17:19, 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>
> ---
>   drivers/media/platform/qcom/venus/hfi_msgs.c | 72 ++++++++++++++++++++++++----
>   1 file changed, 62 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
> index 0a041b4db9efc549621de07dd13b4a3a37a70d11..2ad60a3fbfe0286de09a44664fc3b30259aa0368 100644
> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
> @@ -19,6 +19,16 @@
>   #define VER_STR_SZ		128
>   #define SMEM_IMG_OFFSET_VENUS	(14 * 128)
>   
> +static inline int increment_data_ptr(u8 *data_ptr, u32 *rem_bytes)
> +{
> +	if (*rem_bytes < sizeof(u32))
> +		return -EINVAL;
> +	data_ptr += sizeof(u32);
> +	*rem_bytes -= sizeof(u32);
> +
> +	return 0;
> +}
> +
>   static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
>   			      struct hfi_msg_event_notify_pkt *pkt)
>   {
> @@ -33,8 +43,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 +66,108 @@ 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;
> +	}

This doesn't make sense.

u32 rem_bytes;

if (rem_bytes - 4 < 0)


> +
>   	do {
>   		ptype = *((u32 *)data_ptr);
>   		switch (ptype) {
>   		case HFI_PROPERTY_PARAM_FRAME_SIZE:
> -			data_ptr += sizeof(u32);
> +			if (increment_data_ptr(data_ptr, &rem_bytes))
> +				break;
> +			if (rem_bytes < sizeof(struct hfi_framesize))
> +				break;

In every case you are return -EINVAL but not setting

inst->error = HFI_ERR_SESSION_INSUFFICIENT_RESOURCES;

surely that is a natural and logical outcome of running out of buffer 
and a fact you'd want to communicate outside of the driver, rather than 
silent failing in this switch ?

---
bod

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

[Bryan O'Donoghue]

On 15/02/2025 17:19, 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. 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..c124db8ac79d18f32289a690ee82145dc93daee6 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);
>   		}
> +		*(u32 *)pkt = dwords << 2;
>   	} else {
>   		/* bad packet received, dropping */
>   		new_rd_idx = qhdr->write_idx;
> 

This is confusing - where is the read

Your previous code

https://lore.kernel.org/lkml/20250104-venus-security-fixes-v1-1-9d0dd4594cb4@quicinc.com/

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

V1 then would have been:

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

V2 proposed:

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

+ *(u32 *)pkt = dwords << 2;

My comment wasn't about use of memcpy() it was about why we are doing this.

For example if new_rd_idx < qsize is true then we literally do

a) memcpy(pkt, rd_ptr, dwords << 2);
b) *(u32 *)pkt = dwords << 2;

and the question is why ? That is an unambiguous cast of pkt to the 
value of dwords << 2;

What is the scope of how the data can change from a to b ?

And why is the data considered potentially invalid @ the memcpy() but 
valid subsequent the cast ?

Assuming rd_ptr contains the length of dwords << 2 to begin with in the 
first 4 bytes - why is it necessary to make _really_ _really_ sure by 
restuffing the data ?

For example if *(u32 *)rd_ptr != dwords << 2 - why shouldn't we just 
throw the whole frame away as containing junk data ?

---
bod

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

[Dikshita Agarwal]

On 3/4/2025 7:05 PM, Bryan O'Donoghue wrote:
> On 15/02/2025 17:19, 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>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_msgs.c | 72
>> ++++++++++++++++++++++++----
>>   1 file changed, 62 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c
>> b/drivers/media/platform/qcom/venus/hfi_msgs.c
>> index
>> 0a041b4db9efc549621de07dd13b4a3a37a70d11..2ad60a3fbfe0286de09a44664fc3b30259aa0368 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
>> @@ -19,6 +19,16 @@
>>   #define VER_STR_SZ        128
>>   #define SMEM_IMG_OFFSET_VENUS    (14 * 128)
>>   +static inline int increment_data_ptr(u8 *data_ptr, u32 *rem_bytes)
>> +{
>> +    if (*rem_bytes < sizeof(u32))
>> +        return -EINVAL;
>> +    data_ptr += sizeof(u32);
>> +    *rem_bytes -= sizeof(u32);
>> +
>> +    return 0;
>> +}
>> +
>>   static void event_seq_changed(struct venus_core *core, struct
>> venus_inst *inst,
>>                     struct hfi_msg_event_notify_pkt *pkt)
>>   {
>> @@ -33,8 +43,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 +66,108 @@ 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;
>> +    }
> 
> This doesn't make sense.
> 
> u32 rem_bytes;
> 
> if (rem_bytes - 4 < 0)
> 
Would be better to replace it with
if (rem_bytes < sizeof(u32))

this make sure that rem_bytes contain some valid packet.
> 
>> +
>>       do {
>>           ptype = *((u32 *)data_ptr);
>>           switch (ptype) {
>>           case HFI_PROPERTY_PARAM_FRAME_SIZE:
>> -            data_ptr += sizeof(u32);
>> +            if (increment_data_ptr(data_ptr, &rem_bytes))
>> +                break;
>> +            if (rem_bytes < sizeof(struct hfi_framesize))
>> +                break;
> 
> In every case you are return -EINVAL but not setting
> 
> inst->error = HFI_ERR_SESSION_INSUFFICIENT_RESOURCES;
> 
> surely that is a natural and logical outcome of running out of buffer and a
> fact you'd want to communicate outside of the driver, rather than silent
> failing in this switch ?
> 
Make sense.
we should set the inst->error instead of silently breaking from loop.
Will handle this in next revision.

Thanks,
Dikshita
> ---
> bod
> 

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

[Vikash Garodia]

On 2/15/2025 10:49 PM, 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>
> ---
>  drivers/media/platform/qcom/venus/hfi_msgs.c | 72 ++++++++++++++++++++++++----
>  1 file changed, 62 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
> index 0a041b4db9efc549621de07dd13b4a3a37a70d11..2ad60a3fbfe0286de09a44664fc3b30259aa0368 100644
> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
> @@ -19,6 +19,16 @@
>  #define VER_STR_SZ		128
>  #define SMEM_IMG_OFFSET_VENUS	(14 * 128)
>  
> +static inline int increment_data_ptr(u8 *data_ptr, u32 *rem_bytes)
> +{
> +	if (*rem_bytes < sizeof(u32))
> +		return -EINVAL;
> +	data_ptr += sizeof(u32);
> +	*rem_bytes -= sizeof(u32);
> +
> +	return 0;
> +}
> +
>  static void event_seq_changed(struct venus_core *core, struct venus_inst *inst,
>  			      struct hfi_msg_event_notify_pkt *pkt)
>  {
> @@ -33,8 +43,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 +66,108 @@ 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:
> -			data_ptr += sizeof(u32);
> +			if (increment_data_ptr(data_ptr, &rem_bytes))
> +				break;
Instead of doing this for every switch, better to do it once before the switch.
> +			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);
for this one, lets store the size in a local variable, update for every switch,
and do the increment/decrement logic once at the end with that size. It avoids
doing the same logic for every switch.

Regards,
Vikash
>  			break;
>  		case HFI_PROPERTY_PARAM_PROFILE_LEVEL_CURRENT:
> -			data_ptr += sizeof(u32);
> +			if (increment_data_ptr(data_ptr, &rem_bytes))
> +				break;
> +			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:
> -			data_ptr += sizeof(u32);
> +			if (increment_data_ptr(data_ptr, &rem_bytes))
> +				break;
> +			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:
> -			data_ptr += sizeof(u32);
> +			if (increment_data_ptr(data_ptr, &rem_bytes))
> +				break;
> +			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:
> -			data_ptr += sizeof(u32);
> +			if (increment_data_ptr(data_ptr, &rem_bytes))
> +				break;
> +			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:
> -			data_ptr += sizeof(u32);
> +			if (increment_data_ptr(data_ptr, &rem_bytes))
> +				break;
> +			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:
> -			data_ptr += sizeof(u32);
> +			if (increment_data_ptr(data_ptr, &rem_bytes))
> +				break;
> +			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:
> -			data_ptr += sizeof(u32);
> +			if (increment_data_ptr(data_ptr, &rem_bytes))
> +				break;
> +			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:
> -			data_ptr += sizeof(u32);
> +			if (increment_data_ptr(data_ptr, &rem_bytes))
> +				break;
> +			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;
> 

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

[Dikshita Agarwal]

On 3/4/2025 7:15 PM, Bryan O'Donoghue wrote:
> On 15/02/2025 17:19, 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. 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..c124db8ac79d18f32289a690ee82145dc93daee6 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);
>>           }
>> +        *(u32 *)pkt = dwords << 2;
>>       } else {
>>           /* bad packet received, dropping */
>>           new_rd_idx = qhdr->write_idx;
>>
> 
> This is confusing - where is the read
> 
> Your previous code
> 
> https://lore.kernel.org/lkml/20250104-venus-security-fixes-v1-1-9d0dd4594cb4@quicinc.com/
> 
> memcpy(pkt, (u32 *)(queue->qmem.kva + (rd_idx << 2)), sizeof(u32));
> 
> V1 then would have been:
> 
> 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));
> 
> V2 proposed:
> 
> 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);
> }
> 
> + *(u32 *)pkt = dwords << 2;
> 
> My comment wasn't about use of memcpy() it was about why we are doing this.
> 
> For example if new_rd_idx < qsize is true then we literally do
> 
> a) memcpy(pkt, rd_ptr, dwords << 2);
> b) *(u32 *)pkt = dwords << 2;
> 
> and the question is why ? That is an unambiguous cast of pkt to the value
> of dwords << 2;
> 
> What is the scope of how the data can change from a to b ?
> 
> And why is the data considered potentially invalid @ the memcpy() but valid
> subsequent the cast ?
> 
The concern here is not about the value of dword which will be fixed once
we read the *rd_ptr first time and validate it.

The concern here is, with the data contents at rd_ptr which could be
tempered with after validation, because the memory is shared by firmware

So the real problem here is:
- driver read *rd_ptr, extract dwords = *rd_ptr >> 2, and validate it, lets
say this value is 10
- then copy dwords << 2 bytes from rd_ptr to a kernel buffer (pkt).
- But: the first 4 bytes at rd_ptr (which represent the size field again)
could have changed in the meantime — now saying 40 instead of the original 10.

security concern here is:
Even though the outer dwords is correct, the copied content might claim to
be bigger than what was validated — potentially tricking the rest of the
system into processing garbage or running past buffer bounds.

This is not a TOCTOU on the dwords, but a TOCTOU on the contents at rd_ptr,
where the firmware changes the actual data after size is checked, but
before it's copied.

> Assuming rd_ptr contains the length of dwords << 2 to begin with in the
> first 4 bytes - why is it necessary to make _really_ _really_ sure by
> restuffing the data ?
> 
> For example if *(u32 *)rd_ptr != dwords << 2 - why shouldn't we just throw
> the whole frame away as containing junk data ?

Agree with returning error during mismatch instead of forcefully
overwriting the content.

Thank,
Dikshita
> 
> ---
> bod
>