| Message ID | 20190724022153.8927-6-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uC fw path unification + misc clean-up | expand |
On Wed, 24 Jul 2019 04:21:50 +0200, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote: > The binary is perma-pinned and the rsa is not going to change, so copy > it only once and not on every load. as this new location is accessible from the GuC, what if GuC (or whoever else) corrupts it ? with stale RSA we will fail to authenticate HuC on subsequent resets. > > v2: onion unwind (Chris) > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Fernando Pacheco <fernando.pacheco@intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> #v1 > --- > drivers/gpu/drm/i915/gt/uc/intel_huc.c | 27 +++++++++++++++++++---- > drivers/gpu/drm/i915/gt/uc/intel_huc.h | 1 - > drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 17 -------------- > 3 files changed, 23 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c > b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > index 7804ea5f699c..41f62bdf6022 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > @@ -50,6 +50,7 @@ static int intel_huc_rsa_data_create(struct intel_huc > *huc) > struct intel_gt *gt = huc_to_gt(huc); > struct intel_guc *guc = >->uc.guc; > struct i915_vma *vma; > + size_t copied; > void *vaddr; > /* > @@ -62,6 +63,7 @@ static int intel_huc_rsa_data_create(struct intel_huc > *huc) > * the authentication since its GGTT offset will be GuC > * accessible. > */ > + GEM_BUG_ON(huc->fw.rsa_size > PAGE_SIZE); > vma = intel_guc_allocate_vma(guc, PAGE_SIZE); > if (IS_ERR(vma)) > return PTR_ERR(vma); > @@ -72,26 +74,43 @@ static int intel_huc_rsa_data_create(struct > intel_huc *huc) > return PTR_ERR(vaddr); > } > + copied = intel_uc_fw_copy_rsa(&huc->fw, vaddr, vma->size); > + GEM_BUG_ON(copied < huc->fw.rsa_size); > + > + i915_gem_object_unpin_map(vma->obj); > + > huc->rsa_data = vma; > - huc->rsa_data_vaddr = vaddr; > return 0; > } > static void intel_huc_rsa_data_destroy(struct intel_huc *huc) > { > - i915_vma_unpin_and_release(&huc->rsa_data, I915_VMA_RELEASE_MAP); > + i915_vma_unpin_and_release(&huc->rsa_data, 0); > } > int intel_huc_init(struct intel_huc *huc) > { > int err; > - err = intel_huc_rsa_data_create(huc); > + err = intel_uc_fw_init(&huc->fw); > if (err) > return err; > - return intel_uc_fw_init(&huc->fw); > + /* > + * HuC firmware image is outside GuC accessible range. > + * Copy the RSA signature out of the image into > + * a perma-pinned region set aside for it > + */ > + err = intel_huc_rsa_data_create(huc); > + if (err) > + goto out_fini; > + > + return 0; > + > +out_fini: > + intel_uc_fw_fini(&huc->fw); > + return err; > } > void intel_huc_fini(struct intel_huc *huc) > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h > b/drivers/gpu/drm/i915/gt/uc/intel_huc.h > index ea340f85bc46..4465209ce233 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h > @@ -35,7 +35,6 @@ struct intel_huc { > /* HuC-specific additions */ > struct i915_vma *rsa_data; > - void *rsa_data_vaddr; > struct { > i915_reg_t reg; > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > index 8f119ff291fa..f7049f0c7444 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > @@ -36,21 +36,6 @@ void intel_huc_fw_init_early(struct intel_huc *huc) > intel_uc_fw_init_early(huc_to_gt(huc)->i915, huc_fw, > INTEL_UC_FW_TYPE_HUC); > } > -static void huc_xfer_rsa(struct intel_huc *huc) > -{ > - size_t copied; > - > - /* > - * HuC firmware image is outside GuC accessible range. > - * Copy the RSA signature out of the image into > - * the perma-pinned region set aside for it > - */ > - GEM_BUG_ON(huc->fw.rsa_size > huc->rsa_data->size); > - copied = intel_uc_fw_copy_rsa(&huc->fw, huc->rsa_data_vaddr, > - huc->rsa_data->size); > - GEM_BUG_ON(copied < huc->fw.rsa_size); > -} > - > static int huc_xfer_ucode(struct intel_huc *huc) > { > struct intel_uc_fw *huc_fw = &huc->fw; > @@ -110,8 +95,6 @@ static int huc_fw_xfer(struct intel_uc_fw *huc_fw) > { > struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw); > - huc_xfer_rsa(huc); > - > return huc_xfer_ucode(huc); > }
Quoting Michal Wajdeczko (2019-07-24 13:55:23) > On Wed, 24 Jul 2019 04:21:50 +0200, Daniele Ceraolo Spurio > <daniele.ceraolospurio@intel.com> wrote: > > > The binary is perma-pinned and the rsa is not going to change, so copy > > it only once and not on every load. > > as this new location is accessible from the GuC, what if GuC (or whoever > else) corrupts it ? with stale RSA we will fail to authenticate HuC on > subsequent resets. Refusing to run after misbehaviour is reasonable, and probably better than running with a successful adversary. We can equally conjecture how to respond to an attack against any other GGTT or even ppGTT object, where we have no idea on the identity of the culprit. That's before we even start on hidden hypervisors and microcontrollers. -Chris
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index 7804ea5f699c..41f62bdf6022 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -50,6 +50,7 @@ static int intel_huc_rsa_data_create(struct intel_huc *huc) struct intel_gt *gt = huc_to_gt(huc); struct intel_guc *guc = >->uc.guc; struct i915_vma *vma; + size_t copied; void *vaddr; /* @@ -62,6 +63,7 @@ static int intel_huc_rsa_data_create(struct intel_huc *huc) * the authentication since its GGTT offset will be GuC * accessible. */ + GEM_BUG_ON(huc->fw.rsa_size > PAGE_SIZE); vma = intel_guc_allocate_vma(guc, PAGE_SIZE); if (IS_ERR(vma)) return PTR_ERR(vma); @@ -72,26 +74,43 @@ static int intel_huc_rsa_data_create(struct intel_huc *huc) return PTR_ERR(vaddr); } + copied = intel_uc_fw_copy_rsa(&huc->fw, vaddr, vma->size); + GEM_BUG_ON(copied < huc->fw.rsa_size); + + i915_gem_object_unpin_map(vma->obj); + huc->rsa_data = vma; - huc->rsa_data_vaddr = vaddr; return 0; } static void intel_huc_rsa_data_destroy(struct intel_huc *huc) { - i915_vma_unpin_and_release(&huc->rsa_data, I915_VMA_RELEASE_MAP); + i915_vma_unpin_and_release(&huc->rsa_data, 0); } int intel_huc_init(struct intel_huc *huc) { int err; - err = intel_huc_rsa_data_create(huc); + err = intel_uc_fw_init(&huc->fw); if (err) return err; - return intel_uc_fw_init(&huc->fw); + /* + * HuC firmware image is outside GuC accessible range. + * Copy the RSA signature out of the image into + * a perma-pinned region set aside for it + */ + err = intel_huc_rsa_data_create(huc); + if (err) + goto out_fini; + + return 0; + +out_fini: + intel_uc_fw_fini(&huc->fw); + return err; } void intel_huc_fini(struct intel_huc *huc) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h index ea340f85bc46..4465209ce233 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h @@ -35,7 +35,6 @@ struct intel_huc { /* HuC-specific additions */ struct i915_vma *rsa_data; - void *rsa_data_vaddr; struct { i915_reg_t reg; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c index 8f119ff291fa..f7049f0c7444 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c @@ -36,21 +36,6 @@ void intel_huc_fw_init_early(struct intel_huc *huc) intel_uc_fw_init_early(huc_to_gt(huc)->i915, huc_fw, INTEL_UC_FW_TYPE_HUC); } -static void huc_xfer_rsa(struct intel_huc *huc) -{ - size_t copied; - - /* - * HuC firmware image is outside GuC accessible range. - * Copy the RSA signature out of the image into - * the perma-pinned region set aside for it - */ - GEM_BUG_ON(huc->fw.rsa_size > huc->rsa_data->size); - copied = intel_uc_fw_copy_rsa(&huc->fw, huc->rsa_data_vaddr, - huc->rsa_data->size); - GEM_BUG_ON(copied < huc->fw.rsa_size); -} - static int huc_xfer_ucode(struct intel_huc *huc) { struct intel_uc_fw *huc_fw = &huc->fw; @@ -110,8 +95,6 @@ static int huc_fw_xfer(struct intel_uc_fw *huc_fw) { struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw); - huc_xfer_rsa(huc); - return huc_xfer_ucode(huc); }