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

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

[Vikash Garodia]

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

Please review and share your feedback.

Validated on sc7180(v4), rb5(v6) and db410c(v1).

Changes in v4:
- fix an uninitialize variable(media ci)
- Link to v3: https://lore.kernel.org/r/20250128-venus_oob_2-v3-0-0144ecee68d8@quicinc.com

Changes in v3:
- update the packet parsing logic in hfi_parser. The utility parsing api 
  now returns the size of data parsed, accordingly the parser adjust the 
  remaining bytes, taking care of OOB scenario as well (Bryan)
- Link to v2: 
  https://lore.kernel.org/r/20241128-venus_oob_2-v2-0-483ae0a464b8@quicinc.com

Changes in v2:
- init_codec to always update with latest payload from firmware
  (Dmitry/Bryan)
- Rewrite the logic of packet parsing to consider payload size for
  different packet type (Bryan)
- Consider reading sfr data till available space (Dmitry)
- Add reviewed-by tags
- Link to v1: 
  https://lore.kernel.org/all/20241105-venus_oob-v1-0-8d4feedfe2bb@quicinc.com/

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

 drivers/media/platform/qcom/venus/hfi_parser.c | 96 +++++++++++++++++++-------
 drivers/media/platform/qcom/venus/hfi_venus.c  | 15 +++-
 2 files changed, 83 insertions(+), 28 deletions(-)
---
base-commit: c7ccf3683ac9746b263b0502255f5ce47f64fe0a
change-id: 20241115-venus_oob_2-21708239176a

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

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

[Vikash Garodia]

There is a possibility that init_codecs is invoked multiple times during
manipulated payload from video firmware. In such case, if codecs_count
can get incremented to value more than MAX_CODEC_NUM, there can be OOB
access. Reset the count so that it always starts from beginning.

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

diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
index 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..1cc17f3dc8948160ea6c3015d2c03e475b8aa29e 100644
--- a/drivers/media/platform/qcom/venus/hfi_parser.c
+++ b/drivers/media/platform/qcom/venus/hfi_parser.c
@@ -17,6 +17,7 @@ typedef void (*func)(struct hfi_plat_caps *cap, const void *data,
 static void init_codecs(struct venus_core *core)
 {
 	struct hfi_plat_caps *caps = core->caps, *cap;
+	core->codecs_count = 0;
 	unsigned long bit;
 
 	if (hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) > MAX_CODEC_NUM)

-- 
2.34.1

[PATCH v4 2/4] media: venus: hfi_parser: refactor hfi packet parsing logic

[Vikash Garodia]

words_count denotes the number of words in total payload, while data
points to payload of various property within it. When words_count
reaches last word, data can access memory beyond the total payload. This
can lead to OOB access. With this patch, the utility api for handling
individual properties now returns the size of data consumed. Accordingly
remaining bytes are calculated before parsing the payload, thereby
eliminates the OOB access possibilities.

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 | 95 +++++++++++++++++++-------
 1 file changed, 69 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
index 1cc17f3dc8948160ea6c3015d2c03e475b8aa29e..404c527329c5fa89ee885a6ad15620c9c90a99e4 100644
--- a/drivers/media/platform/qcom/venus/hfi_parser.c
+++ b/drivers/media/platform/qcom/venus/hfi_parser.c
@@ -63,7 +63,7 @@ fill_buf_mode(struct hfi_plat_caps *cap, const void *data, unsigned int num)
 		cap->cap_bufs_mode_dynamic = true;
 }
 
-static void
+static int
 parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
 {
 	struct hfi_buffer_alloc_mode_supported *mode = data;
@@ -71,7 +71,7 @@ parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
 	u32 *type;
 
 	if (num_entries > MAX_ALLOC_MODE_ENTRIES)
-		return;
+		return -EINVAL;
 
 	type = mode->data;
 
@@ -83,6 +83,8 @@ parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
 
 		type++;
 	}
+
+	return sizeof(*mode);
 }
 
 static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
@@ -97,7 +99,7 @@ static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
 	cap->num_pl += num;
 }
 
-static void
+static int
 parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, void *data)
 {
 	struct hfi_profile_level_supported *pl = data;
@@ -105,12 +107,14 @@ parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, void *data)
 	struct hfi_profile_level pl_arr[HFI_MAX_PROFILE_COUNT] = {};
 
 	if (pl->profile_count > HFI_MAX_PROFILE_COUNT)
-		return;
+		return -EINVAL;
 
 	memcpy(pl_arr, proflevel, pl->profile_count * sizeof(*proflevel));
 
 	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
 		       fill_profile_level, pl_arr, pl->profile_count);
+
+	return pl->profile_count * sizeof(*proflevel) + sizeof(u32);
 }
 
 static void
@@ -125,7 +129,7 @@ fill_caps(struct hfi_plat_caps *cap, const void *data, unsigned int num)
 	cap->num_caps += num;
 }
 
-static void
+static int
 parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
 {
 	struct hfi_capabilities *caps = data;
@@ -134,12 +138,14 @@ parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
 	struct hfi_capability caps_arr[MAX_CAP_ENTRIES] = {};
 
 	if (num_caps > MAX_CAP_ENTRIES)
-		return;
+		return -EINVAL;
 
 	memcpy(caps_arr, cap, num_caps * sizeof(*cap));
 
 	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
 		       fill_caps, caps_arr, num_caps);
+
+	return sizeof(*caps);
 }
 
 static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
@@ -154,7 +160,7 @@ static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
 	cap->num_fmts += num_fmts;
 }
 
-static void
+static int
 parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
 {
 	struct hfi_uncompressed_format_supported *fmt = data;
@@ -163,7 +169,8 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
 	struct raw_formats rawfmts[MAX_FMT_ENTRIES] = {};
 	u32 entries = fmt->format_entries;
 	unsigned int i = 0;
-	u32 num_planes;
+	u32 num_planes = 0;
+	u32 size;
 
 	while (entries) {
 		num_planes = pinfo->num_planes;
@@ -173,7 +180,7 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
 		i++;
 
 		if (i >= MAX_FMT_ENTRIES)
-			return;
+			return -EINVAL;
 
 		if (pinfo->num_planes > MAX_PLANES)
 			break;
@@ -185,9 +192,13 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
 
 	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
 		       fill_raw_fmts, rawfmts, i);
+	size = fmt->format_entries * (sizeof(*constr) * num_planes + 2 * sizeof(u32))
+		+ 2 * sizeof(u32);
+
+	return size;
 }
 
-static void parse_codecs(struct venus_core *core, void *data)
+static int parse_codecs(struct venus_core *core, void *data)
 {
 	struct hfi_codec_supported *codecs = data;
 
@@ -199,21 +210,27 @@ static void parse_codecs(struct venus_core *core, void *data)
 		core->dec_codecs &= ~HFI_VIDEO_CODEC_SPARK;
 		core->enc_codecs &= ~HFI_VIDEO_CODEC_HEVC;
 	}
+
+	return sizeof(*codecs);
 }
 
-static void parse_max_sessions(struct venus_core *core, const void *data)
+static int parse_max_sessions(struct venus_core *core, const void *data)
 {
 	const struct hfi_max_sessions_supported *sessions = data;
 
 	core->max_sessions_supported = sessions->max_sessions;
+
+	return sizeof(*sessions);
 }
 
-static void parse_codecs_mask(u32 *codecs, u32 *domain, void *data)
+static int parse_codecs_mask(u32 *codecs, u32 *domain, void *data)
 {
 	struct hfi_codec_mask_supported *mask = data;
 
 	*codecs = mask->codecs;
 	*domain = mask->video_domains;
+
+	return sizeof(*mask);
 }
 
 static void parser_init(struct venus_inst *inst, u32 *codecs, u32 *domain)
@@ -282,8 +299,9 @@ static int hfi_platform_parser(struct venus_core *core, struct venus_inst *inst)
 u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
 	       u32 size)
 {
-	unsigned int words_count = size >> 2;
-	u32 *word = buf, *data, codecs = 0, domain = 0;
+	u32 *words = buf, *payload, codecs = 0, domain = 0;
+	u32 *frame_size = buf + size;
+	u32 rem_bytes = size;
 	int ret;
 
 	ret = hfi_platform_parser(core, inst);
@@ -300,38 +318,63 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
 		memset(core->caps, 0, sizeof(core->caps));
 	}
 
