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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 5 Jun 2019 17:27:38 -0400
From:   Jonathan Marek <jonathan@...ek.ca>
To:     Mauro Carvalho Chehab <mchehab@...nel.org>
Cc:     Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Andy Gross <agross@...nel.org>,
        David Brown <david.brown@...aro.org>,
        "open list:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER" 
        <linux-media@...r.kernel.org>,
        "open list:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER" 
        <linux-arm-msm@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Revert "media: hfi_parser: don't trick gcc with a wrong
 expected size"

hfi_capabilities /  hfi_profile_level_supported come from hardware so 
there is no option to dynamically allocate, and using size [1] doesn't 
cause any bug.

Your enclosed patch is wrong in a way because MAX_CAP_ENTRIES is not a 
hardware limit but the size of the statically allocated array used by 
the driver. I don't think there is any defined hardware limit, otherwise 
the driver author would've defined it as they did with 
HFI_MAX_PROFILE_COUNT.

A better solution (IMO) if you want to avoid these warnings is to remove 
those memcpy() and work on the data[] / profile_level[] from the struct 
directly:

diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c 
b/drivers/media/platform/qcom/venus/hfi_parser.c
index 2293d936e49c..ecaa336b2cb9 100644
--- a/drivers/media/platform/qcom/venus/hfi_parser.c
+++ b/drivers/media/platform/qcom/venus/hfi_parser.c
@@ -94,16 +94,12 @@ static void
  parse_profile_level(struct venus_core *core, u32 codecs, u32 domain, 
void *data)
  {
  	struct hfi_profile_level_supported *pl = data;
-	struct hfi_profile_level *proflevel = pl->profile_level;
-	struct hfi_profile_level pl_arr[HFI_MAX_PROFILE_COUNT] = {};

  	if (pl->profile_count > HFI_MAX_PROFILE_COUNT)
  		return;

-	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);
+		       fill_profile_level, pl->profile_level, pl->profile_count);
  }

  static void
@@ -119,17 +115,12 @@ static void
  parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
  {
  	struct hfi_capabilities *caps = data;
-	struct hfi_capability *cap = caps->data;
-	u32 num_caps = caps->num_capabilities;
-	struct hfi_capability caps_arr[MAX_CAP_ENTRIES] = {};

-	if (num_caps > MAX_CAP_ENTRIES)
+	if (caps->num_capabilities > MAX_CAP_ENTRIES)
  		return;

-	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);
+		       fill_caps, caps->data, caps->num_capabilities);
  }

  static void fill_raw_fmts(struct venus_caps *cap, const void *fmts,

On 6/5/19 4:41 PM, Mauro Carvalho Chehab wrote:
> 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 {
> 
> 
>