[Intel-gfx] [PATCH 1/1] drm/i915: Allow parsing of variable size child device entries from VBT

[Ville Syrjälä]

On Fri, Aug 21, 2015 at 04:52:01PM +0300, Jani Nikula wrote:
> From: David Weinehall <david.weinehall at linux.intel.com>
> 
> VBT version 196 increased the size of common_child_dev_config. The
> parser code assumed that the size of this structure would not change.
> 
> The modified code now copies the amount needed based on the VBT version,
> and emits a debug message if the VBT version is unknown (too new); since
> the struct config block won't shrink in newer versions it should be
> harmless to copy the maximum known size in such cases, so that's what we
> do, but emitting the warning is probably sensible anyway.
> 
> In the longer run it might make sense to modify the parser code to use a
> version/feature mapping, rather than hardcoding things like this, but
> for now the variants are fairly managable.
> 
> This fixes a regression introduced in
> 
> commit 75067ddecf21271631bc018d2fb23ddd09b66aae
> Author: Antti Koskipaa <antti.koskipaa at linux.intel.com>
> Date:   Fri Jul 10 14:10:55 2015 +0300
> 
>     drm/i915: Per-DDI I_boost override
> 
> since that commit changed the child device config size without updating
> the checks and memcpy.
> 
> v2: Stricter size checks
> 
> v3 by Jani:
> - Keep the checks strict, and warnigns verbose, but keep going anyway.
> - Take care to copy the max amount of child device config we can.
> - Fix the messages.
> 
> Signed-off-by: David Weinehall <david.weinehall at linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 37 +++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_bios.h |  6 ++++--
>  2 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 64e5b15ae0b6..be83b77aa018 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1051,17 +1051,39 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  	const union child_device_config *p_child;
>  	union child_device_config *child_dev_ptr;
>  	int i, child_device_num, count;
> -	u16	block_size;
> +	u8 expected_size;
> +	u16 block_size;
>  
>  	p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
>  	if (!p_defs) {
>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>  		return;
>  	}
> -	if (p_defs->child_dev_size < sizeof(*p_child)) {
> -		DRM_ERROR("General definiton block child device size is too small.\n");
> +	if (bdb->version < 195) {
> +		expected_size = sizeof(struct old_child_dev_config);
> +	} else if (bdb->version == 195) {
> +		expected_size = 37;
> +	} else if (bdb->version <= 197) {
> +		expected_size = 38;
> +	} else {
> +		expected_size = 38;
> +		BUILD_BUG_ON(sizeof(*p_child) < 38);
> +		DRM_DEBUG_DRIVER("Expected child device config size for VBT version %u not known; assuming %u\n",
> +				 bdb->version, expected_size);
> +	}
> +
> +	/* The legacy sized child device config is the minimum we need. */
> +	if (p_defs->child_dev_size < sizeof(struct old_child_dev_config)) {
> +		DRM_ERROR("Child device config size %u is too small.\n",
> +			  p_defs->child_dev_size);
>  		return;
>  	}
> +
> +	/* Flag an error for unexpected size, but continue anyway. */
> +	if (p_defs->child_dev_size != expected_size)
> +		DRM_ERROR("Unexpected child device config size %u (expected %u for VBT version %u)\n",
> +			  p_defs->child_dev_size, expected_size, bdb->version);
> +
>  	/* get the block size of general definitions */
>  	block_size = get_blocksize(p_defs);
>  	/* get the number of child device */
> @@ -1106,7 +1128,14 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  
>  		child_dev_ptr = dev_priv->vbt.child_dev + count;
>  		count++;
> -		memcpy(child_dev_ptr, p_child, sizeof(*p_child));
> +
> +		/*
> +		 * Copy as much as we know (sizeof) and is available
> +		 * (child_dev_size) of the child device. Accessing the data must
> +		 * depend on VBT version.
> +		 */
> +		memcpy(child_dev_ptr, p_child,
> +		       min_t(size_t, p_defs->child_dev_size, sizeof(*p_child)));

Looks good. Could maybe use a
BUILD_BUG_ON(sizeof(struct old_child_dev_config) != 33)
somewhere to make sure people don't make a mess of things.

Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

>  	}
>  	return;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 6d909efbf43f..06d0dbde2be6 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -203,9 +203,11 @@ struct bdb_general_features {
>  #define DEVICE_PORT_DVOB	0x01
>  #define DEVICE_PORT_DVOC	0x02
>  
> -/* We used to keep this struct but without any version control. We should avoid
> +/*
> + * We used to keep this struct but without any version control. We should avoid
>   * using it in the future, but it should be safe to keep using it in the old
> - * code. */
> + * code. Do not change; we rely on its size.
> + */
>  struct old_child_dev_config {
>  	u16 handle;
>  	u16 device_type;
> -- 
> 2.1.4

-- 
Ville Syrjälä
Intel OTC