[Intel-gfx] [PATCH 09/10] drm/i915/dmc: protect against reading random memory

Lucas De Marchi lucas.demarchi at intel.com
Thu May 23 19:41:23 UTC 2019


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 at 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 at 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
>>


More information about the Intel-gfx mailing list