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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 08.02.2022 22:05, 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
> 
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> ---
>  .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 56 ++++++++++++++++++-
>  1 file changed, 53 insertions(+), 3 deletions(-)
> 
> 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..350a0517b9f0 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
> @@ -71,7 +71,52 @@ static int guc_hwconfig_discover_size(struct intel_guc_hwconfig *hwconfig)
>  	return 0;
>  }
>  
> -static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
> +static int verify_hwconfig_blob(struct drm_device *drm,

no need to pass drm as you can use:

+	struct intel_guc *guc = hwconfig_to_guc(hwconfig);
+	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;

> +				const struct intel_guc_hwconfig *hwconfig)
> +{
> +	struct drm_i915_query_hwconfig_blob_item *pos;
> +	u32 remaining;
> +
> +	if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL)

size alignment could be verified in guc_hwconfig_discover_size()

nit: instead of hardcoded 4 you may use 'sizeof(u32)'
nit: and IS_ALIGNED

and non-null ptr shall be enforced with GEM_BUG_ON as you are calling
this function after memcpy

> +		return -EINVAL;
> +
> +	pos = hwconfig->ptr;

add line space

and please update below multi-line comments format to
	/*
	 * blah...
	
> +	/* The number of dwords in the blob to validate. Each loop
> +	 * pass will process at least 2 dwords corresponding to the
> +	 * key and length fields of the item. In addition, the length
> +	 * field of the item indicates the length of the data array,
> +	 * and that number of dwords will be processed (skipped) as
> +	 * well.
> +	 */
> +	remaining = hwconfig->size / 4;
> +
> +	while (remaining > 0) {
> +		/* Each item requires at least 2 dwords for the key
> +		 * and length fields. If the length field is 0, then
> +		 * the data array would be of length 0.
> +		 */
> +		if (remaining < 2)
> +			return -EINVAL;
> +		/* remaining >= 2, so subtracting 2 is ok, whereas
> +		 * adding 2 to pos->length could overflow.
> +		 */
> +		if (pos->length > remaining - 2)
> +			return -EINVAL;
> +		/* The length check above ensures that the adjustment
> +		 * of the remaining variable will not underflow, and
> +		 * that the adjustment of the pos variable will not
> +		 * pass the end of the blob data.
> +		 */
> +		remaining -= 2 + pos->length;
> +		pos = (void *)&pos->data[pos->length];
> +	}

btw, if it needs comments then it is too complicated ;)

> +
> +	drm_dbg(drm, "hwconfig blob format is valid\n");

not sure if we need this since we have error message in case of failure
maybe better to add dbg message why we claim it is invalid

> +	return 0;
> +}
> +
> +static int guc_hwconfig_fill_buffer(struct drm_device *drm,

no need to pass drm

> +				    struct intel_guc_hwconfig *hwconfig)
>  {
>  	struct intel_guc *guc = hwconfig_to_guc(hwconfig);
>  	struct i915_vma *vma;
> @@ -88,8 +133,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(hwconfig, ggtt_offset, hwconfig->size);
> -	if (ret >= 0)
> +	if (ret >= 0) {
>  		memcpy(hwconfig->ptr, vaddr, hwconfig->size);
> +		if (verify_hwconfig_blob(drm, hwconfig)) {
> +			drm_err(drm, "Ignoring invalid hwconfig blob received from GuC!\n");
> +			ret = -EINVAL;

btw, since we are about to release blob on verification failure,
shouldn't we hexdump whole (or part of) blob somewhere for investigations ?

or maybe we should expose this blob in debugfs, and do it regardless if
it is valid or not, and just fail ioctl if blob is believed to be corrupted.

~Michal

> +		}
> +	}
>  
>  	i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
>  
> @@ -141,7 +191,7 @@ int intel_guc_hwconfig_init(struct intel_guc_hwconfig *hwconfig)
>  		return -ENOMEM;
>  	}
>  
> -	ret = guc_hwconfig_fill_buffer(hwconfig);
> +	ret = guc_hwconfig_fill_buffer(&i915->drm, hwconfig);
>  	if (ret < 0) {
>  		intel_guc_hwconfig_fini(hwconfig);
>  		return ret;