| Message ID | 20150804135552.GC6150@boom (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote: > 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. > > v2: Stricter size checks > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> Since Chris mentioned that this should fix a regression I applied it to drm-intel-fixes. -Daniel > --- > drivers/gpu/drm/i915/intel_bios.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > index 2ff9eb00fdec..8a1f3e1fc598 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -1015,15 +1015,33 @@ 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 = 33; > + } else if (bdb->version == 195) { > + expected_size = 37; > + } else if (bdb->version <= 197) { > + expected_size = 38; > + } else { > + expected_size = 38; > + DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n", expected_size); > + } > + > + if (expected_size > sizeof(*p_child)) { > + DRM_ERROR("child_device_config cannot fit in p_child\n"); > + return; > + } > + > + if (p_defs->child_dev_size != expected_size) { > + DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n", > + p_defs->child_dev_size, expected_size, bdb->version); > return; > } > /* get the block size of general definitions */ > @@ -1070,7 +1088,7 @@ 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)); > + memcpy(child_dev_ptr, p_child, p_defs->child_dev_size); > } > return; > } > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Aug 05, 2015 at 10:59:00AM +0200, Daniel Vetter wrote: > On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote: > > 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. > > > > v2: Stricter size checks > > > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > Since Chris mentioned that this should fix a regression I applied it to > drm-intel-fixes. Great! Will you merge the other patch in the series to the nightly build? Regards, David
On Wed, Aug 05, 2015 at 06:32:01PM +0300, David Weinehall wrote: > On Wed, Aug 05, 2015 at 10:59:00AM +0200, Daniel Vetter wrote: > > On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote: > > > 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. > > > > > > v2: Stricter size checks > > > > > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > > > Since Chris mentioned that this should fix a regression I applied it to > > drm-intel-fixes. > > Great! Will you merge the other patch in the series to the nightly > build? I only merged this fast-tracked because it fixes a regression. For the other patch normal review rules still apply (i.e. I won't do it). -Daniel
On 8/5/2015 9:59 AM, Daniel Vetter wrote: > On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote: >> 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. >> >> v2: Stricter size checks >> >> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > Since Chris mentioned that this should fix a regression I applied it to > drm-intel-fixes. > -Daniel > >> --- >> drivers/gpu/drm/i915/intel_bios.c | 26 ++++++++++++++++++++++---- >> 1 file changed, 22 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c >> index 2ff9eb00fdec..8a1f3e1fc598 100644 >> --- a/drivers/gpu/drm/i915/intel_bios.c >> +++ b/drivers/gpu/drm/i915/intel_bios.c >> @@ -1015,15 +1015,33 @@ 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 = 33; >> + } else if (bdb->version == 195) { >> + expected_size = 37; >> + } else if (bdb->version <= 197) { >> + expected_size = 38; >> + } else { >> + expected_size = 38; >> + DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n", expected_size); Hi, the line above throws a warning because there are two '%u' but only one variable. Looks like bdb->version should be the 1st printed value. >> + } >> + >> + if (expected_size > sizeof(*p_child)) { >> + DRM_ERROR("child_device_config cannot fit in p_child\n"); >> + return; >> + } >> + >> + if (p_defs->child_dev_size != expected_size) { >> + DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n", >> + p_defs->child_dev_size, expected_size, bdb->version); >> return; >> } >> /* get the block size of general definitions */ >> @@ -1070,7 +1088,7 @@ 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)); >> + memcpy(child_dev_ptr, p_child, p_defs->child_dev_size); >> } >> return; >> } >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Aug 06, 2015 at 02:59:10PM +0100, Michel Thierry wrote: > On 8/5/2015 9:59 AM, Daniel Vetter wrote: > >On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote: > >>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. > >> > >>v2: Stricter size checks > >> > >>Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > > >Since Chris mentioned that this should fix a regression I applied it to > >drm-intel-fixes. > >-Daniel > > > >>--- > >> drivers/gpu/drm/i915/intel_bios.c | 26 ++++++++++++++++++++++---- > >> 1 file changed, 22 insertions(+), 4 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > >>index 2ff9eb00fdec..8a1f3e1fc598 100644 > >>--- a/drivers/gpu/drm/i915/intel_bios.c > >>+++ b/drivers/gpu/drm/i915/intel_bios.c > >>@@ -1015,15 +1015,33 @@ 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 = 33; > >>+ } else if (bdb->version == 195) { > >>+ expected_size = 37; > >>+ } else if (bdb->version <= 197) { > >>+ expected_size = 38; > >>+ } else { > >>+ expected_size = 38; > >>+ DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n", expected_size); > > Hi, the line above throws a warning because there are two '%u' but only one > variable. Looks like bdb->version should be the 1st printed value. Good catch, your analysis is indeed correct. Daniel, will you do the fixup or should I submit a fixed version? Kind regards, David
On Thu, Aug 06, 2015 at 02:18:35PM +0200, Daniel Vetter wrote: > On Wed, Aug 05, 2015 at 06:32:01PM +0300, David Weinehall wrote: > > On Wed, Aug 05, 2015 at 10:59:00AM +0200, Daniel Vetter wrote: > > > On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote: > > > > 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. > > > > > > > > v2: Stricter size checks > > > > > > > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > > > > > Since Chris mentioned that this should fix a regression I applied it to > > > drm-intel-fixes. > > > > Great! Will you merge the other patch in the series to the nightly > > build? > > I only merged this fast-tracked because it fixes a regression. For the > other patch normal review rules still apply (i.e. I won't do it). I wasn't expecting fast tracking. I'd missed the fact that patch 2/2 wasn't reviewed by anyone yet; I've done so now. Kind regards, David
On Thu, Aug 06, 2015 at 05:08:55PM +0300, David Weinehall wrote: > On Thu, Aug 06, 2015 at 02:59:10PM +0100, Michel Thierry wrote: > > On 8/5/2015 9:59 AM, Daniel Vetter wrote: > > >On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote: > > >>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. > > >> > > >>v2: Stricter size checks > > >> > > >>Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > > > > >Since Chris mentioned that this should fix a regression I applied it to > > >drm-intel-fixes. > > >-Daniel > > > > > >>--- > > >> drivers/gpu/drm/i915/intel_bios.c | 26 ++++++++++++++++++++++---- > > >> 1 file changed, 22 insertions(+), 4 deletions(-) > > >> > > >>diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > > >>index 2ff9eb00fdec..8a1f3e1fc598 100644 > > >>--- a/drivers/gpu/drm/i915/intel_bios.c > > >>+++ b/drivers/gpu/drm/i915/intel_bios.c > > >>@@ -1015,15 +1015,33 @@ 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 = 33; > > >>+ } else if (bdb->version == 195) { > > >>+ expected_size = 37; > > >>+ } else if (bdb->version <= 197) { > > >>+ expected_size = 38; > > >>+ } else { > > >>+ expected_size = 38; > > >>+ DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n", expected_size); > > > > Hi, the line above throws a warning because there are two '%u' but only one > > variable. Looks like bdb->version should be the 1st printed value. > > Good catch, your analysis is indeed correct. > > Daniel, will you do the fixup or should I submit a fixed version? Fixed up locally. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 2ff9eb00fdec..8a1f3e1fc598 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1015,15 +1015,33 @@ 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 = 33; + } else if (bdb->version == 195) { + expected_size = 37; + } else if (bdb->version <= 197) { + expected_size = 38; + } else { + expected_size = 38; + DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n", expected_size); + } + + if (expected_size > sizeof(*p_child)) { + DRM_ERROR("child_device_config cannot fit in p_child\n"); + return; + } + + if (p_defs->child_dev_size != expected_size) { + DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n", + p_defs->child_dev_size, expected_size, bdb->version); return; } /* get the block size of general definitions */ @@ -1070,7 +1088,7 @@ 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)); + memcpy(child_dev_ptr, p_child, p_defs->child_dev_size); } return; }
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. v2: Stricter size checks Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> --- drivers/gpu/drm/i915/intel_bios.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)