Re: [PATCH 1/3] media: venus: hfi_parser: account for all capabilities when skipping a property

On 8/12/26 10:01 PM, Dmitry Baryshkov wrote:
> hfi_parser() walks the property list of the message it is given by
> advancing over each property by the length its handler returns.
> parse_caps() returns a fixed sizeof(*caps), which only covers the
> num_capabilities field and, at the time the code was written, the single
> struct hfi_capability that struct hfi_capabilities then declared.  A
> property carrying N capabilities is therefore under-skipped by N-1 entries,
> and struct hfi_capabilities has since become a flexible array member, so
> today the whole capability array is left behind.
> 
> The parser recovers from this because unrecognized words are skipped one at
> a time and capability types, limits and step sizes do not collide with the
> HFI property identifiers, but nothing guarantees that: any capability value
> that happens to equal a property ID is parsed as a property, at an offset
> that is not a property boundary.
> 
> Return the length the payload actually has.  Like the other handlers this
> leaves the property identifier itself unaccounted for, so the walk resumes
> on the last word of the payload rather than on the next property, and
> relies on that word being skipped as unrecognized on the following
> iteration.
> 
> Fixes: 09c2845e8fe4 ("[media] media: venus: hfi: add Host Firmware Interface (HFI)")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Dmitry Baryshkov <[email protected]>
> ---
>  drivers/media/platform/qcom/venus/hfi_parser.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_parser.c b/drivers/media/platform/qcom/venus/hfi_parser.c
> index b1657443f23f..3413b91b0b7e 100644
> --- a/drivers/media/platform/qcom/venus/hfi_parser.c
> +++ b/drivers/media/platform/qcom/venus/hfi_parser.c
> @@ -146,7 +146,7 @@ parse_caps(struct venus_core *core, u32 codecs, u32 domain, void *data)
>  	for_each_codec(core->caps, ARRAY_SIZE(core->caps), codecs, domain,
>  		       fill_caps, caps_arr, num_caps);
>  
> -	return sizeof(*caps);
> +	return struct_size(caps, data, num_caps);

This is already solved in:

a51cea23e409 ("media: venus: fix payload size returned by parse_caps() and parse_alloc_mode()")

using struct_size instead of the open-coded version would still
be welcome though

Konrad