| Message ID | 20190523082420.10352-9-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] drm/i915/dmc: use kernel types | expand |
On Thu, May 23, 2019 at 01:24:19AM -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. > > Before parsing any part of the firmware, validate the input first. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> I wonder if this patch should be the first one on the series for easy backport if needed. Maybe cc: Stable? Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_csr.c | 71 ++++++++++++++++++++++++-------- > 1 file changed, 53 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index 7ff08de83cc6..b7181ca6c8f5 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -360,7 +360,8 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info, > } > > static u32 parse_csr_fw_dmc(struct intel_csr *csr, > - const struct intel_dmc_header_base *dmc_header) > + const struct intel_dmc_header_base *dmc_header, > + size_t rem_size) > { > unsigned int header_len_bytes, dmc_header_size, payload_size, i; > const u32 *mmioaddr, *mmiodata; > @@ -375,6 +376,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, > const struct intel_dmc_header_v3 *v3 = > (const struct intel_dmc_header_v3 *)dmc_header; > > + if (rem_size < sizeof(struct intel_dmc_header_v3)) > + goto error_truncated; > + > mmioaddr = v3->mmioaddr; > mmiodata = v3->mmiodata; > mmio_count = v3->mmio_count; > @@ -386,6 +390,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, > const struct intel_dmc_header_v1 *v1 = > (const struct intel_dmc_header_v1 *)dmc_header; > > + if (rem_size < sizeof(struct intel_dmc_header_v1)) > + goto error_truncated; > + > mmioaddr = v1->mmioaddr; > mmiodata = v1->mmiodata; > mmio_count = v1->mmio_count; > @@ -404,6 +411,8 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, > return 0; > } > > + rem_size -= header_len_bytes; > + > /* Cache the dmc header info. */ > if (mmio_count > mmio_count_max) { > DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count); > @@ -424,6 +433,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, > > /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */ > payload_size = dmc_header->fw_size * 4; > + if (rem_size < payload_size) > + goto error_truncated; > + > if (payload_size > csr->max_fw_size) { > DRM_ERROR("DMC FW too big (%u bytes)\n", payload_size); > return 0; > @@ -440,16 +452,25 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, > memcpy(csr->dmc_payload, payload, payload_size); > > return header_len_bytes + payload_size; > + > +error_truncated: > + DRM_ERROR("Truncated DMC firmware, refusing.\n"); > + return 0; > } > > static u32 > parse_csr_fw_package(struct intel_csr *csr, > const struct intel_package_header *package_header, > - const struct stepping_info *si) > + const struct stepping_info *si, > + size_t rem_size) > { > - u32 num_entries, max_entries, dmc_offset, r; > + u32 package_size = sizeof(struct intel_package_header); > + u32 num_entries, max_entries, dmc_offset; > const struct intel_fw_info *fw_info; > > + if (rem_size < package_size) > + goto error_truncated; > + > if (package_header->header_ver == 1) { > max_entries = PACKAGE_MAX_FW_INFO_ENTRIES; > } else if (package_header->header_ver == 2) { > @@ -460,11 +481,17 @@ parse_csr_fw_package(struct intel_csr *csr, > return 0; > } > > - if (package_header->header_len * 4 != > - sizeof(struct intel_package_header) + > - max_entries * sizeof(struct intel_fw_info)) { > + /* > + * We should always have space for max_entries, > + * even if not all are used > + */ > + package_size += max_entries * sizeof(struct intel_fw_info); > + if (rem_size < package_size) > + goto error_truncated; > + > + if (package_header->header_len * 4 != package_size) { > DRM_ERROR("DMC firmware has wrong package header length " > - "(%u bytes)\n", package_header->header_len * 4); > + "(%u bytes)\n", package_size); > return 0; > } > > @@ -481,21 +508,24 @@ parse_csr_fw_package(struct intel_csr *csr, > return 0; > } > > - r = sizeof(struct intel_package_header); > - > - /* we always have space for max_entries, even if not all are used */ > - r += max_entries * sizeof(struct intel_fw_info); > - > /* dmc_offset is in dwords */ > - r += dmc_offset * 4; > + return package_size + dmc_offset * 4; > > - return r; > +error_truncated: > + DRM_ERROR("Truncated DMC firmware, refusing.\n"); > + return 0; > } > > /* Return number of bytes parsed or 0 on error */ > static u32 parse_csr_fw_css(struct intel_csr *csr, > - struct intel_css_header *css_header) > + struct intel_css_header *css_header, > + size_t rem_size) > { > + if (rem_size < sizeof(struct intel_css_header)) { > + DRM_ERROR("Truncated DMC firmware, refusing.\n"); > + return 0; > + } > + > if (sizeof(struct intel_css_header) != > (css_header->header_len * 4)) { > DRM_ERROR("DMC firmware has wrong CSS header length " > @@ -530,29 +560,34 @@ static void parse_csr_fw(struct drm_i915_private *dev_priv, > const struct stepping_info *si = intel_get_stepping_info(dev_priv); > u32 readcount = 0; > u32 r; > + size_t rem_size; > > if (!fw) > return; > > + rem_size = fw->size; > + > /* Extract CSS Header information*/ > css_header = (struct intel_css_header *)fw->data; > - r = parse_csr_fw_css(csr, css_header); > + r = parse_csr_fw_css(csr, css_header, rem_size); > if (!r) > return; > > + rem_size -= r; > readcount += r; > > /* Extract Package Header information*/ > package_header = (struct intel_package_header *)&fw->data[readcount]; > - r = parse_csr_fw_package(csr, package_header, si); > + r = parse_csr_fw_package(csr, package_header, si, rem_size); > if (!r) > return; > > + rem_size -= r; > readcount += r; > > /* Extract dmc_header information. */ > dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount]; > - parse_csr_fw_dmc(csr, dmc_header); > + parse_csr_fw_dmc(csr, dmc_header, rem_size); > } > > static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv) > -- > 2.21.0 >
On Thu, May 23, 2019 at 11:58:46AM -0700, Rodrigo Vivi wrote: >On Thu, May 23, 2019 at 01:24:19AM -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. >> >> Before parsing any part of the firmware, validate the input first. >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > >I wonder if this patch should be the first one on the series >for easy backport if needed. yeah, too bad I only noticed after doing the first ones. This patch as the first one will be very different, but I agree that is what I need to do. >Maybe cc: Stable? yep. I will wait a little more on review and send a new version with order swapped. > >Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> thanks Lucas De Marchi > >> --- >> drivers/gpu/drm/i915/intel_csr.c | 71 ++++++++++++++++++++++++-------- >> 1 file changed, 53 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c >> index 7ff08de83cc6..b7181ca6c8f5 100644 >> --- a/drivers/gpu/drm/i915/intel_csr.c >> +++ b/drivers/gpu/drm/i915/intel_csr.c >> @@ -360,7 +360,8 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info, >> } >> >> static u32 parse_csr_fw_dmc(struct intel_csr *csr, >> - const struct intel_dmc_header_base *dmc_header) >> + const struct intel_dmc_header_base *dmc_header, >> + size_t rem_size) >> { >> unsigned int header_len_bytes, dmc_header_size, payload_size, i; >> const u32 *mmioaddr, *mmiodata; >> @@ -375,6 +376,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, >> const struct intel_dmc_header_v3 *v3 = >> (const struct intel_dmc_header_v3 *)dmc_header; >> >> + if (rem_size < sizeof(struct intel_dmc_header_v3)) >> + goto error_truncated; >> + >> mmioaddr = v3->mmioaddr; >> mmiodata = v3->mmiodata; >> mmio_count = v3->mmio_count; >> @@ -386,6 +390,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, >> const struct intel_dmc_header_v1 *v1 = >> (const struct intel_dmc_header_v1 *)dmc_header; >> >> + if (rem_size < sizeof(struct intel_dmc_header_v1)) >> + goto error_truncated; >> + >> mmioaddr = v1->mmioaddr; >> mmiodata = v1->mmiodata; >> mmio_count = v1->mmio_count; >> @@ -404,6 +411,8 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, >> return 0; >> } >> >> + rem_size -= header_len_bytes; >> + >> /* Cache the dmc header info. */ >> if (mmio_count > mmio_count_max) { >> DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count); >> @@ -424,6 +433,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, >> >> /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */ >> payload_size = dmc_header->fw_size * 4; >> + if (rem_size < payload_size) >> + goto error_truncated; >> + >> if (payload_size > csr->max_fw_size) { >> DRM_ERROR("DMC FW too big (%u bytes)\n", payload_size); >> return 0; >> @@ -440,16 +452,25 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, >> memcpy(csr->dmc_payload, payload, payload_size); >> >> return header_len_bytes + payload_size; >> + >> +error_truncated: >> + DRM_ERROR("Truncated DMC firmware, refusing.\n"); >> + return 0; >> } >> >> static u32 >> parse_csr_fw_package(struct intel_csr *csr, >> const struct intel_package_header *package_header, >> - const struct stepping_info *si) >> + const struct stepping_info *si, >> + size_t rem_size) >> { >> - u32 num_entries, max_entries, dmc_offset, r; >> + u32 package_size = sizeof(struct intel_package_header); >> + u32 num_entries, max_entries, dmc_offset; >> const struct intel_fw_info *fw_info; >> >> + if (rem_size < package_size) >> + goto error_truncated; >> + >> if (package_header->header_ver == 1) { >> max_entries = PACKAGE_MAX_FW_INFO_ENTRIES; >> } else if (package_header->header_ver == 2) { >> @@ -460,11 +481,17 @@ parse_csr_fw_package(struct intel_csr *csr, >> return 0; >> } >> >> - if (package_header->header_len * 4 != >> - sizeof(struct intel_package_header) + >> - max_entries * sizeof(struct intel_fw_info)) { >> + /* >> + * We should always have space for max_entries, >> + * even if not all are used >> + */ >> + package_size += max_entries * sizeof(struct intel_fw_info); >> + if (rem_size < package_size) >> + goto error_truncated; >> + >> + if (package_header->header_len * 4 != package_size) { >> DRM_ERROR("DMC firmware has wrong package header length " >> - "(%u bytes)\n", package_header->header_len * 4); >> + "(%u bytes)\n", package_size); >> return 0; >> } >> >> @@ -481,21 +508,24 @@ parse_csr_fw_package(struct intel_csr *csr, >> return 0; >> } >> >> - r = sizeof(struct intel_package_header); >> - >> - /* we always have space for max_entries, even if not all are used */ >> - r += max_entries * sizeof(struct intel_fw_info); >> - >> /* dmc_offset is in dwords */ >> - r += dmc_offset * 4; >> + return package_size + dmc_offset * 4; >> >> - return r; >> +error_truncated: >> + DRM_ERROR("Truncated DMC firmware, refusing.\n"); >> + return 0; >> } >> >> /* Return number of bytes parsed or 0 on error */ >> static u32 parse_csr_fw_css(struct intel_csr *csr, >> - struct intel_css_header *css_header) >> + struct intel_css_header *css_header, >> + size_t rem_size) >> { >> + if (rem_size < sizeof(struct intel_css_header)) { >> + DRM_ERROR("Truncated DMC firmware, refusing.\n"); >> + return 0; >> + } >> + >> if (sizeof(struct intel_css_header) != >> (css_header->header_len * 4)) { >> DRM_ERROR("DMC firmware has wrong CSS header length " >> @@ -530,29 +560,34 @@ static void parse_csr_fw(struct drm_i915_private *dev_priv, >> const struct stepping_info *si = intel_get_stepping_info(dev_priv); >> u32 readcount = 0; >> u32 r; >> + size_t rem_size; >> >> if (!fw) >> return; >> >> + rem_size = fw->size; >> + >> /* Extract CSS Header information*/ >> css_header = (struct intel_css_header *)fw->data; >> - r = parse_csr_fw_css(csr, css_header); >> + r = parse_csr_fw_css(csr, css_header, rem_size); >> if (!r) >> return; >> >> + rem_size -= r; >> readcount += r; >> >> /* Extract Package Header information*/ >> package_header = (struct intel_package_header *)&fw->data[readcount]; >> - r = parse_csr_fw_package(csr, package_header, si); >> + r = parse_csr_fw_package(csr, package_header, si, rem_size); >> if (!r) >> return; >> >> + rem_size -= r; >> readcount += r; >> >> /* Extract dmc_header information. */ >> dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount]; >> - parse_csr_fw_dmc(csr, dmc_header); >> + parse_csr_fw_dmc(csr, dmc_header, rem_size); >> } >> >> static void intel_csr_runtime_pm_get(struct drm_i915_private *dev_priv) >> -- >> 2.21.0 >>
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index 7ff08de83cc6..b7181ca6c8f5 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -360,7 +360,8 @@ static u32 find_dmc_fw_offset(const struct intel_fw_info *fw_info, } static u32 parse_csr_fw_dmc(struct intel_csr *csr, - const struct intel_dmc_header_base *dmc_header) + const struct intel_dmc_header_base *dmc_header, + size_t rem_size) { unsigned int header_len_bytes, dmc_header_size, payload_size, i; const u32 *mmioaddr, *mmiodata; @@ -375,6 +376,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, const struct intel_dmc_header_v3 *v3 = (const struct intel_dmc_header_v3 *)dmc_header; + if (rem_size < sizeof(struct intel_dmc_header_v3)) + goto error_truncated; + mmioaddr = v3->mmioaddr; mmiodata = v3->mmiodata; mmio_count = v3->mmio_count; @@ -386,6 +390,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, const struct intel_dmc_header_v1 *v1 = (const struct intel_dmc_header_v1 *)dmc_header; + if (rem_size < sizeof(struct intel_dmc_header_v1)) + goto error_truncated; + mmioaddr = v1->mmioaddr; mmiodata = v1->mmiodata; mmio_count = v1->mmio_count; @@ -404,6 +411,8 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, return 0; } + rem_size -= header_len_bytes; + /* Cache the dmc header info. */ if (mmio_count > mmio_count_max) { DRM_ERROR("DMC firmware has wrong mmio count %u\n", mmio_count); @@ -424,6 +433,9 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */ payload_size = dmc_header->fw_size * 4; + if (rem_size < payload_size) + goto error_truncated; + if (payload_size > csr->max_fw_size) { DRM_ERROR("DMC FW too big (%u bytes)\n", payload_size); return 0; @@ -440,16 +452,25 @@ static u32 parse_csr_fw_dmc(struct intel_csr *csr, memcpy(csr->dmc_payload, payload, payload_size); return header_len_bytes + payload_size; + +error_truncated: + DRM_ERROR("Truncated DMC firmware, refusing.\n"); + return 0; } static u32 parse_csr_fw_package(struct intel_csr *csr, const struct intel_package_header *package_header, - const struct stepping_info *si) + const struct stepping_info *si, + size_t rem_size) { - u32 num_entries, max_entries, dmc_offset, r; + u32 package_size = sizeof(struct intel_package_header); + u32 num_entries, max_entries, dmc_offset; const struct intel_fw_info *fw_info; + if (rem_size < package_size) + goto error_truncated; + if (package_header->header_ver == 1) { max_entries = PACKAGE_MAX_FW_INFO_ENTRIES; } else if (package_header->header_ver == 2) { @@ -460,11 +481,17 @@ parse_csr_fw_package(struct intel_csr *csr, return 0; } - if (package_header->header_len * 4 != - sizeof(struct intel_package_header) + - max_entries * sizeof(struct intel_fw_info)) { + /* + * We should always have space for max_entries, + * even if not all are used + */ + package_size += max_entries * sizeof(struct intel_fw_info); + if (rem_size < package_size) + goto error_truncated; + + if (package_header->header_len * 4 != package_size) { DRM_ERROR("DMC firmware has wrong package header length " - "(%u bytes)\n", package_header->header_len * 4); + "(%u bytes)\n", package_size); return 0; } @@ -481,21 +508,24 @@ parse_csr_fw_package(struct intel_csr *csr, return 0; } - r = sizeof(struct intel_package_header); - - /* we always have space for max_entries, even if not all are used */ - r += max_entries * sizeof(struct intel_fw_info); - /* dmc_offset is in dwords */ - r += dmc_offset * 4; + return package_size + dmc_offset * 4; - return r; +error_truncated: + DRM_ERROR("Truncated DMC firmware, refusing.\n"); + return 0; } /* Return number of bytes parsed or 0 on error */ static u32 parse_csr_fw_css(struct intel_csr *csr, - struct intel_css_header *css_header) + struct intel_css_header *css_header, + size_t rem_size) { + if (rem_size < sizeof(struct intel_css_header)) { + DRM_ERROR("Truncated DMC firmware, refusing.\n"); + return 0; + } + if (sizeof(struct intel_css_header) != (css_header->header_len * 4)) { DRM_ERROR("DMC firmware has wrong CSS header length " @@ -530,29 +560,34 @@ static void parse_csr_fw(struct drm_i915_private *dev_priv, const struct stepping_info *si = intel_get_stepping_info(dev_priv); u32 readcount = 0; u32 r; + size_t rem_size; if (!fw) return; + rem_size = fw->size; + /* Extract CSS Header information*/ css_header = (struct intel_css_header *)fw->data; - r = parse_csr_fw_css(csr, css_header); + r = parse_csr_fw_css(csr, css_header, rem_size); if (!r) return; + rem_size -= r; readcount += r; /* Extract Package Header information*/ package_header = (struct intel_package_header *)&fw->data[readcount]; - r = parse_csr_fw_package(csr, package_header, si); + r = parse_csr_fw_package(csr, package_header, si, rem_size); if (!r) return; + rem_size -= r; readcount += r; /* Extract dmc_header information. */ dmc_header = (struct intel_dmc_header_base *)&fw->data[readcount]; - parse_csr_fw_dmc(csr, dmc_header); + parse_csr_fw_dmc(csr, dmc_header, rem_size); } 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. Before parsing any part of the firmware, validate the input first. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/intel_csr.c | 71 ++++++++++++++++++++++++-------- 1 file changed, 53 insertions(+), 18 deletions(-)