On 12/2/2024 5:38 PM, Bryan O'Donoghue wrote:
> On 28/11/2024 05:05, 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. Refactor the parsing logic such that the
>> remaining payload is checked before parsing it.
>>
>> Cc: stable@xxxxxxxxxxxxxxx
>> Fixes: 1a73374a04e5 ("media: venus: hfi_parser: add common capability parser")
>> Signed-off-by: Vikash Garodia <quic_vgarodia@xxxxxxxxxxx>
>> ---
>> drivers/media/platform/qcom/venus/hfi_parser.c | 57 +++++++++++++++++++++-----
>> 1 file changed, 46 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c
>> b/drivers/media/platform/qcom/venus/hfi_parser.c
>> index
>> 1cc17f3dc8948160ea6c3015d2c03e475b8aa29e..14349c2f84b205a8b79dee3acff1408bb63ac54a 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
>> @@ -282,8 +282,8 @@ 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)
>> {
>> + u32 *words = buf, *payload, codecs = 0, domain = 0;
>> unsigned int words_count = size >> 2;
>> - u32 *word = buf, *data, codecs = 0, domain = 0;
>> int ret;
>> ret = hfi_platform_parser(core, inst);
>> @@ -301,36 +301,71 @@ u32 hfi_parser(struct venus_core *core, struct
>> venus_inst *inst, void *buf,
>> }
>> while (words_count) {
>> - data = word + 1;
>> + payload = words + 1;
>> - switch (*word) {
>> + switch (*words) {
>> case HFI_PROPERTY_PARAM_CODEC_SUPPORTED:
>> - parse_codecs(core, data);
>> + if (words_count < sizeof(struct hfi_codec_supported))
>> + return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> + parse_codecs(core, payload);
>> init_codecs(core);
>> + words_count -= sizeof(struct hfi_codec_supported);
>> + words += sizeof(struct hfi_codec_supported);
>> break;
>> case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED:
>> - parse_max_sessions(core, data);
>> + if (words_count < sizeof(struct hfi_max_sessions_supported))
>> + return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> + parse_max_sessions(core, payload);
>> + words_count -= sizeof(struct hfi_max_sessions_supported);
>> + words += sizeof(struct hfi_max_sessions_supported);
>> break;
>> case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED:
>> - parse_codecs_mask(&codecs, &domain, data);
>> + if (words_count < sizeof(struct hfi_codec_mask_supported))
>> + return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> + parse_codecs_mask(&codecs, &domain, payload);
>> + words_count -= sizeof(struct hfi_codec_mask_supported);
>> + words += sizeof(struct hfi_codec_mask_supported);
>> break;
>> case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED:
>> - parse_raw_formats(core, codecs, domain, data);
>> + if (words_count < sizeof(struct hfi_uncompressed_format_supported))
>> + return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> + parse_raw_formats(core, codecs, domain, payload);
>> + words_count -= sizeof(struct hfi_uncompressed_format_supported);
>> + words += sizeof(struct hfi_uncompressed_format_supported);
>> break;
>> case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED:
>> - parse_caps(core, codecs, domain, data);
>> + if (words_count < sizeof(struct hfi_capabilities))
>> + return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> + parse_caps(core, codecs, domain, payload);
>> + words_count -= sizeof(struct hfi_capabilities);
>> + words += sizeof(struct hfi_capabilities);
>> break;
>> case HFI_PROPERTY_PARAM_PROFILE_LEVEL_SUPPORTED:
>> - parse_profile_level(core, codecs, domain, data);
>> + if (words_count < sizeof(struct hfi_profile_level_supported))
>> + return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> + parse_profile_level(core, codecs, domain, payload);
>> + words_count -= sizeof(struct hfi_profile_level_supported);
>> + words += sizeof(struct hfi_profile_level_supported);
>> break;
>> case HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE_SUPPORTED:
>> - parse_alloc_mode(core, codecs, domain, data);
>> + if (words_count < sizeof(struct hfi_buffer_alloc_mode_supported))
>> + return HFI_ERR_SYS_INSUFFICIENT_RESOURCES;
>> +
>> + parse_alloc_mode(core, codecs, domain, payload);
>> + words_count -= sizeof(struct hfi_buffer_alloc_mode_supported);
>> + words += sizeof(struct hfi_buffer_alloc_mode_supported);
>> break;
>> default:
>> break;
>> }
>> - word++;
>> + words++;
>> words_count--;
>> }
>>
>
> I like the changes made here.
>
> Let me suggest you have the parse_something() return the size of the buffer
> consumed or an error code.
>
> If you calculate the maximum pointer instead of the words_count
>
> frame_size = payload + max;
>
> /* Your while can look like this */
>
> while (words < frame_size)
> switch(*words){
> case HFI_PROPERTY_X:
> /* if the function returns the bytes consumed */
> ret = parse_x();
> break;
> case HFI_PROPERTY_X:
> ret = parse_x();
> break;
> }
>
> if (ret < 0)
> return -ret;
>
> /* you can increment the pointer once at the bottom of the loop */
> words += ret;
> }
>
>
> That way you can
>
> 1. Get rid of words_count and not have to decrement it
> 2. Have one variable words which is checked against the maximum
> size while(words < frame_size)
> 3. Have the function that consumes the data return
> how much buffer it has consumed, instead of inlining in the
> switch
> 4. Increment at the bottom of the switch once instead
> of several times in the switch
>
> IMO it would be clearer/neater that way. Please consider.
Appreciate your time to dig deeper into it. Expanding your suggestion, filling
in the details
frame_size = words + size;
/* Your while can look like this */
while (words < frame_size)
remaining_size = framesize - words;
switch(*words){
case HFI_PROPERTY_X:
if (remaining_size < sizeof(payload_X)
return insuff_res;
/* if the function returns the bytes consumed */
ret = parse_x(core, words + 1);
break;
case HFI_PROPERTY_Y:
if (remaining_size < sizeof(payload_X)
return insuff_res;
ret = parse_y(core, words + 1);
break;
default:
ret = 1;
}
if (ret < 0)
return -ret;
/* you can increment the pointer once at the bottom of the loop */
words += ret;
}
If you see, words_count is doing the role of remaining_size. In existing
implementation as well, we can move those increments per case to once per loop,
just that to avoid incrementing for default case.
Regards,
Vikash
