diff mbox series

[7/7] drm/i915/uc: Hardening firmware fetch

Message ID 20190807170034.8440-8-michal.wajdeczko@intel.com (mailing list archive)
State New, archived
Headers show
Series Hardening firmware fetch | expand

Commit Message

Michal Wajdeczko Aug. 7, 2019, 5 p.m. UTC
Insert few more failure points into firmware fetch procedure to check
use of the wrong blob name or use of the mismatched firmware versions.

Also update some messages (remove ptr, duplicated infos) and stop
treating all fetch errors as missing firmware case.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 137 ++++++++++++++++-------
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h |   3 +
 2 files changed, 97 insertions(+), 43 deletions(-)

Comments

Chris Wilson Aug. 7, 2019, 5:45 p.m. UTC | #1
Quoting Michal Wajdeczko (2019-08-07 18:00:34)
> Insert few more failure points into firmware fetch procedure to check
> use of the wrong blob name or use of the mismatched firmware versions.
> 
> Also update some messages (remove ptr, duplicated infos) and stop
> treating all fetch errors as missing firmware case.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 137 ++++++++++++++++-------
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h |   3 +
>  2 files changed, 97 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index 3a3803bfa5a8..4faff1010398 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -153,20 +153,23 @@ static const char *__override_huc_firmware_path(void)
>         return "";
>  }
>  
> -static bool
> -__uc_fw_override(struct intel_uc_fw *uc_fw)
> +static void __uc_fw_user_override(struct intel_uc_fw *uc_fw)
>  {
> +       const char *path = NULL;
> +
>         switch (uc_fw->type) {
>         case INTEL_UC_FW_TYPE_GUC:
> -               uc_fw->path = __override_guc_firmware_path();
> +               path = __override_guc_firmware_path();
>                 break;
>         case INTEL_UC_FW_TYPE_HUC:
> -               uc_fw->path = __override_huc_firmware_path();
> +               path = __override_huc_firmware_path();
>                 break;
>         }
>  
> -       uc_fw->user_overridden = uc_fw->path;
> -       return uc_fw->user_overridden;
> +       if (unlikely(path)) {
> +               uc_fw->path = path;
> +               uc_fw->user_overridden = true;
> +       }

The code got longer for no real improvement in clarity?
Oh, because now uc_fw->path may be non-NULL on entry due to call
reordering.

>  }
>  
>  /**
> @@ -194,8 +197,10 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
>  
>         uc_fw->type = type;
>  
> -       if (supported && likely(!__uc_fw_override(uc_fw)))
> +       if (supported) {
>                 __uc_fw_auto_select(uc_fw, platform, rev);
> +               __uc_fw_user_override(uc_fw);
> +       }
>  
>         if (uc_fw->path && *uc_fw->path)
>                 uc_fw->status = INTEL_UC_FIRMWARE_SELECTED;
> @@ -203,6 +208,41 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
>                 uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>  }
>  
> +static void __force_fw_fetch_failures(struct intel_uc_fw *uc_fw,
> +                                     struct drm_i915_private *i915, bool user)
> +{
> +       int e = user ? -EINVAL : -ESTALE;
> +
> +       if (i915_inject_load_error(i915, e)) {
> +               /* non-existing blob */
> +               uc_fw->path = "<invalid>";
> +               uc_fw->user_overridden = user;
> +       } else if (i915_inject_load_error(i915, e)) {
> +               /* require next major version */
> +               uc_fw->major_ver_wanted += 1;
> +               uc_fw->minor_ver_wanted = 0;
> +               uc_fw->user_overridden = user;
> +       } else if (i915_inject_load_error(i915, e)) {
> +               /* require next minor version */
> +               uc_fw->minor_ver_wanted += 1;
> +               uc_fw->user_overridden = user;
> +       } else if (uc_fw->major_ver_wanted && i915_inject_load_error(i915, e)) {
> +               /* require prev major version */
> +               uc_fw->major_ver_wanted -= 1;
> +               uc_fw->minor_ver_wanted = 0;
> +               uc_fw->user_overridden = user;
> +       } else if (uc_fw->minor_ver_wanted && i915_inject_load_error(i915, e)) {
> +               /* require prev minor version - hey, this should work! */
> +               uc_fw->minor_ver_wanted -= 1;
> +               uc_fw->user_overridden = user;
> +       } else if (user && i915_inject_load_error(i915, e)) {
> +               /* officially unsupported platform */
> +               uc_fw->major_ver_wanted = 0;
> +               uc_fw->minor_ver_wanted = 0;
> +               uc_fw->user_overridden = true;
> +       }

