| Message ID | 20190605235535.17791-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dmc: protect against reading random memory | expand |
On Wed, Jun 05, 2019 at 04:55:35PM -0700, Lucas De Marchi wrote: > While loading the DMC firmware we were double checking the headers made > sense, but in no place we checked that we were actually reading memory > we were supposed to. This could be wrong in case the firmware file is > truncated or malformed. > > Before this patch: > # ls -l /lib/firmware/i915/icl_dmc_ver1_07.bin > -rw-r--r-- 1 root root 25716 Feb 1 12:26 icl_dmc_ver1_07.bin > # truncate -s 25700 /lib/firmware/i915/icl_dmc_ver1_07.bin > # modprobe i915 > # dmesg| grep -i dmc > [drm:intel_csr_ucode_init [i915]] Loading i915/icl_dmc_ver1_07.bin > [drm] Finished loading DMC firmware i915/icl_dmc_ver1_07.bin (v1.7) > > i.e. it loads random data. Now it fails like below: > [drm:intel_csr_ucode_init [i915]] Loading i915/icl_dmc_ver1_07.bin > [drm:csr_load_work_fn [i915]] *ERROR* Truncated DMC firmware, rejecting. > i915 0000:00:02.0: Failed to load DMC firmware i915/icl_dmc_ver1_07.bin. Disabling runtime power management. > i915 0000:00:02.0: DMC firmware homepage: https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915 > > Before reading any part of the firmware file, validate the input first. > > Fixes: eb805623d8b1 ("drm/i915/skl: Add support to load SKL CSR firmware.") > Cc: stable@vger.kernel.org > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > > This has been extracted from the bigger series > https://patchwork.freedesktop.org/series/61016/ in a way that can be > propagated to stable. Thanks! > > drivers/gpu/drm/i915/intel_csr.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index 4527b9662330..bf0eebd385b9 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -303,10 +303,17 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > u32 dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes; > u32 i; > u32 *dmc_payload; > + size_t fsize; > > if (!fw) > return NULL; > > + fsize = sizeof(struct intel_css_header) + > + sizeof(struct intel_package_header) + > + sizeof(struct intel_dmc_header); > + if (fsize > fw->size) > + goto error_truncated; > + > /* Extract CSS Header information*/ > css_header = (struct intel_css_header *)fw->data; > if (sizeof(struct intel_css_header) != > @@ -366,6 +373,9 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > /* Convert dmc_offset into number of bytes. By default it is in dwords*/ > dmc_offset *= 4; > readcount += dmc_offset; > + fsize += dmc_offset; > + if (fsize > fw->size) > + goto error_truncated; > > /* Extract dmc_header information. */ > dmc_header = (struct intel_dmc_header *)&fw->data[readcount]; > @@ -397,6 +407,10 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > > /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */ > nbytes = dmc_header->fw_size * 4; > + fsize += nbytes; > + if (fsize > fw->size) > + goto error_truncated; > + > if (nbytes > csr->max_fw_size) { > DRM_ERROR("DMC FW too big (%u bytes)\n", nbytes); > return NULL; > @@ -410,6 +424,10 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > } > > return memcpy(dmc_payload, &fw->data[readcount], nbytes); > + > +error_truncated: > + DRM_ERROR("Truncated DMC firmware, rejecting.\n"); > + return NULL; > } > > static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv) > -- > 2.21.0 >
On Wed, Jun 05, 2019 at 04:55:35PM -0700, Lucas De Marchi wrote: > While loading the DMC firmware we were double checking the headers made > sense, but in no place we checked that we were actually reading memory > we were supposed to. This could be wrong in case the firmware file is > truncated or malformed. Random idea: Should we hash the file and store the hash in the driver? If the file gets corrupted in other ways this would prevent us feeding garbage to the dmc. Although it would then make testing firmware modifications a bit more tedious as you'd have to adjust the hash or bypass the check somehow. > > Before this patch: > # ls -l /lib/firmware/i915/icl_dmc_ver1_07.bin > -rw-r--r-- 1 root root 25716 Feb 1 12:26 icl_dmc_ver1_07.bin > # truncate -s 25700 /lib/firmware/i915/icl_dmc_ver1_07.bin > # modprobe i915 > # dmesg| grep -i dmc > [drm:intel_csr_ucode_init [i915]] Loading i915/icl_dmc_ver1_07.bin > [drm] Finished loading DMC firmware i915/icl_dmc_ver1_07.bin (v1.7) > > i.e. it loads random data. Now it fails like below: > [drm:intel_csr_ucode_init [i915]] Loading i915/icl_dmc_ver1_07.bin > [drm:csr_load_work_fn [i915]] *ERROR* Truncated DMC firmware, rejecting. > i915 0000:00:02.0: Failed to load DMC firmware i915/icl_dmc_ver1_07.bin. Disabling runtime power management. > i915 0000:00:02.0: DMC firmware homepage: https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915 > > Before reading any part of the firmware file, validate the input first. > > Fixes: eb805623d8b1 ("drm/i915/skl: Add support to load SKL CSR firmware.") > Cc: stable@vger.kernel.org > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > > This has been extracted from the bigger series > https://patchwork.freedesktop.org/series/61016/ in a way that can be > propagated to stable. > > drivers/gpu/drm/i915/intel_csr.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index 4527b9662330..bf0eebd385b9 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -303,10 +303,17 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > u32 dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes; > u32 i; > u32 *dmc_payload; > + size_t fsize; > > if (!fw) > return NULL; > > + fsize = sizeof(struct intel_css_header) + > + sizeof(struct intel_package_header) + > + sizeof(struct intel_dmc_header); > + if (fsize > fw->size) > + goto error_truncated; > + > /* Extract CSS Header information*/ > css_header = (struct intel_css_header *)fw->data; > if (sizeof(struct intel_css_header) != > @@ -366,6 +373,9 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > /* Convert dmc_offset into number of bytes. By default it is in dwords*/ > dmc_offset *= 4; > readcount += dmc_offset; > + fsize += dmc_offset; > + if (fsize > fw->size) > + goto error_truncated; > > /* Extract dmc_header information. */ > dmc_header = (struct intel_dmc_header *)&fw->data[readcount]; > @@ -397,6 +407,10 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > > /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */ > nbytes = dmc_header->fw_size * 4; > + fsize += nbytes; > + if (fsize > fw->size) > + goto error_truncated; > + > if (nbytes > csr->max_fw_size) { > DRM_ERROR("DMC FW too big (%u bytes)\n", nbytes); > return NULL; > @@ -410,6 +424,10 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, > } > > return memcpy(dmc_payload, &fw->data[readcount], nbytes); > + > +error_truncated: > + DRM_ERROR("Truncated DMC firmware, rejecting.\n"); > + return NULL; > } > > static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv) > -- > 2.21.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 4527b9662330..bf0eebd385b9 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -303,10 +303,17 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, u32 dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes; u32 i; u32 *dmc_payload; + size_t fsize; if (!fw) return NULL; + fsize = sizeof(struct intel_css_header) + + sizeof(struct intel_package_header) + + sizeof(struct intel_dmc_header); + if (fsize > fw->size) + goto error_truncated; + /* Extract CSS Header information*/ css_header = (struct intel_css_header *)fw->data; if (sizeof(struct intel_css_header) != @@ -366,6 +373,9 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, /* Convert dmc_offset into number of bytes. By default it is in dwords*/ dmc_offset *= 4; readcount += dmc_offset; + fsize += dmc_offset; + if (fsize > fw->size) + goto error_truncated; /* Extract dmc_header information. */ dmc_header = (struct intel_dmc_header *)&fw->data[readcount]; @@ -397,6 +407,10 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */ nbytes = dmc_header->fw_size * 4; + fsize += nbytes; + if (fsize > fw->size) + goto error_truncated; + if (nbytes > csr->max_fw_size) { DRM_ERROR("DMC FW too big (%u bytes)\n", nbytes); return NULL; @@ -410,6 +424,10 @@ static u32 *parse_csr_fw(struct drm_i915_private *dev_priv, } return memcpy(dmc_payload, &fw->data[readcount], nbytes); + +error_truncated: + DRM_ERROR("Truncated DMC firmware, rejecting.\n"); + return NULL; } static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv)
While loading the DMC firmware we were double checking the headers made sense, but in no place we checked that we were actually reading memory we were supposed to. This could be wrong in case the firmware file is truncated or malformed. Before this patch: # ls -l /lib/firmware/i915/icl_dmc_ver1_07.bin -rw-r--r-- 1 root root 25716 Feb 1 12:26 icl_dmc_ver1_07.bin # truncate -s 25700 /lib/firmware/i915/icl_dmc_ver1_07.bin # modprobe i915 # dmesg| grep -i dmc [drm:intel_csr_ucode_init [i915]] Loading i915/icl_dmc_ver1_07.bin [drm] Finished loading DMC firmware i915/icl_dmc_ver1_07.bin (v1.7) i.e. it loads random data. Now it fails like below: [drm:intel_csr_ucode_init [i915]] Loading i915/icl_dmc_ver1_07.bin [drm:csr_load_work_fn [i915]] *ERROR* Truncated DMC firmware, rejecting. i915 0000:00:02.0: Failed to load DMC firmware i915/icl_dmc_ver1_07.bin. Disabling runtime power management. i915 0000:00:02.0: DMC firmware homepage: https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915 Before reading any part of the firmware file, validate the input first. Fixes: eb805623d8b1 ("drm/i915/skl: Add support to load SKL CSR firmware.") Cc: stable@vger.kernel.org Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- This has been extracted from the bigger series https://patchwork.freedesktop.org/series/61016/ in a way that can be propagated to stable. drivers/gpu/drm/i915/intel_csr.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)