Re: [Intel-gfx] [PATCH 4/4] drm/i915/guc: Verify hwconfig blob matches supported format

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi,

Commit message please.

Will GuC folks be reviewing this work?

Quick sanity check maybe makes sense, given data is being "sent" to 
userspace directly, I am just not sure if it is worth having in 
non-debug builds of i915. Though I will agree not having it in 
production then largely defeats the purpose so dunno. Effective 
difference if GuC load fails versus userspace libraries failing to parse 
hwconfig?

On 07/02/2022 19:28, Jordan Justen wrote:
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> ---
>   .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 26 +++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
> index ce6088f112d4..695ef7a8f519 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
> @@ -71,6 +71,26 @@ static int guc_hwconfig_discover_size(struct intel_guc_hwconfig *hwconfig)
>   	return 0;
>   }
>   
> +static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig)
> +{
> +	if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL)
> +		return -EINVAL;

So individual item size is minimum one u32, or zero? Document that in 
patch 3?

> +
> +	struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;

kbuild robot told you about mixing declarations and code already. :)

> +	u32 remaining = (hwconfig->size / 4);

Blank line here and braces not needed.

> +	while (remaining > 0) {
> +		if (remaining < 2)
> +			return -EINVAL;
> +		if (pos->length > remaining - 2)
> +			return -EINVAL;
> +		remaining -= 2 + pos->length;
> +		pos = (void *)&pos->data[pos->length];
> +	}
> +
> +	DRM_INFO("hwconfig blob format appears valid\n");

Probably debug level at most.

> +	return 0;
> +}
> +
>   static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
>   {
>   	struct intel_guc *guc = hwconfig_to_guc(hwconfig);
> @@ -91,6 +111,12 @@ static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
>   	if (ret >= 0)
>   		memcpy(hwconfig->ptr, vaddr, hwconfig->size);
>   
> +	if (verify_hwconfig_blob(hwconfig)) {

Merge under the "ret >= 0" branch above?

> +		DRM_ERROR("Ignoring invalid hwconfig blob received from "
> +			  "GuC!\n");

Use drm_dbg/drm_err so the log is tied to a device in multi-gpu systems.

Also keep the string on one line as per kernel coding style guide.

Regards,

Tvrtko

> +		return -EINVAL;
> +	}
> +
>   	i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
>   
>   	return ret;