-	while (words_count) {
-		data = word + 1;
+	while (words < frame_size) {
+		payload = words + 1;
 
-		switch (*word) {
+		switch (*words) {
 		case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
-			parse_codecs(core, data);
+			if (rem_bytes <= sizeof(struct hfi_codec_supported))
+				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
+
+			ret = parse_codecs(core, payload);
 			init_codecs(core);
 			break;
 		case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
-			parse_max_sessions(core, data);
+			if (rem_bytes <= sizeof(struct hfi_max_sessions_supported))
+				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
+
+			ret = parse_max_sessions(core, payload);
 			break;
 		case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
-			parse_codecs_mask(&codecs, &domain, data);
+			if (rem_bytes <= sizeof(struct hfi_codec_mask_supported))
+				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
+
+			ret = parse_codecs_mask(&codecs, &domain, payload);
 			break;
 		case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
-			parse_raw_formats(core, codecs, domain, data);
+			if (rem_bytes <= sizeof(struct hfi_uncompressed_format_supported))
+				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
+
+			ret = parse_raw_formats(core, codecs, domain, payload);
 			break;
 		case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
-			parse_caps(core, codecs, domain, data);
+			if (rem_bytes <= sizeof(struct hfi_capabilities))
+				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
+
+			ret = parse_caps(core, codecs, domain, payload);
 			break;
 		case HFI_PROPERTY_PARAM_PROFILE_LEVEL_SUPPORTED:
-			parse_profile_level(core, codecs, domain, data);
+			if (rem_bytes <= sizeof(struct hfi_profile_level_supported))
+				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
+
+			ret = parse_profile_level(core, codecs, domain, payload);
 			break;
 		case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE_SUPPORTED:
-			parse_alloc_mode(core, codecs, domain, data);
+			if (rem_bytes <= sizeof(struct hfi_buffer_alloc_mode_supported))
+				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
+
+			ret = parse_alloc_mode(core, codecs, domain, payload);
 			break;
 		default:
+			ret = sizeof(u32);
 			break;
 		}
 
-		word++;
-		words_count--;
+		if (ret < 0)
+			return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
+
+		words += ret / sizeof(u32);
+		rem_bytes -= ret;
 	}
 
 	if (!core->max_sessions_supported)

-- 
2.34.1

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

[Vikash Garodia]

qsize represents size of shared queued between driver and video
firmware. Firmware can modify this value to an invalid large value. In
such situation, empty_space will be bigger than the space actually
available. Since new_wr_idx is not checked, so the following code will
result in an OOB write.
...
qsize = qhdr->q_size

if (wr_idx >= rd_idx)
 empty_space = qsize - (wr_idx - rd_idx)
....
if (new_wr_idx < qsize) {
 memcpy(wr_ptr, packet, dwords << 2) --> OOB write

Add check to ensure qsize is within the allocated size while
reading and writing packets into the queue.

Cc: stable@vger.kernel.org
Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index f9437b6412b91c2483670a2b11f4fd43f3206404..6b615270c5dae470c6fad408c9b5bc037883e56e 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -187,6 +187,9 @@ static int venus_write_queue(struct venus_hfi_device *hdev,
 	/* ensure rd/wr indices's are read from memory */
 	rmb();
 
+	if (qsize > IFACEQ_QUEUE_SIZE / 4)
+		return -EINVAL;
+
 	if (wr_idx >= rd_idx)
 		empty_space = qsize - (wr_idx - rd_idx);
 	else
@@ -255,6 +258,9 @@ static int venus_read_queue(struct venus_hfi_device *hdev,
 	wr_idx = qhdr->write_idx;
 	qsize = qhdr->q_size;
 
+	if (qsize > IFACEQ_QUEUE_SIZE / 4)
+		return -EINVAL;
+
 	/* make sure data is valid before using it */
 	rmb();
 

-- 
2.34.1

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

[Vikash Garodia]

sfr->buf_size is in shared memory and can be modified by malicious user.
OOB write is possible when the size is made higher than actual sfr data
buffer. Cap the size to allocated size for such cases.

Cc: stable@vger.kernel.org
Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 6b615270c5dae470c6fad408c9b5bc037883e56e..c3113420d266e61fcab44688580288d7408b50f4 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -1041,18 +1041,23 @@ static void venus_sfr_print(struct venus_hfi_device *hdev)
 {
 	struct device *dev = hdev->core->dev;
 	struct hfi_sfr *sfr = hdev->sfr.kva;
+	u32 size;
 	void *p;
 
 	if (!sfr)
 		return;
 
-	p = memchr(sfr->data, '\0', sfr->buf_size);
+	size = sfr->buf_size;
+	if (size > ALIGNED_SFR_SIZE)
+		size = ALIGNED_SFR_SIZE;
+
+	p = memchr(sfr->data, '\0', size);
 	/*
 	 * SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates
 	 * that Venus is in the process of crashing.
 	 */
 	if (!p)
-		sfr->data[sfr->buf_size - 1] = '\0';
+		sfr->data[size - 1] = '\0';
 
 	dev_err_ratelimited(dev, "SFR message from FW: %s\n", sfr->data);
 }

-- 
2.34.1

Re: [PATCH v4 2/4] media: venus: hfi_parser: refactor hfi packet parsing logic

[Bryan O'Donoghue]

On 07/02/2025 08:24, Vikash Garodia wrote:
> words_count denotes the number of words in total payload, while data
> points to payload of various property within it. When words_count
> reaches last word, data can access memory beyond the total payload. This
> can lead to OOB access. With this patch, the utility api for handling
> individual properties now returns the size of data consumed. Accordingly
> remaining bytes are calculated before parsing the payload, thereby
> eliminates the OOB access possibilities.
> 
> 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 | 95 +++++++++++++++++++-------
>   1 file changed, 69 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 1cc17f3dc8948160ea6c3015d2c03e475b8aa29e..404c527329c5fa89ee885a6ad15620c9c90a99e4 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -63,7 +63,7 @@ fill_buf_mode(struct hfi_plat_caps *cap, const void *data, unsigned int num)
>   		cap->cap_bufs_mode_dynamic = true;
>   }
>   
> -static void
> +static int
>   parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>   {
>   	struct hfi_buffer_alloc_mode_supported *mode = data;
> @@ -71,7 +71,7 @@ parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>   	u32 *type;
>   
>   	if (num_entries > MAX_ALLOC_MODE_ENTRIES)
> -		return;
> +		return -EINVAL;
>   
>   	type = mode->data;
>   
> @@ -83,6 +83,8 @@ parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>   
>   		type++;
>   	}
> +
> +	return sizeof(*mode);
>   }
>   
>   static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
> @@ -97,7 +99,7 @@ static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
>   	cap->num_pl += num;
>   }
>   
> -static void
> +static int
>   parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, void *data)
>   {
>   	struct hfi_profile_level_supported *pl = data;
> @@ -105,12 +107,14 @@ parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, void *data)
>   	struct hfi_profile_level pl_arr[HFI_MAX_PROFILE_COUNT] = {};
>   
>   	if (pl->profile_count > HFI_MAX_PROFILE_COUNT)
> -		return;
> +		return -EINVAL;
>   
>   	memcpy(pl_arr, proflevel, pl->profile_count * sizeof(*proflevel));
>   
>   	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>   		       fill_profile_level, pl_arr, pl->profile_count);
> +
> +	return pl->profile_count * sizeof(*proflevel) + sizeof(u32);

I'm not going to check your maths here and will instead assume you've 
corrected your own homework and these calculations are correct..

>   }
>   
>   static void
> @@ -125,7 +129,7 @@ fill_caps(struct hfi_plat_caps *cap, const void *data, unsigned int num)
>   	cap->num_caps += num;
>   }
>   
> -static void
> +static int
>   parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
>   {
>   	struct hfi_capabilities *caps = data;
> @@ -134,12 +138,14 @@ parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
>   	struct hfi_capability caps_arr[MAX_CAP_ENTRIES] = {};
>   
>   	if (num_caps > MAX_CAP_ENTRIES)
> -		return;
> +		return -EINVAL;
>   
>   	memcpy(caps_arr, cap, num_caps * sizeof(*cap));
>   
>   	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>   		       fill_caps, caps_arr, num_caps);
> +
> +	return sizeof(*caps);
>   }
>   
>   static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
> @@ -154,7 +160,7 @@ static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
>   	cap->num_fmts += num_fmts;
>   }
>   
> -static void
> +static int
>   parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>   {
>   	struct hfi_uncompressed_format_supported *fmt = data;
> @@ -163,7 +169,8 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>   	struct raw_formats rawfmts[MAX_FMT_ENTRIES] = {};
>   	u32 entries = fmt->format_entries;
>   	unsigned int i = 0;
> -	u32 num_planes;
> +	u32 num_planes = 0;
> +	u32 size;
>   
>   	while (entries) {
>   		num_planes = pinfo->num_planes;
> @@ -173,7 +180,7 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>   		i++;
>   
>   		if (i >= MAX_FMT_ENTRIES)
> -			return;
> +			return -EINVAL;
>   
>   		if (pinfo->num_planes > MAX_PLANES)
>   			break;
> @@ -185,9 +192,13 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>   
>   	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>   		       fill_raw_fmts, rawfmts, i);
> +	size = fmt->format_entries * (sizeof(*constr) * num_planes + 2 * sizeof(u32))
> +		+ 2 * sizeof(u32);
> +
> +	return size;
>   }
>   
> -static void parse_codecs(struct venus_core *core, void *data)
> +static int parse_codecs(struct venus_core *core, void *data)
>   {
>   	struct hfi_codec_supported *codecs = data;
>   
> @@ -199,21 +210,27 @@ static void parse_codecs(struct venus_core *core, void *data)
>   		core->dec_codecs &= ~HFI_VIDEO_CODEC_SPARK;
>   		core->enc_codecs &= ~HFI_VIDEO_CODEC_HEVC;
>   	}
> +
> +	return sizeof(*codecs);
>   }
>   
> -static void parse_max_sessions(struct venus_core *core, const void *data)
> +static int parse_max_sessions(struct venus_core *core, const void *data)
>   {
>   	const struct hfi_max_sessions_supported *sessions = data;
>   
>   	core->max_sessions_supported = sessions->max_sessions;
> +
> +	return sizeof(*sessions);
>   }
>   
> -static void parse_codecs_mask(u32 *codecs, u32 *domain, void *data)
> +static int parse_codecs_mask(u32 *codecs, u32 *domain, void *data)
>   {
>   	struct hfi_codec_mask_supported *mask = data;
>   
>   	*codecs = mask->codecs;
>   	*domain = mask->video_domains;
> +
> +	return sizeof(*mask);
>   }
>   
>   static void parser_init(struct venus_inst *inst, u32 *codecs, u32 *domain)
> @@ -282,8 +299,9 @@ static int hfi_platform_parser(struct venus_core *core, struct venus_inst *inst)
>   u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>   	       u32 size)
>   {
> -	unsigned int words_count = size >> 2;
> -	u32 *word = buf, *data, codecs = 0, domain = 0;
> +	u32 *words = buf, *payload, codecs = 0, domain = 0;
> +	u32 *frame_size = buf + size;
> +	u32 rem_bytes = size;
>   	int ret;
>   
>   	ret = hfi_platform_parser(core, inst);
> @@ -300,38 +318,63 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>   		memset(core->caps, 0, sizeof(core->caps));
>   	}
>   
> -	while (words_count) {
> -		data = word + 1;
> +	while (words < frame_size) {
> +		payload = words + 1;
>   
> -		switch (*word) {
> +		switch (*words) {
>   		case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
> -			parse_codecs(core, data);
> +			if (rem_bytes <= sizeof(struct hfi_codec_supported))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_codecs(core, payload);
>   			init_codecs(core);
>   			break;
>   		case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
> -			parse_max_sessions(core, data);
> +			if (rem_bytes <= sizeof(struct hfi_max_sessions_supported))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_max_sessions(core, payload);
>   			break;
>   		case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
> -			parse_codecs_mask(&codecs, &domain, data);
> +			if (rem_bytes <= sizeof(struct hfi_codec_mask_supported))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_codecs_mask(&codecs, &domain, payload);
>   			break;
>   		case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
> -			parse_raw_formats(core, codecs, domain, data);
> +			if (rem_bytes <= sizeof(struct hfi_uncompressed_format_supported))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_raw_formats(core, codecs, domain, payload);
>   			break;
>   		case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
> -			parse_caps(core, codecs, domain, data);
> +			if (rem_bytes <= sizeof(struct hfi_capabilities))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_caps(core, codecs, domain, payload);
>   			break;
>   		case HFI_PROPERTY_PARAM_PROFILE_LEVEL_SUPPORTED:
> -			parse_profile_level(core, codecs, domain, data);
> +			if (rem_bytes <= sizeof(struct hfi_profile_level_supported))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_profile_level(core, codecs, domain, payload);
>   			break;
>   		case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE_SUPPORTED:
> -			parse_alloc_mode(core, codecs, domain, data);
> +			if (rem_bytes <= sizeof(struct hfi_buffer_alloc_mode_supported))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_alloc_mode(core, codecs, domain, payload);
>   			break;
>   		default:
> +			ret = sizeof(u32);
>   			break;
>   		}
>   
> -		word++;
> -		words_count--;
> +		if (ret < 0)
> +			return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +		words += ret / sizeof(u32);
> +		rem_bytes -= ret;
>   	}
>   
>   	if (!core->max_sessions_supported)
> 

LGTM

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

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

[Bryan O'Donoghue]

On 07/02/2025 08:24, Vikash Garodia 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.
> 
> Please review and share your feedback.
> 
> Validated on sc7180(v4), rb5(v6) and db410c(v1).
> 
> Changes in v4:
> - fix an uninitialize variable(media ci)
> - Link to v3: https://lore.kernel.org/r/20250128-venus_oob_2-v3-0-0144ecee68d8@quicinc.com
> 
> Changes in v3:
> - update the packet parsing logic in hfi_parser. The utility parsing api
>    now returns the size of data parsed, accordingly the parser adjust the
>    remaining bytes, taking care of OOB scenario as well (Bryan)
> - Link to v2:
>    https://lore.kernel.org/r/20241128-venus_oob_2-v2-0-483ae0a464b8@quicinc.com
> 
> Changes in v2:
> - init_codec to always update with latest payload from firmware
>    (Dmitry/Bryan)
> - Rewrite the logic of packet parsing to consider payload size for
>    different packet type (Bryan)
> - Consider reading sfr data till available space (Dmitry)
> - Add reviewed-by tags
> - Link to v1:
>    https://lore.kernel.org/all/20241105-venus_oob-v1-0-8d4feedfe2bb@quicinc.com/
> 
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
> Vikash Garodia (4):
>        media: venus: hfi_parser: add check to avoid out of bound access
>        media: venus: hfi_parser: refactor hfi packet parsing logic
>        media: venus: hfi: add check to handle incorrect queue size
>        media: venus: hfi: add a check to handle OOB in sfr region
> 
>   drivers/media/platform/qcom/venus/hfi_parser.c | 96 +++++++++++++++++++-------
>   drivers/media/platform/qcom/venus/hfi_venus.c  | 15 +++-
>   2 files changed, 83 insertions(+), 28 deletions(-)
> ---
> base-commit: c7ccf3683ac9746b263b0502255f5ce47f64fe0a
> change-id: 20241115-venus_oob_2-21708239176a
> 
> Best regards,

I think this series is ready for merge.

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

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

[Hans Verkuil]

On 2/7/25 09:24, Vikash Garodia wrote:
> There is a possibility that init_codecs is invoked multiple times during
> manipulated payload from video firmware. In such case, if codecs_count
> can get incremented to value more than MAX_CODEC_NUM, there can be OOB
> access. Reset the count so that it always starts from beginning.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>  drivers/media/platform/qcom/venus/hfi_parser.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..1cc17f3dc8948160ea6c3015d2c03e475b8aa29e 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -17,6 +17,7 @@ typedef void (*func)(struct hfi_plat_caps *cap, const void *data,
>  static void init_codecs(struct venus_core *core)
>  {
>  	struct hfi_plat_caps *caps = core->caps, *cap;
> +	core->codecs_count = 0;

This really should be moved down to before the 'if'. There is no reason to mix the assignment
with variable declarations.

>  	unsigned long bit;
>  
>  	if (hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) > MAX_CODEC_NUM)
> 

Regards,

	Hans

Re: [PATCH v4 2/4] media: venus: hfi_parser: refactor hfi packet parsing logic

[Hans Verkuil]

On 2/7/25 09:24, Vikash Garodia wrote:
> words_count denotes the number of words in total payload, while data
> points to payload of various property within it. When words_count
> reaches last word, data can access memory beyond the total payload. This
> can lead to OOB access. With this patch, the utility api for handling
> individual properties now returns the size of data consumed. Accordingly
> remaining bytes are calculated before parsing the payload, thereby
> eliminates the OOB access possibilities.
> 
> 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 | 95 +++++++++++++++++++-------
>  1 file changed, 69 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index 1cc17f3dc8948160ea6c3015d2c03e475b8aa29e..404c527329c5fa89ee885a6ad15620c9c90a99e4 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -63,7 +63,7 @@ fill_buf_mode(struct hfi_plat_caps *cap, const void *data, unsigned int num)
>  		cap->cap_bufs_mode_dynamic = true;
>  }
>  
> -static void
> +static int
>  parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  {
>  	struct hfi_buffer_alloc_mode_supported *mode = data;
> @@ -71,7 +71,7 @@ parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  	u32 *type;
>  
>  	if (num_entries > MAX_ALLOC_MODE_ENTRIES)
> -		return;
> +		return -EINVAL;
>  
>  	type = mode->data;
>  
> @@ -83,6 +83,8 @@ parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  
>  		type++;
>  	}
> +
> +	return sizeof(*mode);
>  }
>  
>  static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
> @@ -97,7 +99,7 @@ static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
>  	cap->num_pl += num;
>  }
>  
> -static void
> +static int
>  parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  {
>  	struct hfi_profile_level_supported *pl = data;
> @@ -105,12 +107,14 @@ parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  	struct hfi_profile_level pl_arr[HFI_MAX_PROFILE_COUNT] = {};
>  
>  	if (pl->profile_count > HFI_MAX_PROFILE_COUNT)
> -		return;
> +		return -EINVAL;
>  
>  	memcpy(pl_arr, proflevel, pl->profile_count * sizeof(*proflevel));
>  
>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>  		       fill_profile_level, pl_arr, pl->profile_count);
> +
> +	return pl->profile_count * sizeof(*proflevel) + sizeof(u32);
>  }
>  
>  static void
> @@ -125,7 +129,7 @@ fill_caps(struct hfi_plat_caps *cap, const void *data, unsigned int num)
>  	cap->num_caps += num;
>  }
>  
> -static void
> +static int
>  parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  {
>  	struct hfi_capabilities *caps = data;
> @@ -134,12 +138,14 @@ parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  	struct hfi_capability caps_arr[MAX_CAP_ENTRIES] = {};
>  
>  	if (num_caps > MAX_CAP_ENTRIES)
> -		return;
> +		return -EINVAL;
>  
>  	memcpy(caps_arr, cap, num_caps * sizeof(*cap));
>  
>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>  		       fill_caps, caps_arr, num_caps);
> +
> +	return sizeof(*caps);
>  }
>  
>  static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
> @@ -154,7 +160,7 @@ static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
>  	cap->num_fmts += num_fmts;
>  }
>  
> -static void
> +static int
>  parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  {
>  	struct hfi_uncompressed_format_supported *fmt = data;
> @@ -163,7 +169,8 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  	struct raw_formats rawfmts[MAX_FMT_ENTRIES] = {};
>  	u32 entries = fmt->format_entries;
>  	unsigned int i = 0;
> -	u32 num_planes;
> +	u32 num_planes = 0;
> +	u32 size;
>  
>  	while (entries) {
>  		num_planes = pinfo->num_planes;
> @@ -173,7 +180,7 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  		i++;
>  
>  		if (i >= MAX_FMT_ENTRIES)
> -			return;
> +			return -EINVAL;
>  
>  		if (pinfo->num_planes > MAX_PLANES)
>  			break;
> @@ -185,9 +192,13 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  
>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>  		       fill_raw_fmts, rawfmts, i);
> +	size = fmt->format_entries * (sizeof(*constr) * num_planes + 2 * sizeof(u32))
> +		+ 2 * sizeof(u32);
> +
> +	return size;
>  }
>  
> -static void parse_codecs(struct venus_core *core, void *data)
> +static int parse_codecs(struct venus_core *core, void *data)
>  {
>  	struct hfi_codec_supported *codecs = data;
>  
> @@ -199,21 +210,27 @@ static void parse_codecs(struct venus_core *core, void *data)
>  		core->dec_codecs &= ~HFI_VIDEO_CODEC_SPARK;
>  		core->enc_codecs &= ~HFI_VIDEO_CODEC_HEVC;
>  	}
> +
> +	return sizeof(*codecs);
>  }
>  
> -static void parse_max_sessions(struct venus_core *core, const void *data)
> +static int parse_max_sessions(struct venus_core *core, const void *data)
>  {
>  	const struct hfi_max_sessions_supported *sessions = data;
>  
>  	core->max_sessions_supported = sessions->max_sessions;
> +
> +	return sizeof(*sessions);
>  }
>  
> -static void parse_codecs_mask(u32 *codecs, u32 *domain, void *data)
> +static int parse_codecs_mask(u32 *codecs, u32 *domain, void *data)
>  {
>  	struct hfi_codec_mask_supported *mask = data;
>  
>  	*codecs = mask->codecs;
>  	*domain = mask->video_domains;
> +
> +	return sizeof(*mask);
>  }
>  
>  static void parser_init(struct venus_inst *inst, u32 *codecs, u32 *domain)
> @@ -282,8 +299,9 @@ static int hfi_platform_parser(struct venus_core *core, struct venus_inst *inst)
>  u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>  	       u32 size)
>  {
> -	unsigned int words_count = size >> 2;
> -	u32 *word = buf, *data, codecs = 0, domain = 0;
> +	u32 *words = buf, *payload, codecs = 0, domain = 0;
> +	u32 *frame_size = buf + size;
> +	u32 rem_bytes = size;
>  	int ret;
>  
>  	ret = hfi_platform_parser(core, inst);
> @@ -300,38 +318,63 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>  		memset(core->caps, 0, sizeof(core->caps));
>  	}
>  
> -	while (words_count) {
> -		data = word + 1;
> +	while (words < frame_size) {
> +		payload = words + 1;
>  
> -		switch (*word) {
> +		switch (*words) {
>  		case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
> -			parse_codecs(core, data);
> +			if (rem_bytes <= sizeof(struct hfi_codec_supported))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_codecs(core, payload);
>  			init_codecs(core);

Does it make sense to call init_codecs if parse_codecs returned an error?
It certainly looks weird, so even if it is OK, perhaps a comment might be
useful.

>  			break;
>  		case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
> -			parse_max_sessions(core, data);
> +			if (rem_bytes <= sizeof(struct hfi_max_sessions_supported))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_max_sessions(core, payload);
>  			break;
>  		case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
> -			parse_codecs_mask(&codecs, &domain, data);
> +			if (rem_bytes <= sizeof(struct hfi_codec_mask_supported))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_codecs_mask(&codecs, &domain, payload);
>  			break;
>  		case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
> -			parse_raw_formats(core, codecs, domain, data);
> +			if (rem_bytes <= sizeof(struct hfi_uncompressed_format_supported))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_raw_formats(core, codecs, domain, payload);
>  			break;
>  		case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
> -			parse_caps(core, codecs, domain, data);
> +			if (rem_bytes <= sizeof(struct hfi_capabilities))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_caps(core, codecs, domain, payload);
>  			break;
>  		case HFI_PROPERTY_PARAM_PROFILE_LEVEL_SUPPORTED:
> -			parse_profile_level(core, codecs, domain, data);
> +			if (rem_bytes <= sizeof(struct hfi_profile_level_supported))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_profile_level(core, codecs, domain, payload);
>  			break;
>  		case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE_SUPPORTED:
> -			parse_alloc_mode(core, codecs, domain, data);
> +			if (rem_bytes <= sizeof(struct hfi_buffer_alloc_mode_supported))
> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +			ret = parse_alloc_mode(core, codecs, domain, payload);
>  			break;
>  		default:
> +			ret = sizeof(u32);
>  			break;
>  		}
>  
> -		word++;
> -		words_count--;
> +		if (ret < 0)
> +			return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
> +
> +		words += ret / sizeof(u32);

Would it make sense to check and warn if ret is not a multiple of sizeof(u32)?
Up to you, just an idea.

> +		rem_bytes -= ret;
>  	}
>  
>  	if (!core->max_sessions_supported)
> 

Regards,

	Hans

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

[Hans Verkuil]

On 2/7/25 09:24, Vikash Garodia wrote:
> sfr->buf_size is in shared memory and can be modified by malicious user.
> OOB write is possible when the size is made higher than actual sfr data
> buffer. Cap the size to allocated size for such cases.
> 
> Cc: stable@vger.kernel.org
> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> ---
>  drivers/media/platform/qcom/venus/hfi_venus.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 6b615270c5dae470c6fad408c9b5bc037883e56e..c3113420d266e61fcab44688580288d7408b50f4 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -1041,18 +1041,23 @@ static void venus_sfr_print(struct venus_hfi_device *hdev)
>  {
>  	struct device *dev = hdev->core->dev;
>  	struct hfi_sfr *sfr = hdev->sfr.kva;
> +	u32 size;
>  	void *p;
>  
>  	if (!sfr)
>  		return;
>  
> -	p = memchr(sfr->data, '\0', sfr->buf_size);
> +	size = sfr->buf_size;

If this is ever 0...

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

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

Regards,

	Hans

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

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

[Vikash Garodia]

On 2/20/2025 8:46 PM, Hans Verkuil wrote:
> On 2/7/25 09:24, Vikash Garodia wrote:
>> There is a possibility that init_codecs is invoked multiple times during
>> manipulated payload from video firmware. In such case, if codecs_count
>> can get incremented to value more than MAX_CODEC_NUM, there can be OOB
>> access. Reset the count so that it always starts from beginning.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
>>  drivers/media/platform/qcom/venus/hfi_parser.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
>> index 3df241dc3a118bcdeb2c28a6ffdb907b644d5653..1cc17f3dc8948160ea6c3015d2c03e475b8aa29e 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>> @@ -17,6 +17,7 @@ typedef void (*func)(struct hfi_plat_caps *cap, const void *data,
>>  static void init_codecs(struct venus_core *core)
>>  {
>>  	struct hfi_plat_caps *caps = core->caps, *cap;
>> +	core->codecs_count = 0;
> 
> This really should be moved down to before the 'if'. There is no reason to mix the assignment
> with variable declarations.
Thats correct, will move it below.
> 
>>  	unsigned long bit;
>>  
>>  	if (hweight_long(core->dec_codecs) + hweight_long(core->enc_codecs) > MAX_CODEC_NUM)
>>
> 
> Regards,
> 
> 	Hans
Regards,
Vikash

Re: [PATCH v4 2/4] media: venus: hfi_parser: refactor hfi packet parsing logic

[Vikash Garodia]

On 2/20/2025 8:50 PM, Hans Verkuil wrote:
> On 2/7/25 09:24, Vikash Garodia wrote:
>> words_count denotes the number of words in total payload, while data
>> points to payload of various property within it. When words_count
>> reaches last word, data can access memory beyond the total payload. This
>> can lead to OOB access. With this patch, the utility api for handling
>> individual properties now returns the size of data consumed. Accordingly
>> remaining bytes are calculated before parsing the payload, thereby
>> eliminates the OOB access possibilities.
>>
>> 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 | 95 +++++++++++++++++++-------
>>  1 file changed, 69 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
>> index 1cc17f3dc8948160ea6c3015d2c03e475b8aa29e..404c527329c5fa89ee885a6ad15620c9c90a99e4 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>> @@ -63,7 +63,7 @@ fill_buf_mode(struct hfi_plat_caps *cap, const void *data, unsigned int num)
>>  		cap->cap_bufs_mode_dynamic = true;
>>  }
>>  
>> -static void
>> +static int
>>  parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>  {
>>  	struct hfi_buffer_alloc_mode_supported *mode = data;
>> @@ -71,7 +71,7 @@ parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>  	u32 *type;
>>  
>>  	if (num_entries > MAX_ALLOC_MODE_ENTRIES)
>> -		return;
>> +		return -EINVAL;
>>  
>>  	type = mode->data;
>>  
>> @@ -83,6 +83,8 @@ parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>  
>>  		type++;
>>  	}
>> +
>> +	return sizeof(*mode);
>>  }
>>  
>>  static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
>> @@ -97,7 +99,7 @@ static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
>>  	cap->num_pl += num;
>>  }
>>  
>> -static void
>> +static int
>>  parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>  {
>>  	struct hfi_profile_level_supported *pl = data;
>> @@ -105,12 +107,14 @@ parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>  	struct hfi_profile_level pl_arr[HFI_MAX_PROFILE_COUNT] = {};
>>  
>>  	if (pl->profile_count > HFI_MAX_PROFILE_COUNT)
>> -		return;
>> +		return -EINVAL;
>>  
>>  	memcpy(pl_arr, proflevel, pl->profile_count * sizeof(*proflevel));
>>  
>>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>>  		       fill_profile_level, pl_arr, pl->profile_count);
>> +
>> +	return pl->profile_count * sizeof(*proflevel) + sizeof(u32);
>>  }
>>  
>>  static void
>> @@ -125,7 +129,7 @@ fill_caps(struct hfi_plat_caps *cap, const void *data, unsigned int num)
>>  	cap->num_caps += num;
>>  }
>>  
>> -static void
>> +static int
>>  parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>  {
>>  	struct hfi_capabilities *caps = data;
>> @@ -134,12 +138,14 @@ parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>  	struct hfi_capability caps_arr[MAX_CAP_ENTRIES] = {};
>>  
>>  	if (num_caps > MAX_CAP_ENTRIES)
>> -		return;
>> +		return -EINVAL;
>>  
>>  	memcpy(caps_arr, cap, num_caps * sizeof(*cap));
>>  
>>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>>  		       fill_caps, caps_arr, num_caps);
>> +
>> +	return sizeof(*caps);
>>  }
>>  
>>  static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
>> @@ -154,7 +160,7 @@ static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
>>  	cap->num_fmts += num_fmts;
>>  }
>>  
>> -static void
>> +static int
>>  parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>  {
>>  	struct hfi_uncompressed_format_supported *fmt = data;
>> @@ -163,7 +169,8 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>  	struct raw_formats rawfmts[MAX_FMT_ENTRIES] = {};
>>  	u32 entries = fmt->format_entries;
>>  	unsigned int i = 0;
>> -	u32 num_planes;
>> +	u32 num_planes = 0;
>> +	u32 size;
>>  
>>  	while (entries) {
>>  		num_planes = pinfo->num_planes;
>> @@ -173,7 +180,7 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>  		i++;
>>  
>>  		if (i >= MAX_FMT_ENTRIES)
>> -			return;
>> +			return -EINVAL;
>>  
>>  		if (pinfo->num_planes > MAX_PLANES)
>>  			break;
>> @@ -185,9 +192,13 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>  
>>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>>  		       fill_raw_fmts, rawfmts, i);
>> +	size = fmt->format_entries * (sizeof(*constr) * num_planes + 2 * sizeof(u32))
>> +		+ 2 * sizeof(u32);
>> +
>> +	return size;
>>  }
>>  
>> -static void parse_codecs(struct venus_core *core, void *data)
>> +static int parse_codecs(struct venus_core *core, void *data)
>>  {
>>  	struct hfi_codec_supported *codecs = data;
>>  
>> @@ -199,21 +210,27 @@ static void parse_codecs(struct venus_core *core, void *data)
>>  		core->dec_codecs &= ~HFI_VIDEO_CODEC_SPARK;
>>  		core->enc_codecs &= ~HFI_VIDEO_CODEC_HEVC;
>>  	}
>> +
>> +	return sizeof(*codecs);
>>  }
>>  
>> -static void parse_max_sessions(struct venus_core *core, const void *data)
>> +static int parse_max_sessions(struct venus_core *core, const void *data)
>>  {
>>  	const struct hfi_max_sessions_supported *sessions = data;
>>  
>>  	core->max_sessions_supported = sessions->max_sessions;
>> +
>> +	return sizeof(*sessions);
>>  }
>>  
>> -static void parse_codecs_mask(u32 *codecs, u32 *domain, void *data)
>> +static int parse_codecs_mask(u32 *codecs, u32 *domain, void *data)
>>  {
>>  	struct hfi_codec_mask_supported *mask = data;
>>  
>>  	*codecs = mask->codecs;
>>  	*domain = mask->video_domains;
>> +
>> +	return sizeof(*mask);
>>  }
>>  
>>  static void parser_init(struct venus_inst *inst, u32 *codecs, u32 *domain)
>> @@ -282,8 +299,9 @@ static int hfi_platform_parser(struct venus_core *core, struct venus_inst *inst)
>>  u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>>  	       u32 size)
>>  {
>> -	unsigned int words_count = size >> 2;
>> -	u32 *word = buf, *data, codecs = 0, domain = 0;
>> +	u32 *words = buf, *payload, codecs = 0, domain = 0;
>> +	u32 *frame_size = buf + size;
>> +	u32 rem_bytes = size;
>>  	int ret;
>>  
>>  	ret = hfi_platform_parser(core, inst);
>> @@ -300,38 +318,63 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>>  		memset(core->caps, 0, sizeof(core->caps));
>>  	}
>>  
>> -	while (words_count) {
>> -		data = word + 1;
>> +	while (words < frame_size) {
>> +		payload = words + 1;
>>  
>> -		switch (*word) {
>> +		switch (*words) {
>>  		case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
>> -			parse_codecs(core, data);
>> +			if (rem_bytes <= sizeof(struct hfi_codec_supported))
>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +			ret = parse_codecs(core, payload);
>>  			init_codecs(core);
> 
> Does it make sense to call init_codecs if parse_codecs returned an error?
> It certainly looks weird, so even if it is OK, perhaps a comment might be
> useful.
parse_codecs() returns the sizeof(struct hfi_codec_supported), so it would
always be a valid value. I can put up a comment though.
> 
>>  			break;
>>  		case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
>> -			parse_max_sessions(core, data);
>> +			if (rem_bytes <= sizeof(struct hfi_max_sessions_supported))
>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +			ret = parse_max_sessions(core, payload);
>>  			break;
>>  		case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
>> -			parse_codecs_mask(&codecs, &domain, data);
>> +			if (rem_bytes <= sizeof(struct hfi_codec_mask_supported))
>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +			ret = parse_codecs_mask(&codecs, &domain, payload);
>>  			break;
>>  		case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
>> -			parse_raw_formats(core, codecs, domain, data);
>> +			if (rem_bytes <= sizeof(struct hfi_uncompressed_format_supported))
>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +			ret = parse_raw_formats(core, codecs, domain, payload);
>>  			break;
>>  		case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
>> -			parse_caps(core, codecs, domain, data);
>> +			if (rem_bytes <= sizeof(struct hfi_capabilities))
>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +			ret = parse_caps(core, codecs, domain, payload);
>>  			break;
>>  		case HFI_PROPERTY_PARAM_PROFILE_LEVEL_SUPPORTED:
>> -			parse_profile_level(core, codecs, domain, data);
>> +			if (rem_bytes <= sizeof(struct hfi_profile_level_supported))
>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +			ret = parse_profile_level(core, codecs, domain, payload);
>>  			break;
>>  		case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE_SUPPORTED:
>> -			parse_alloc_mode(core, codecs, domain, data);
>> +			if (rem_bytes <= sizeof(struct hfi_buffer_alloc_mode_supported))
>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +			ret = parse_alloc_mode(core, codecs, domain, payload);
>>  			break;
>>  		default:
>> +			ret = sizeof(u32);
>>  			break;
>>  		}
>>  
>> -		word++;
>> -		words_count--;
>> +		if (ret < 0)
>> +			return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> +		words += ret / sizeof(u32);
> 
> Would it make sense to check and warn if ret is not a multiple of sizeof(u32)?
> Up to you, just an idea.
almost all the individual parsing api in the various cases returns size of
predefined structures (or in their multiples), which would make them return in
multiple of sizeof(u32). Let say, if it encounters a non multiple of
sizeof(u32), the while loop would iterate couple of more iterations to hit the
next case in the payload.
> 
>> +		rem_bytes -= ret;
>>  	}
>>  
>>  	if (!core->max_sessions_supported)
>>
> 
> Regards,
> 
> 	Hans
Regards,
Vikash

Re: [PATCH v4 2/4] media: venus: hfi_parser: refactor hfi packet parsing logic

[Hans Verkuil]

On 2/20/25 16:38, Vikash Garodia wrote:
> 
> On 2/20/2025 8:50 PM, Hans Verkuil wrote:
>> On 2/7/25 09:24, Vikash Garodia wrote:
>>> words_count denotes the number of words in total payload, while data
>>> points to payload of various property within it. When words_count
>>> reaches last word, data can access memory beyond the total payload. This
>>> can lead to OOB access. With this patch, the utility api for handling
>>> individual properties now returns the size of data consumed. Accordingly
>>> remaining bytes are calculated before parsing the payload, thereby
>>> eliminates the OOB access possibilities.
>>>
>>> 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 | 95 +++++++++++++++++++-------
>>>  1 file changed, 69 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
>>> index 1cc17f3dc8948160ea6c3015d2c03e475b8aa29e..404c527329c5fa89ee885a6ad15620c9c90a99e4 100644
>>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>>> @@ -63,7 +63,7 @@ fill_buf_mode(struct hfi_plat_caps *cap, const void *data, unsigned int num)
>>>  		cap->cap_bufs_mode_dynamic = true;
>>>  }
>>>  
>>> -static void
>>> +static int
>>>  parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>  {
>>>  	struct hfi_buffer_alloc_mode_supported *mode = data;
>>> @@ -71,7 +71,7 @@ parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>  	u32 *type;
>>>  
>>>  	if (num_entries > MAX_ALLOC_MODE_ENTRIES)
>>> -		return;
>>> +		return -EINVAL;
>>>  
>>>  	type = mode->data;
>>>  
>>> @@ -83,6 +83,8 @@ parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>  
>>>  		type++;
>>>  	}
>>> +
>>> +	return sizeof(*mode);
>>>  }
>>>  
>>>  static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
>>> @@ -97,7 +99,7 @@ static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
>>>  	cap->num_pl += num;
>>>  }
>>>  
>>> -static void
>>> +static int
>>>  parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>  {
>>>  	struct hfi_profile_level_supported *pl = data;
>>> @@ -105,12 +107,14 @@ parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>  	struct hfi_profile_level pl_arr[HFI_MAX_PROFILE_COUNT] = {};
>>>  
>>>  	if (pl->profile_count > HFI_MAX_PROFILE_COUNT)
>>> -		return;
>>> +		return -EINVAL;
>>>  
>>>  	memcpy(pl_arr, proflevel, pl->profile_count * sizeof(*proflevel));
>>>  
>>>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>>>  		       fill_profile_level, pl_arr, pl->profile_count);
>>> +
>>> +	return pl->profile_count * sizeof(*proflevel) + sizeof(u32);
>>>  }
>>>  
>>>  static void
>>> @@ -125,7 +129,7 @@ fill_caps(struct hfi_plat_caps *cap, const void *data, unsigned int num)
>>>  	cap->num_caps += num;
>>>  }
>>>  
>>> -static void
>>> +static int
>>>  parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>  {
>>>  	struct hfi_capabilities *caps = data;
>>> @@ -134,12 +138,14 @@ parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>  	struct hfi_capability caps_arr[MAX_CAP_ENTRIES] = {};
>>>  
>>>  	if (num_caps > MAX_CAP_ENTRIES)
>>> -		return;
>>> +		return -EINVAL;
>>>  
>>>  	memcpy(caps_arr, cap, num_caps * sizeof(*cap));
>>>  
>>>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>>>  		       fill_caps, caps_arr, num_caps);
>>> +
>>> +	return sizeof(*caps);
>>>  }
>>>  
>>>  static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
>>> @@ -154,7 +160,7 @@ static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
>>>  	cap->num_fmts += num_fmts;
>>>  }
>>>  
>>> -static void
>>> +static int
>>>  parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>  {
>>>  	struct hfi_uncompressed_format_supported *fmt = data;
>>> @@ -163,7 +169,8 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>  	struct raw_formats rawfmts[MAX_FMT_ENTRIES] = {};
>>>  	u32 entries = fmt->format_entries;
>>>  	unsigned int i = 0;
>>> -	u32 num_planes;
>>> +	u32 num_planes = 0;
>>> +	u32 size;
>>>  
>>>  	while (entries) {
>>>  		num_planes = pinfo->num_planes;
>>> @@ -173,7 +180,7 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>  		i++;
>>>  
>>>  		if (i >= MAX_FMT_ENTRIES)
>>> -			return;
>>> +			return -EINVAL;
>>>  
>>>  		if (pinfo->num_planes > MAX_PLANES)
>>>  			break;
>>> @@ -185,9 +192,13 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>  
>>>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>>>  		       fill_raw_fmts, rawfmts, i);
>>> +	size = fmt->format_entries * (sizeof(*constr) * num_planes + 2 * sizeof(u32))
>>> +		+ 2 * sizeof(u32);
>>> +
>>> +	return size;
>>>  }
>>>  
>>> -static void parse_codecs(struct venus_core *core, void *data)
>>> +static int parse_codecs(struct venus_core *core, void *data)
>>>  {
>>>  	struct hfi_codec_supported *codecs = data;
>>>  
>>> @@ -199,21 +210,27 @@ static void parse_codecs(struct venus_core *core, void *data)
>>>  		core->dec_codecs &= ~HFI_VIDEO_CODEC_SPARK;
>>>  		core->enc_codecs &= ~HFI_VIDEO_CODEC_HEVC;
>>>  	}
>>> +
>>> +	return sizeof(*codecs);
>>>  }
>>>  
>>> -static void parse_max_sessions(struct venus_core *core, const void *data)
>>> +static int parse_max_sessions(struct venus_core *core, const void *data)
>>>  {
>>>  	const struct hfi_max_sessions_supported *sessions = data;
>>>  
>>>  	core->max_sessions_supported = sessions->max_sessions;
>>> +
>>> +	return sizeof(*sessions);
>>>  }
>>>  
>>> -static void parse_codecs_mask(u32 *codecs, u32 *domain, void *data)
>>> +static int parse_codecs_mask(u32 *codecs, u32 *domain, void *data)
>>>  {
>>>  	struct hfi_codec_mask_supported *mask = data;
>>>  
>>>  	*codecs = mask->codecs;
>>>  	*domain = mask->video_domains;
>>> +
>>> +	return sizeof(*mask);
>>>  }
>>>  
>>>  static void parser_init(struct venus_inst *inst, u32 *codecs, u32 *domain)
>>> @@ -282,8 +299,9 @@ static int hfi_platform_parser(struct venus_core *core, struct venus_inst *inst)
>>>  u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>>>  	       u32 size)
>>>  {
>>> -	unsigned int words_count = size >> 2;
>>> -	u32 *word = buf, *data, codecs = 0, domain = 0;
>>> +	u32 *words = buf, *payload, codecs = 0, domain = 0;
>>> +	u32 *frame_size = buf + size;
>>> +	u32 rem_bytes = size;
>>>  	int ret;
>>>  
>>>  	ret = hfi_platform_parser(core, inst);
>>> @@ -300,38 +318,63 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>>>  		memset(core->caps, 0, sizeof(core->caps));
>>>  	}
>>>  
>>> -	while (words_count) {
>>> -		data = word + 1;
>>> +	while (words < frame_size) {
>>> +		payload = words + 1;
>>>  
>>> -		switch (*word) {
>>> +		switch (*words) {
>>>  		case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
>>> -			parse_codecs(core, data);
>>> +			if (rem_bytes <= sizeof(struct hfi_codec_supported))
>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>> +
>>> +			ret = parse_codecs(core, payload);
>>>  			init_codecs(core);
>>
>> Does it make sense to call init_codecs if parse_codecs returned an error?
>> It certainly looks weird, so even if it is OK, perhaps a comment might be
>> useful.
> parse_codecs() returns the sizeof(struct hfi_codec_supported), so it would
> always be a valid value. I can put up a comment though.

Ah, so parse_codecs can never return an error. But what if someone in the future
does exactly that? I think it is still better to check for an error here. It just
feels like a bug waiting to happen in the future.

Regards,

	Hans

>>
>>>  			break;
>>>  		case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
>>> -			parse_max_sessions(core, data);
>>> +			if (rem_bytes <= sizeof(struct hfi_max_sessions_supported))
>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>> +
>>> +			ret = parse_max_sessions(core, payload);
>>>  			break;
>>>  		case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
>>> -			parse_codecs_mask(&codecs, &domain, data);
>>> +			if (rem_bytes <= sizeof(struct hfi_codec_mask_supported))
>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>> +
>>> +			ret = parse_codecs_mask(&codecs, &domain, payload);
>>>  			break;
>>>  		case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
>>> -			parse_raw_formats(core, codecs, domain, data);
>>> +			if (rem_bytes <= sizeof(struct hfi_uncompressed_format_supported))
>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>> +
>>> +			ret = parse_raw_formats(core, codecs, domain, payload);
>>>  			break;
>>>  		case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
>>> -			parse_caps(core, codecs, domain, data);
>>> +			if (rem_bytes <= sizeof(struct hfi_capabilities))
>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>> +
>>> +			ret = parse_caps(core, codecs, domain, payload);
>>>  			break;
>>>  		case HFI_PROPERTY_PARAM_PROFILE_LEVEL_SUPPORTED:
>>> -			parse_profile_level(core, codecs, domain, data);
>>> +			if (rem_bytes <= sizeof(struct hfi_profile_level_supported))
>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>> +
>>> +			ret = parse_profile_level(core, codecs, domain, payload);
>>>  			break;
>>>  		case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE_SUPPORTED:
>>> -			parse_alloc_mode(core, codecs, domain, data);
>>> +			if (rem_bytes <= sizeof(struct hfi_buffer_alloc_mode_supported))
>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>> +
>>> +			ret = parse_alloc_mode(core, codecs, domain, payload);
>>>  			break;
>>>  		default:
>>> +			ret = sizeof(u32);
>>>  			break;
>>>  		}
>>>  
>>> -		word++;
>>> -		words_count--;
>>> +		if (ret < 0)
>>> +			return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>> +
>>> +		words += ret / sizeof(u32);
>>
>> Would it make sense to check and warn if ret is not a multiple of sizeof(u32)?
>> Up to you, just an idea.
> almost all the individual parsing api in the various cases returns size of
> predefined structures (or in their multiples), which would make them return in
> multiple of sizeof(u32). Let say, if it encounters a non multiple of
> sizeof(u32), the while loop would iterate couple of more iterations to hit the
> next case in the payload.
>>
>>> +		rem_bytes -= ret;
>>>  	}
>>>  
>>>  	if (!core->max_sessions_supported)
>>>
>>
>> Regards,
>>
>> 	Hans
> Regards,
> Vikash

Re: [PATCH v4 2/4] media: venus: hfi_parser: refactor hfi packet parsing logic

[Vikash Garodia]

On 2/20/2025 9:12 PM, Hans Verkuil wrote:
> On 2/20/25 16:38, Vikash Garodia wrote:
>>
>> On 2/20/2025 8:50 PM, Hans Verkuil wrote:
>>> On 2/7/25 09:24, Vikash Garodia wrote:
>>>> words_count denotes the number of words in total payload, while data
>>>> points to payload of various property within it. When words_count
>>>> reaches last word, data can access memory beyond the total payload. This
>>>> can lead to OOB access. With this patch, the utility api for handling
>>>> individual properties now returns the size of data consumed. Accordingly
>>>> remaining bytes are calculated before parsing the payload, thereby
>>>> eliminates the OOB access possibilities.
>>>>
>>>> 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 | 95 +++++++++++++++++++-------
>>>>  1 file changed, 69 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
>>>> index 1cc17f3dc8948160ea6c3015d2c03e475b8aa29e..404c527329c5fa89ee885a6ad15620c9c90a99e4 100644
>>>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>>>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>>>> @@ -63,7 +63,7 @@ fill_buf_mode(struct hfi_plat_caps *cap, const void *data, unsigned int num)
>>>>  		cap->cap_bufs_mode_dynamic = true;
>>>>  }
>>>>  
>>>> -static void
>>>> +static int
>>>>  parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>>  {
>>>>  	struct hfi_buffer_alloc_mode_supported *mode = data;
>>>> @@ -71,7 +71,7 @@ parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>>  	u32 *type;
>>>>  
>>>>  	if (num_entries > MAX_ALLOC_MODE_ENTRIES)
>>>> -		return;
>>>> +		return -EINVAL;
>>>>  
>>>>  	type = mode->data;
>>>>  
>>>> @@ -83,6 +83,8 @@ parse_alloc_mode(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>>  
>>>>  		type++;
>>>>  	}
>>>> +
>>>> +	return sizeof(*mode);
>>>>  }
>>>>  
>>>>  static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
>>>> @@ -97,7 +99,7 @@ static void fill_profile_level(struct hfi_plat_caps *cap, const void *data,
>>>>  	cap->num_pl += num;
>>>>  }
>>>>  
>>>> -static void
>>>> +static int
>>>>  parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>>  {
>>>>  	struct hfi_profile_level_supported *pl = data;
>>>> @@ -105,12 +107,14 @@ parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>>  	struct hfi_profile_level pl_arr[HFI_MAX_PROFILE_COUNT] = {};
>>>>  
>>>>  	if (pl->profile_count > HFI_MAX_PROFILE_COUNT)
>>>> -		return;
>>>> +		return -EINVAL;
>>>>  
>>>>  	memcpy(pl_arr, proflevel, pl->profile_count * sizeof(*proflevel));
>>>>  
>>>>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>>>>  		       fill_profile_level, pl_arr, pl->profile_count);
>>>> +
>>>> +	return pl->profile_count * sizeof(*proflevel) + sizeof(u32);
>>>>  }
>>>>  
>>>>  static void
>>>> @@ -125,7 +129,7 @@ fill_caps(struct hfi_plat_caps *cap, const void *data, unsigned int num)
>>>>  	cap->num_caps += num;
>>>>  }
>>>>  
>>>> -static void
>>>> +static int
>>>>  parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>>  {
>>>>  	struct hfi_capabilities *caps = data;
>>>> @@ -134,12 +138,14 @@ parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>>  	struct hfi_capability caps_arr[MAX_CAP_ENTRIES] = {};
>>>>  
>>>>  	if (num_caps > MAX_CAP_ENTRIES)
>>>> -		return;
>>>> +		return -EINVAL;
>>>>  
>>>>  	memcpy(caps_arr, cap, num_caps * sizeof(*cap));
>>>>  
>>>>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>>>>  		       fill_caps, caps_arr, num_caps);
>>>> +
>>>> +	return sizeof(*caps);
>>>>  }
>>>>  
>>>>  static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
>>>> @@ -154,7 +160,7 @@ static void fill_raw_fmts(struct hfi_plat_caps *cap, const void *fmts,
>>>>  	cap->num_fmts += num_fmts;
>>>>  }
>>>>  
>>>> -static void
>>>> +static int
>>>>  parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>>  {
>>>>  	struct hfi_uncompressed_format_supported *fmt = data;
>>>> @@ -163,7 +169,8 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>>  	struct raw_formats rawfmts[MAX_FMT_ENTRIES] = {};
>>>>  	u32 entries = fmt->format_entries;
>>>>  	unsigned int i = 0;
>>>> -	u32 num_planes;
>>>> +	u32 num_planes = 0;
>>>> +	u32 size;
>>>>  
>>>>  	while (entries) {
>>>>  		num_planes = pinfo->num_planes;
>>>> @@ -173,7 +180,7 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>>  		i++;
>>>>  
>>>>  		if (i >= MAX_FMT_ENTRIES)
>>>> -			return;
>>>> +			return -EINVAL;
>>>>  
>>>>  		if (pinfo->num_planes > MAX_PLANES)
>>>>  			break;
>>>> @@ -185,9 +192,13 @@ parse_raw_formats(struct venus_core *core, u32 codecs, u32 domain, void *data)
>>>>  
>>>>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>>>>  		       fill_raw_fmts, rawfmts, i);
>>>> +	size = fmt->format_entries * (sizeof(*constr) * num_planes + 2 * sizeof(u32))
>>>> +		+ 2 * sizeof(u32);
>>>> +
>>>> +	return size;
>>>>  }
>>>>  
>>>> -static void parse_codecs(struct venus_core *core, void *data)
>>>> +static int parse_codecs(struct venus_core *core, void *data)
>>>>  {
>>>>  	struct hfi_codec_supported *codecs = data;
>>>>  
>>>> @@ -199,21 +210,27 @@ static void parse_codecs(struct venus_core *core, void *data)
>>>>  		core->dec_codecs &= ~HFI_VIDEO_CODEC_SPARK;
>>>>  		core->enc_codecs &= ~HFI_VIDEO_CODEC_HEVC;
>>>>  	}
>>>> +
>>>> +	return sizeof(*codecs);
>>>>  }
>>>>  
>>>> -static void parse_max_sessions(struct venus_core *core, const void *data)
>>>> +static int parse_max_sessions(struct venus_core *core, const void *data)
>>>>  {
>>>>  	const struct hfi_max_sessions_supported *sessions = data;
>>>>  
>>>>  	core->max_sessions_supported = sessions->max_sessions;
>>>> +
>>>> +	return sizeof(*sessions);
>>>>  }
>>>>  
>>>> -static void parse_codecs_mask(u32 *codecs, u32 *domain, void *data)
>>>> +static int parse_codecs_mask(u32 *codecs, u32 *domain, void *data)
>>>>  {
>>>>  	struct hfi_codec_mask_supported *mask = data;
>>>>  
>>>>  	*codecs = mask->codecs;
>>>>  	*domain = mask->video_domains;
>>>> +
>>>> +	return sizeof(*mask);
>>>>  }
>>>>  
>>>>  static void parser_init(struct venus_inst *inst, u32 *codecs, u32 *domain)
>>>> @@ -282,8 +299,9 @@ static int hfi_platform_parser(struct venus_core *core, struct venus_inst *inst)
>>>>  u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>>>>  	       u32 size)
>>>>  {
>>>> -	unsigned int words_count = size >> 2;
>>>> -	u32 *word = buf, *data, codecs = 0, domain = 0;
>>>> +	u32 *words = buf, *payload, codecs = 0, domain = 0;
>>>> +	u32 *frame_size = buf + size;
>>>> +	u32 rem_bytes = size;
>>>>  	int ret;
>>>>  
>>>>  	ret = hfi_platform_parser(core, inst);
>>>> @@ -300,38 +318,63 @@ u32 hfi_parser(struct venus_core *core, struct venus_inst *inst, void *buf,
>>>>  		memset(core->caps, 0, sizeof(core->caps));
>>>>  	}
>>>>  
>>>> -	while (words_count) {
>>>> -		data = word + 1;
>>>> +	while (words < frame_size) {
>>>> +		payload = words + 1;
>>>>  
>>>> -		switch (*word) {
>>>> +		switch (*words) {
>>>>  		case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
>>>> -			parse_codecs(core, data);
>>>> +			if (rem_bytes <= sizeof(struct hfi_codec_supported))
>>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>>> +
>>>> +			ret = parse_codecs(core, payload);
>>>>  			init_codecs(core);
>>>
>>> Does it make sense to call init_codecs if parse_codecs returned an error?
>>> It certainly looks weird, so even if it is OK, perhaps a comment might be
>>> useful.
>> parse_codecs() returns the sizeof(struct hfi_codec_supported), so it would
>> always be a valid value. I can put up a comment though.
> 
> Ah, so parse_codecs can never return an error. But what if someone in the future
> does exactly that? I think it is still better to check for an error here. It just
> feels like a bug waiting to happen in the future.
Ok, i can add a check to safeguard it.
> 
> Regards,
> 
> 	Hans
> 
>>>
>>>>  			break;
>>>>  		case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
>>>> -			parse_max_sessions(core, data);
>>>> +			if (rem_bytes <= sizeof(struct hfi_max_sessions_supported))
>>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>>> +
>>>> +			ret = parse_max_sessions(core, payload);
>>>>  			break;
>>>>  		case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
>>>> -			parse_codecs_mask(&codecs, &domain, data);
>>>> +			if (rem_bytes <= sizeof(struct hfi_codec_mask_supported))
>>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>>> +
>>>> +			ret = parse_codecs_mask(&codecs, &domain, payload);
>>>>  			break;
>>>>  		case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
>>>> -			parse_raw_formats(core, codecs, domain, data);
>>>> +			if (rem_bytes <= sizeof(struct hfi_uncompressed_format_supported))
>>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>>> +
>>>> +			ret = parse_raw_formats(core, codecs, domain, payload);
>>>>  			break;
>>>>  		case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
>>>> -			parse_caps(core, codecs, domain, data);
>>>> +			if (rem_bytes <= sizeof(struct hfi_capabilities))
>>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>>> +
>>>> +			ret = parse_caps(core, codecs, domain, payload);
>>>>  			break;
>>>>  		case HFI_PROPERTY_PARAM_PROFILE_LEVEL_SUPPORTED:
>>>> -			parse_profile_level(core, codecs, domain, data);
>>>> +			if (rem_bytes <= sizeof(struct hfi_profile_level_supported))
>>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>>> +
>>>> +			ret = parse_profile_level(core, codecs, domain, payload);
>>>>  			break;
>>>>  		case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE_SUPPORTED:
>>>> -			parse_alloc_mode(core, codecs, domain, data);
>>>> +			if (rem_bytes <= sizeof(struct hfi_buffer_alloc_mode_supported))
>>>> +				return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>>> +
>>>> +			ret = parse_alloc_mode(core, codecs, domain, payload);
>>>>  			break;
>>>>  		default:
>>>> +			ret = sizeof(u32);
>>>>  			break;
>>>>  		}
>>>>  
>>>> -		word++;
>>>> -		words_count--;
>>>> +		if (ret < 0)
>>>> +			return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>>>> +
>>>> +		words += ret / sizeof(u32);
>>>
>>> Would it make sense to check and warn if ret is not a multiple of sizeof(u32)?
>>> Up to you, just an idea.
>> almost all the individual parsing api in the various cases returns size of
>> predefined structures (or in their multiples), which would make them return in
>> multiple of sizeof(u32). Let say, if it encounters a non multiple of
>> sizeof(u32), the while loop would iterate couple of more iterations to hit the
>> next case in the payload.
>>>
>>>> +		rem_bytes -= ret;
>>>>  	}
>>>>  
>>>>  	if (!core->max_sessions_supported)
>>>>
>>>
>>> Regards,
>>>
>>> 	Hans
>> Regards,
>> Vikash
> 

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

[Vikash Garodia]

On 2/20/2025 8:53 PM, Hans Verkuil wrote:
> On 2/7/25 09:24, Vikash Garodia wrote:
>> sfr->buf_size is in shared memory and can be modified by malicious user.
>> OOB write is possible when the size is made higher than actual sfr data
>> buffer. Cap the size to allocated size for such cases.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> ---
>>  drivers/media/platform/qcom/venus/hfi_venus.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index 6b615270c5dae470c6fad408c9b5bc037883e56e..c3113420d266e61fcab44688580288d7408b50f4 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -1041,18 +1041,23 @@ static void venus_sfr_print(struct venus_hfi_device *hdev)
>>  {
>>  	struct device *dev = hdev->core->dev;
>>  	struct hfi_sfr *sfr = hdev->sfr.kva;
>> +	u32 size;
>>  	void *p;
>>  
>>  	if (!sfr)
>>  		return;
>>  
>> -	p = memchr(sfr->data, '\0', sfr->buf_size);
>> +	size = sfr->buf_size;
> 
> If this is ever 0...
> 
>> +	if (size > ALIGNED_SFR_SIZE)
>> +		size = ALIGNED_SFR_SIZE;
>> +
>> +	p = memchr(sfr->data, '\0', size);
>>  	/*
>>  	 * SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates
>>  	 * that Venus is in the process of crashing.
>>  	 */
>>  	if (!p)
>> -		sfr->data[sfr->buf_size - 1] = '\0';
>> +		sfr->data[size - 1] = '\0';
> 
> ...then this will overwrite memory. It probably can't be 0, but a check or perhaps
> just a comment might be good. It looks a bit scary.
Thats correct, it would not be 0 as its a prefixed one [1]. I can put up a
comment here.

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

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

[Tomasz Figa]

On Fri, Feb 21, 2025 at 12:56 AM Vikash Garodia
<quic_vgarodia@quicinc.com> wrote:
>
>
> On 2/20/2025 8:53 PM, Hans Verkuil wrote:
> > On 2/7/25 09:24, Vikash Garodia wrote:
> >> sfr->buf_size is in shared memory and can be modified by malicious user.
> >> OOB write is possible when the size is made higher than actual sfr data
> >> buffer. Cap the size to allocated size for such cases.
> >>
> >> Cc: stable@vger.kernel.org
> >> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
> >> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> >> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> >> ---
> >>  drivers/media/platform/qcom/venus/hfi_venus.c | 9 +++++++--
> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> >> index 6b615270c5dae470c6fad408c9b5bc037883e56e..c3113420d266e61fcab44688580288d7408b50f4 100644
> >> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> >> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> >> @@ -1041,18 +1041,23 @@ static void venus_sfr_print(struct venus_hfi_device *hdev)
> >>  {
> >>      struct device *dev = hdev->core->dev;
> >>      struct hfi_sfr *sfr = hdev->sfr.kva;
> >> +    u32 size;
> >>      void *p;
> >>
> >>      if (!sfr)
> >>              return;
> >>
> >> -    p = memchr(sfr->data, '\0', sfr->buf_size);
> >> +    size = sfr->buf_size;
> >
> > If this is ever 0...
> >
> >> +    if (size > ALIGNED_SFR_SIZE)
> >> +            size = ALIGNED_SFR_SIZE;
> >> +
> >> +    p = memchr(sfr->data, '\0', size);
> >>      /*
> >>       * SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates
> >>       * that Venus is in the process of crashing.
> >>       */
> >>      if (!p)
> >> -            sfr->data[sfr->buf_size - 1] = '\0';
> >> +            sfr->data[size - 1] = '\0';
> >
> > ...then this will overwrite memory. It probably can't be 0, but a check or perhaps
> > just a comment might be good. It looks a bit scary.
> Thats correct, it would not be 0 as its a prefixed one [1]. I can put up a
> comment here.

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

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

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

[Vikash Garodia]

On 2/21/2025 9:25 AM, Tomasz Figa wrote:
> On Fri, Feb 21, 2025 at 12:56 AM Vikash Garodia
> <quic_vgarodia@quicinc.com> wrote:
>>
>>
>> On 2/20/2025 8:53 PM, Hans Verkuil wrote:
>>> On 2/7/25 09:24, Vikash Garodia wrote:
>>>> sfr->buf_size is in shared memory and can be modified by malicious user.
>>>> OOB write is possible when the size is made higher than actual sfr data
>>>> buffer. Cap the size to allocated size for such cases.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: d96d3f30c0f2 ("[media] media: venus: hfi: add Venus HFI files")
>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>>>> ---
>>>>  drivers/media/platform/qcom/venus/hfi_venus.c | 9 +++++++--
>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>>>> index 6b615270c5dae470c6fad408c9b5bc037883e56e..c3113420d266e61fcab44688580288d7408b50f4 100644
>>>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>>>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>>>> @@ -1041,18 +1041,23 @@ static void venus_sfr_print(struct venus_hfi_device *hdev)
>>>>  {
>>>>      struct device *dev = hdev->core->dev;
>>>>      struct hfi_sfr *sfr = hdev->sfr.kva;
>>>> +    u32 size;
>>>>      void *p;
>>>>
>>>>      if (!sfr)
>>>>              return;
>>>>
>>>> -    p = memchr(sfr->data, '\0', sfr->buf_size);
>>>> +    size = sfr->buf_size;
>>>
>>> If this is ever 0...
>>>
>>>> +    if (size > ALIGNED_SFR_SIZE)
>>>> +            size = ALIGNED_SFR_SIZE;
>>>> +
>>>> +    p = memchr(sfr->data, '\0', size);
>>>>      /*
>>>>       * SFR isn't guaranteed to be NULL terminated since SYS_ERROR indicates
>>>>       * that Venus is in the process of crashing.
>>>>       */
>>>>      if (!p)
>>>> -            sfr->data[sfr->buf_size - 1] = '\0';
>>>> +            sfr->data[size - 1] = '\0';
>>>
>>> ...then this will overwrite memory. It probably can't be 0, but a check or perhaps
>>> just a comment might be good. It looks a bit scary.
>> Thats correct, it would not be 0 as its a prefixed one [1]. I can put up a
>> comment here.
> 
> Couldn't a bug (or vulnerability) in the firmware actually still cause
> it to write 0 there?
Possible. Though the size is initialized in driver with "ALIGNED_SFR_SIZE",
there is a possibility that the same could get overwritten by a rogue firmware.
Kept a check in v5, which cache the value locally and then does the check before
using that value.

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