| 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
| ||
|
Message-ID: <37934625-661f-4347-a1cf-b77d0a8702ca@xs4all.nl>
Date: Thu, 20 Feb 2025 16:42:12 +0100
From: Hans Verkuil <hverkuil@...all.nl>
To: Vikash Garodia <quic_vgarodia@...cinc.com>,
Stanimir Varbanov <stanimir.k.varbanov@...il.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Tomasz Figa
<tfiga@...omium.org>, Hans Verkuil <hans.verkuil@...co.com>
Cc: Stanimir Varbanov <stanimir.varbanov@...aro.org>,
Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, linux-media@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
stable@...r.kernel.org
Subject: Re: [PATCH v4 2/4] media: venus: hfi_parser: refactor hfi packet
parsing logic
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@...r.kernel.org
>>> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
>>> Signed-off-by: Vikash Garodia <quic_vgarodia@...cinc.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
Powered by blists - more mailing lists