linux-kernel - Re: [PATCH] Revert "media: hfi_parser: don't trick gcc with a wrong expected size"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190605174044.65ac1e4a@coco.lan>
Date:   Wed, 5 Jun 2019 17:41:02 -0300
From:   Mauro Carvalho Chehab <mchehab@...nel.org>
To:     Jonathan Marek <jonathan@...ek.ca>
Cc:     Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Andy Gross <agross@...nel.org>,
        David Brown <david.brown@...aro.org>,
        linux-media@...r.kernel.org (open list:QUALCOMM VENUS VIDEO ACCELERATOR
        DRIVER),
        linux-arm-msm@...r.kernel.org (open list:QUALCOMM VENUS VIDEO
        ACCELERATOR DRIVER), linux-kernel@...r.kernel.org (open list)
Subject: Re: [PATCH] Revert "media: hfi_parser: don't trick gcc with a wrong
 expected size"

Em Wed,  5 Jun 2019 16:19:40 -0400
Jonathan Marek <jonathan@...ek.ca> escreveu:

> This reverts commit ded716267196862809e5926072adc962a611a1e3.
> 
> This change doesn't make any sense and breaks the driver.

The fix is indeed wrong, but reverting is the wrong thing to do.

The problem is that the driver is trying to write past the
allocated area, as reported:

	drivers/media/platform/qcom/venus/hfi_parser.c:103 parse_profile_level() error: memcpy() 'proflevel' too small (8 vs 128)
	drivers/media/platform/qcom/venus/hfi_parser.c:129 parse_caps() error: memcpy() 'cap' too small (16 vs 512)

If you check the memcpy() logic at the above lines, you'll see that
hfi_capability.data may have up to 32 entries, and
hfi_profile_level_supported.profile level can have up to it can be up
to 16 entries.

So, the buffer should either be dynamically allocated with the real
size or we need something like the enclosed patch.

Thanks,
Mauro

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 7a3feb5cee00..06a84f266bcc 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -59,7 +59,6 @@ struct venus_format {
 
 #define MAX_PLANES		4
 #define MAX_FMT_ENTRIES		32
-#define MAX_CAP_ENTRIES		32
 #define MAX_ALLOC_MODE_ENTRIES	16
 #define MAX_CODEC_NUM		32
 
diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h
index 34ea503a9842..ca8033381515 100644
--- a/drivers/media/platform/qcom/venus/hfi_helper.h
+++ b/drivers/media/platform/qcom/venus/hfi_helper.h
@@ -560,6 +560,8 @@ struct hfi_bitrate {
 #define HFI_CAPABILITY_HIER_P_HYBRID_NUM_ENH_LAYERS	0x15
 #define HFI_CAPABILITY_MBS_PER_SECOND_POWERSAVE		0x16
 
+#define MAX_CAP_ENTRIES                32
+
 struct hfi_capability {
 	u32 capability_type;
 	u32 min;
@@ -569,7 +571,7 @@ struct hfi_capability {
 
 struct hfi_capabilities {
 	u32 num_capabilities;
-	struct hfi_capability *data;
+	struct hfi_capability data[MAX_CAP_ENTRIES];
 };
 
 #define HFI_DEBUG_MSG_LOW	0x01
@@ -726,7 +728,7 @@ struct hfi_profile_level {
 
 struct hfi_profile_level_supported {
 	u32 profile_count;
-	struct hfi_profile_level *profile_level;
+	struct hfi_profile_level profile_level[HFI_MAX_PROFILE_COUNT];
 };
 
 struct hfi_quality_vs_speed {