Taking self-abuse to a whole new level :)

> +}
> +
>  /**
>   * intel_uc_fw_fetch - fetch uC firmware
>   * @uc_fw: uC firmware
> @@ -222,17 +262,22 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
>  
>         GEM_BUG_ON(!intel_uc_fw_supported(uc_fw));
>  
> +       err = i915_inject_load_error(i915, -ENXIO);
> +       if (err)
> +               return err;
> +
> +       __force_fw_fetch_failures(uc_fw, i915, true);
> +       __force_fw_fetch_failures(uc_fw, i915, false);

Ok. The set of major/minor/user_override make some sort of sense to me,
but I couldn't say if that's a complete set or not.

> @@ -292,29 +343,22 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
>                 break;
>         }
>  
> -       DRM_DEBUG_DRIVER("%s fw version %u.%u (wanted %u.%u)\n",
> -                        intel_uc_fw_type_repr(uc_fw->type),
> -                        uc_fw->major_ver_found, uc_fw->minor_ver_found,
> -                        uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> -
> -       if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) {
> -               DRM_NOTE("%s: Skipping firmware version check\n",
> -                        intel_uc_fw_type_repr(uc_fw->type));
> -       } else if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> -                  uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
> -               DRM_NOTE("%s: Wrong firmware version (%u.%u, required %u.%u)\n",
> -                        intel_uc_fw_type_repr(uc_fw->type),
> +       if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> +           uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
> +               dev_warn(i915->drm.dev,

Still a dev_notice(), especially as this is still user controllable.

> +                        "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
> +                        intel_uc_fw_type_repr(uc_fw->type), uc_fw->path,
>                          uc_fw->major_ver_found, uc_fw->minor_ver_found,
>                          uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> -               err = -ENOEXEC;
> -               goto fail;
> +               if (!intel_uc_fw_is_overridden(uc_fw)) {
> +                       err = -ENOEXEC;
> +                       goto fail;
> +               }
>         }

> -       DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
> +       dev_info(i915->drm.dev, "%s firmware %s: fetch failed with error %d\n",
>                  intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);

WARN -> info ? Not at least a notice?

> -       DRM_INFO("%s: Firmware can be downloaded from %s\n",
> +       dev_info(i915->drm.dev, "%s firmware(s) can be downloaded from %s\n",
>                  intel_uc_fw_type_repr(uc_fw->type), INTEL_UC_FIRMWARE_URL);
>  
>         release_firmware(fw);           /* OK even if fw is NULL */

Just a question of log levels.
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index 3a3803bfa5a8..4faff1010398 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -153,20 +153,23 @@  static const char *__override_huc_firmware_path(void)
 	return "";
 }
 
