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@xxxxxxxxx>
> ---
> .../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;
