| Message ID | 1c02cc0db18863120520926f45db4c6e320a21b8.1440165040.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 21, 2015 at 04:52:01PM +0300, Jani Nikula wrote: > From: David Weinehall <david.weinehall@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@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@linux.intel.com> > Signed-off-by: Jani Nikula <jani.nikula@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@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
On Fri, 21 Aug 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Aug 21, 2015 at 04:52:01PM +0300, Jani Nikula wrote: >> From: David Weinehall <david.weinehall@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@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@linux.intel.com> >> Signed-off-by: Jani Nikula <jani.nikula@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@linux.intel.com> Pushed to drm-intel-next-fixes, thanks for the review. BR, Jani. > >> } >> 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
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))); } 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;