-static bool
-__uc_fw_override(struct intel_uc_fw *uc_fw)
+static void __uc_fw_user_override(struct intel_uc_fw *uc_fw)
 {
+	const char *path = NULL;
+
 	switch (uc_fw->type) {
 	case INTEL_UC_FW_TYPE_GUC:
-		uc_fw->path = __override_guc_firmware_path();
+		path = __override_guc_firmware_path();
 		break;
 	case INTEL_UC_FW_TYPE_HUC:
-		uc_fw->path = __override_huc_firmware_path();
+		path = __override_huc_firmware_path();
 		break;
 	}
 
-	uc_fw->user_overridden = uc_fw->path;
-	return uc_fw->user_overridden;
+	if (unlikely(path)) {
+		uc_fw->path = path;
+		uc_fw->user_overridden = true;
+	}
 }
 
 /**
@@ -194,8 +197,10 @@  void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
 
 	uc_fw->type = type;
 
-	if (supported && likely(!__uc_fw_override(uc_fw)))
+	if (supported) {
 		__uc_fw_auto_select(uc_fw, platform, rev);
+		__uc_fw_user_override(uc_fw);
+	}
 
 	if (uc_fw->path && *uc_fw->path)
 		uc_fw->status = INTEL_UC_FIRMWARE_SELECTED;
@@ -203,6 +208,41 @@  void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
 		uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
 }
 
+static void __force_fw_fetch_failures(struct intel_uc_fw *uc_fw,
+				      struct drm_i915_private *i915, bool user)
+{
+	int e = user ? -EINVAL : -ESTALE;
+
+	if (i915_inject_load_error(i915, e)) {
+		/* non-existing blob */
+		uc_fw->path = "<invalid>";
+		uc_fw->user_overridden = user;
+	} else if (i915_inject_load_error(i915, e)) {
+		/* require next major version */
+		uc_fw->major_ver_wanted += 1;
+		uc_fw->minor_ver_wanted = 0;
+		uc_fw->user_overridden = user;
+	} else if (i915_inject_load_error(i915, e)) {
+		/* require next minor version */
+		uc_fw->minor_ver_wanted += 1;
+		uc_fw->user_overridden = user;
+	} else if (uc_fw->major_ver_wanted && i915_inject_load_error(i915, e)) {
+		/* require prev major version */
+		uc_fw->major_ver_wanted -= 1;
+		uc_fw->minor_ver_wanted = 0;
+		uc_fw->user_overridden = user;
+	} else if (uc_fw->minor_ver_wanted && i915_inject_load_error(i915, e)) {
+		/* require prev minor version - hey, this should work! */
+		uc_fw->minor_ver_wanted -= 1;
+		uc_fw->user_overridden = user;
+	} else if (user && i915_inject_load_error(i915, e)) {
+		/* officially unsupported platform */
+		uc_fw->major_ver_wanted = 0;
+		uc_fw->minor_ver_wanted = 0;
+		uc_fw->user_overridden = true;
+	}
+}
+
 /**
  * intel_uc_fw_fetch - fetch uC firmware
  * @uc_fw: uC firmware
@@ -222,17 +262,22 @@  int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
 
 	GEM_BUG_ON(!intel_uc_fw_supported(uc_fw));
 
+	err = i915_inject_load_error(i915, -ENXIO);
+	if (err)
+		return err;
+
+	__force_fw_fetch_failures(uc_fw, i915, true);
+	__force_fw_fetch_failures(uc_fw, i915, false);
+
 	err = request_firmware(&fw, uc_fw->path, i915->drm.dev);
 	if (err)
 		goto fail;
 
-	DRM_DEBUG_DRIVER("%s fw size %zu ptr %p\n",
-			 intel_uc_fw_type_repr(uc_fw->type), fw->size, fw);
-
 	/* Check the size of the blob before examining buffer contents */
