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

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

On 22/02/2022 10:36, Jordan Justen wrote:
> i915_drm.h now defines the format of the returned
> DRM_I915_QUERY_HWCONFIG_BLOB query item. Since i915 receives this from
> the black box GuC software, it should verify that the data matches
> that format before sending it to user-space.
> 
> The verification makes a single simple pass through the blob contents,
> so this verification step should not add a significant amount of init
> time to i915.
> 
> v3:
>   * Add various changes suggested by Tvrtko
> 
> v4:
>   * Rewrite verify_hwconfig_blob() to hopefully be clearer without
>     relying on comments so much, and add various suggestions from
>     Michal.
> 
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> Acked-by: Jon Bloomfield <jon.bloomfield@intel.com>
> ---
>   .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 44 ++++++++++++++++++-
>   1 file changed, 43 insertions(+), 1 deletion(-)
> 
> 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 ad289603460c..a844b880cbdb 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
> @@ -73,9 +73,46 @@ static int guc_hwconfig_discover_size(struct intel_guc_hwconfig *hwconfig)
>   	return 0;
>   }
>   
> +static int verify_hwconfig_blob(struct intel_guc_hwconfig *hwconfig)
> +{
> +	struct intel_guc *guc = hwconfig_to_guc(hwconfig);
> +	struct drm_device *drm = &guc_to_gt(guc)->i915->drm;
> +	struct drm_i915_query_hwconfig_blob_item *item = hwconfig->ptr;
> +	u64 offset = 0;
> +	u64 remaining = hwconfig->size;
> +	/* Everything before the data field is required */
> +	u64 min_item_size = offsetof(struct drm_i915_query_hwconfig_blob_item, data);
> +	u64 item_size;
> +
> +	if (!IS_ALIGNED(hwconfig->size, sizeof(u32))) {
> +		drm_err(drm, "hwconfig blob size (%d) is not u32 aligned\n", hwconfig->size);
> +		return -EINVAL;
> +	}
> +
> +	while (offset < hwconfig->size) {
> +		if (remaining < min_item_size) {
> +			drm_err(drm, "hwconfig blob invalid (no room for item required fields at offset %lld)\n",
> +				offset);
> +			return -EINVAL;
> +		}
> +		item_size = min_item_size + sizeof(u32) * item->length;
> +		if (item_size > remaining) {
> +			drm_err(drm, "hwconfig blob invalid (no room for data array of item at offset %lld)\n",
> +				offset);
> +			return -EINVAL;
> +		}
> +		offset += item_size;
> +		remaining -= item_size;
> +		item = (void *)&item->data[item->length];
> +	}
> +
> +	return 0;
> +}
> +
>   static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
>   {
>   	struct intel_guc *guc = hwconfig_to_guc(hwconfig);
> +	struct drm_device *drm = &guc_to_gt(guc)->i915->drm;
>   	struct i915_vma *vma;
>   	u32 ggtt_offset;
>   	void *vaddr;
> @@ -90,8 +127,13 @@ static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
>   	ggtt_offset = intel_guc_ggtt_offset(guc, vma);
>   
>   	ret = __guc_action_get_hwconfig(guc, ggtt_offset, hwconfig->size);
> -	if (ret >= 0)
> +	if (ret >= 0) {
>   		memcpy(hwconfig->ptr, vaddr, hwconfig->size);
> +		if (verify_hwconfig_blob(hwconfig)) {
> +			drm_err(drm, "Ignoring invalid hwconfig blob received from GuC!\n");
> +			ret = -EINVAL;
> +		}
> +	}
>   
>   	i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
>   

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko