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;