* [PATCH] Revert "media: hfi_parser: don't trick gcc with a wrong expected size" @ 2019-06-05 20:19 Jonathan Marek 2019-06-05 20:41 ` Mauro Carvalho Chehab 2019-06-06 20:57 ` Stanimir Varbanov 0 siblings, 2 replies; 4+ messages in thread From: Jonathan Marek @ 2019-06-05 20:19 UTC (permalink / raw) Cc: Stanimir Varbanov, Andy Gross, David Brown, Mauro Carvalho Chehab, open list:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER, open list:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER, open list This reverts commit ded716267196862809e5926072adc962a611a1e3. This change doesn't make any sense and breaks the driver. Signed-off-by: Jonathan Marek <jonathan@marek.ca> --- drivers/media/platform/qcom/venus/hfi_helper.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h index 34ea503a9842..15804ad7e65d 100644 --- a/drivers/media/platform/qcom/venus/hfi_helper.h +++ b/drivers/media/platform/qcom/venus/hfi_helper.h @@ -569,7 +569,7 @@ struct hfi_capability { struct hfi_capabilities { u32 num_capabilities; - struct hfi_capability *data; + struct hfi_capability data[1]; }; #define HFI_DEBUG_MSG_LOW 0x01 @@ -726,7 +726,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[1]; }; struct hfi_quality_vs_speed { -- 2.17.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "media: hfi_parser: don't trick gcc with a wrong expected size" 2019-06-05 20:19 [PATCH] Revert "media: hfi_parser: don't trick gcc with a wrong expected size" Jonathan Marek @ 2019-06-05 20:41 ` Mauro Carvalho Chehab 2019-06-05 21:27 ` Jonathan Marek 2019-06-06 20:57 ` Stanimir Varbanov 1 sibling, 1 reply; 4+ messages in thread From: Mauro Carvalho Chehab @ 2019-06-05 20:41 UTC (permalink / raw) To: Jonathan Marek Cc: Stanimir Varbanov, Andy Gross, David Brown, open list:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER, open list:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER, open list Em Wed, 5 Jun 2019 16:19:40 -0400 Jonathan Marek <jonathan@marek.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 { ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "media: hfi_parser: don't trick gcc with a wrong expected size" 2019-06-05 20:41 ` Mauro Carvalho Chehab @ 2019-06-05 21:27 ` Jonathan Marek 0 siblings, 0 replies; 4+ messages in thread From: Jonathan Marek @ 2019-06-05 21:27 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Stanimir Varbanov, Andy Gross, David Brown, open list:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER, open list:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER, open list 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@marek.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 { > > > ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "media: hfi_parser: don't trick gcc with a wrong expected size" 2019-06-05 20:19 [PATCH] Revert "media: hfi_parser: don't trick gcc with a wrong expected size" Jonathan Marek 2019-06-05 20:41 ` Mauro Carvalho Chehab @ 2019-06-06 20:57 ` Stanimir Varbanov 1 sibling, 0 replies; 4+ messages in thread From: Stanimir Varbanov @ 2019-06-06 20:57 UTC (permalink / raw) To: Jonathan Marek Cc: Andy Gross, David Brown, Mauro Carvalho Chehab, open list:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER, open list:QUALCOMM VENUS VIDEO ACCELERATOR DRIVER, open list Hi Jonathan, I sent a fix for that here [1] and Mauro already taken it. regards, Stan [1] https://patchwork.kernel.org/patch/10963369/ On 5.06.19 г. 23:19 ч., Jonathan Marek wrote: > This reverts commit ded716267196862809e5926072adc962a611a1e3. > > This change doesn't make any sense and breaks the driver. > > Signed-off-by: Jonathan Marek <jonathan@marek.ca> > --- > drivers/media/platform/qcom/venus/hfi_helper.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h > index 34ea503a9842..15804ad7e65d 100644 > --- a/drivers/media/platform/qcom/venus/hfi_helper.h > +++ b/drivers/media/platform/qcom/venus/hfi_helper.h > @@ -569,7 +569,7 @@ struct hfi_capability { > > struct hfi_capabilities { > u32 num_capabilities; > - struct hfi_capability *data; > + struct hfi_capability data[1]; > }; > > #define HFI_DEBUG_MSG_LOW 0x01 > @@ -726,7 +726,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[1]; > }; > > struct hfi_quality_vs_speed { > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-06-06 20:57 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-06-05 20:19 [PATCH] Revert "media: hfi_parser: don't trick gcc with a wrong expected size" Jonathan Marek 2019-06-05 20:41 ` Mauro Carvalho Chehab 2019-06-05 21:27 ` Jonathan Marek 2019-06-06 20:57 ` Stanimir Varbanov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).