-	if (fw->size < sizeof(struct uc_css_header)) {
-		DRM_WARN("%s: Unexpected firmware size (%zu, min %zu)\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
+	if (unlikely(fw->size < sizeof(struct uc_css_header))) {
+		dev_warn(i915->drm.dev,
+			 "%s firmware %s: invalid size: %zu < %zu\n",
+			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path,
 			 fw->size, sizeof(struct uc_css_header));
 		err = -ENODATA;
 		goto fail;
@@ -243,10 +288,12 @@  int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
 	/* Check integrity of size values inside CSS header */
 	size = (css->header_size_dw - css->key_size_dw - css->modulus_size_dw -
 		css->exponent_size_dw) * sizeof(u32);
-	if (size != sizeof(struct uc_css_header)) {
-		DRM_WARN("%s: Mismatched firmware header definition\n",
-			 intel_uc_fw_type_repr(uc_fw->type));
-		err = -ENOEXEC;
+	if (unlikely(size != sizeof(struct uc_css_header))) {
+		dev_warn(i915->drm.dev,
+			 "%s firmware %s: unexpected header size: %zu != %zu\n",
+			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path,
+			 fw->size, sizeof(struct uc_css_header));
+		err = -EPROTO;
 		goto fail;
 	}
 
@@ -254,19 +301,23 @@  int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
 	uc_fw->ucode_size = (css->size_dw - css->header_size_dw) * sizeof(u32);
 
 	/* now RSA */
-	if (css->key_size_dw != UOS_RSA_SCRATCH_COUNT) {
-		DRM_WARN("%s: Mismatched firmware RSA key size (%u)\n",
-			 intel_uc_fw_type_repr(uc_fw->type), css->key_size_dw);
-		err = -ENOEXEC;
+	if (unlikely(css->key_size_dw != UOS_RSA_SCRATCH_COUNT)) {
+		dev_warn(i915->drm.dev,
+			 "%s firmware %s: unexpected RSA key size: %u != %u\n",
+			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path,
+			 css->key_size_dw, UOS_RSA_SCRATCH_COUNT);
+		err = -EPROTO;
 		goto fail;
 	}
 	uc_fw->rsa_size = css->key_size_dw * sizeof(u32);
 
 	/* At least, it should have header, uCode and RSA. Size of all three. */
 	size = sizeof(struct uc_css_header) + uc_fw->ucode_size + uc_fw->rsa_size;
-	if (fw->size < size) {
-		DRM_WARN("%s: Truncated firmware (%zu, expected %zu)\n",
-			 intel_uc_fw_type_repr(uc_fw->type), fw->size, size);
+	if (unlikely(fw->size < size)) {
+		dev_warn(i915->drm.dev,
+			 "%s firmware %s: invalid size: %zu < %zu\n",
+			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path,
+			 fw->size, size);
 		err = -ENOEXEC;
 		goto fail;
 	}
@@ -292,29 +343,22 @@  int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
 		break;
 	}
 
-	DRM_DEBUG_DRIVER("%s fw version %u.%u (wanted %u.%u)\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
-			 uc_fw->major_ver_found, uc_fw->minor_ver_found,
-			 uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
-
-	if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) {
-		DRM_NOTE("%s: Skipping firmware version check\n",
-			 intel_uc_fw_type_repr(uc_fw->type));
-	} else if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
-		   uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
-		DRM_NOTE("%s: Wrong firmware version (%u.%u, required %u.%u)\n",
-			 intel_uc_fw_type_repr(uc_fw->type),
+	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
+	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
+		dev_warn(i915->drm.dev,
+			 "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
+			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path,
 			 uc_fw->major_ver_found, uc_fw->minor_ver_found,
 			 uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
-		err = -ENOEXEC;
-		goto fail;
+		if (!intel_uc_fw_is_overridden(uc_fw)) {
+			err = -ENOEXEC;
+			goto fail;
+		}
 	}
 
 	obj = i915_gem_object_create_shmem_from_data(i915, fw->data, fw->size);
 	if (IS_ERR(obj)) {
 		err = PTR_ERR(obj);
-		DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
-				 intel_uc_fw_type_repr(uc_fw->type), err);
 		goto fail;
 	}
 
@@ -322,15 +366,22 @@  int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
 	uc_fw->size = fw->size;
 	uc_fw->status = INTEL_UC_FIRMWARE_AVAILABLE;
 
+	DRM_DEV_DEBUG_DRIVER(i915->drm.dev, "%s firmware %s: %s\n",
+			     intel_uc_fw_type_repr(uc_fw->type), uc_fw->path,
+			     intel_uc_fw_status_repr(uc_fw->status));
+
 	release_firmware(fw);
 	return 0;
 
 fail:
-	uc_fw->status = INTEL_UC_FIRMWARE_MISSING;
+	if (err == -ENOENT)
+		uc_fw->status = INTEL_UC_FIRMWARE_MISSING;
+	else
+		uc_fw->status = INTEL_UC_FIRMWARE_ERROR;
 
-	DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
+	dev_info(i915->drm.dev, "%s firmware %s: fetch failed with error %d\n",
 		 intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
-	DRM_INFO("%s: Firmware can be downloaded from %s\n",
+	dev_info(i915->drm.dev, "%s firmware(s) can be downloaded from %s\n",
 		 intel_uc_fw_type_repr(uc_fw->type), INTEL_UC_FIRMWARE_URL);
 
 	release_firmware(fw);		/* OK even if fw is NULL */
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
index fae45bc16bc7..0d22e73dff15 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -42,6 +42,7 @@  enum intel_uc_fw_status {
 	INTEL_UC_FIRMWARE_UNINITIALIZED = 0, /* used to catch checks done too early */
 	INTEL_UC_FIRMWARE_SELECTED, /* selected the blob we want to load */
 	INTEL_UC_FIRMWARE_MISSING, /* blob not found on the system */
+	INTEL_UC_FIRMWARE_ERROR, /* invalid format or version */
 	INTEL_UC_FIRMWARE_AVAILABLE, /* blob found and copied in mem */
 	INTEL_UC_FIRMWARE_FAIL, /* failed to xfer or init/auth the fw */
 	INTEL_UC_FIRMWARE_TRANSFERRED, /* dma xfer done */
@@ -92,6 +93,8 @@  const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 		return "SELECTED";
 	case INTEL_UC_FIRMWARE_MISSING:
 		return "MISSING";
+	case INTEL_UC_FIRMWARE_ERROR:
+		return "ERROR";
 	case INTEL_UC_FIRMWARE_AVAILABLE:
 		return "AVAILABLE";
 	case INTEL_UC_FIRMWARE_FAIL: