| Message ID | 20220208210503.869491-5-jordan.l.justen@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | GuC HWCONFIG with documentation | expand |
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;
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, + const struct intel_guc_hwconfig *hwconfig) +{ + struct drm_i915_query_hwconfig_blob_item *pos; + u32 remaining; + + if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL) + return -EINVAL; + + pos = hwconfig->ptr; + /* 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]; + } + + drm_dbg(drm, "hwconfig blob format is valid\n"); + return 0; +} + +static int guc_hwconfig_fill_buffer(struct drm_device *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; + } + } 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;
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(-)