Re: [Intel-gfx] [PATCH 1/2] drm/i915: Allow parsing of variable size child device entries from VBT
On Tue, Jul 14, 2015 at 10:11:37AM +0300, David Weinehall wrote: > On Mon, Jul 13, 2015 at 11:24:46AM +0200, Daniel Vetter wrote: > > On Fri, Jul 10, 2015 at 02:10:54PM +0300, Antti Koskipaa 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. > > > > > > So now, instead of checking for smaller size, check that the VBT entry is > > > not too large and memcpy only child_dev_size amount of data, leaving any > > > trailing entries as zero. If this is not good enough for the future, > > > we can always sprinkle extra version checks in there. > > > > > > Signed-off-by: Antti Koskipaa > > > > As I mentioned in the other threads I think with vbt it's not too paranoid > > to double-check our assumptions. So for each vbt version range I'd like us > > to check what size we exactly expect. Being super paranoid with vbt is imo > > good practice since otherwise the hw teams will sneak in another update > > without us realizing it. > > Antti's on vacation now for the next few weeks. Do you need these > modifications as a pre-requisite for merging his patch, or can further > improvements be submitted separately? This patch starts to make our vbt checking lax. I don't want to walk down this road really, so yes I prefer if we just keep on having really strict checks. -Daniel -- 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
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Allow parsing of variable size child device entries from VBT
On Mon, Jul 13, 2015 at 11:24:46AM +0200, Daniel Vetter wrote: > On Fri, Jul 10, 2015 at 02:10:54PM +0300, Antti Koskipaa 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. > > > > So now, instead of checking for smaller size, check that the VBT entry is > > not too large and memcpy only child_dev_size amount of data, leaving any > > trailing entries as zero. If this is not good enough for the future, > > we can always sprinkle extra version checks in there. > > > > Signed-off-by: Antti Koskipaa > > As I mentioned in the other threads I think with vbt it's not too paranoid > to double-check our assumptions. So for each vbt version range I'd like us > to check what size we exactly expect. Being super paranoid with vbt is imo > good practice since otherwise the hw teams will sneak in another update > without us realizing it. Antti's on vacation now for the next few weeks. Do you need these modifications as a pre-requisite for merging his patch, or can further improvements be submitted separately? Kind regards, David ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Allow parsing of variable size child device entries from VBT
On Fri, Jul 10, 2015 at 02:10:54PM +0300, Antti Koskipaa 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.
>
> So now, instead of checking for smaller size, check that the VBT entry is
> not too large and memcpy only child_dev_size amount of data, leaving any
> trailing entries as zero. If this is not good enough for the future,
> we can always sprinkle extra version checks in there.
>
> Signed-off-by: Antti Koskipaa
As I mentioned in the other threads I think with vbt it's not too paranoid
to double-check our assumptions. So for each vbt version range I'd like us
to check what size we exactly expect. Being super paranoid with vbt is imo
good practice since otherwise the hw teams will sneak in another update
without us realizing it.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_bios.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c
> b/drivers/gpu/drm/i915/intel_bios.c
> index 2ff9eb0..763a636 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1022,10 +1022,15 @@ parse_device_mapping(struct drm_i915_private
> *dev_priv,
> DRM_DEBUG_KMS("No general definition block is found, no devices
> defined.\n");
> return;
> }
> - if (p_defs->child_dev_size < sizeof(*p_child)) {
> + /* Historically, child_dev_size has to be at least 33 bytes in size. */
> + if (p_defs->child_dev_size < 33) {
> DRM_ERROR("General definiton block child device size is too
> small.\n");
> return;
> }
> + if (p_defs->child_dev_size > sizeof(*p_child)) {
> + DRM_ERROR("General definiton block child device size is too
> large.\n");
> + return;
> + }
> /* get the block size of general definitions */
> block_size = get_blocksize(p_defs);
> /* get the number of child device */
> @@ -1070,7 +1075,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;
> }
> --
> 2.3.6
>
> ___
> 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
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Allow parsing of variable size child device entries from VBT
On Fri, Jul 10, 2015 at 02:10:54PM +0300, Antti Koskipaa 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.
>
> So now, instead of checking for smaller size, check that the VBT entry is
> not too large and memcpy only child_dev_size amount of data, leaving any
> trailing entries as zero. If this is not good enough for the future,
> we can always sprinkle extra version checks in there.
>
> Signed-off-by: Antti Koskipaa
> ---
> drivers/gpu/drm/i915/intel_bios.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c
> b/drivers/gpu/drm/i915/intel_bios.c
> index 2ff9eb0..763a636 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1022,10 +1022,15 @@ parse_device_mapping(struct drm_i915_private
> *dev_priv,
> DRM_DEBUG_KMS("No general definition block is found, no devices
> defined.\n");
> return;
> }
> - if (p_defs->child_dev_size < sizeof(*p_child)) {
> + /* Historically, child_dev_size has to be at least 33 bytes in size. */
> + if (p_defs->child_dev_size < 33) {
> DRM_ERROR("General definiton block child device size is too
> small.\n");
"definition"
> return;
> }
> + if (p_defs->child_dev_size > sizeof(*p_child)) {
> + DRM_ERROR("General definiton block child device size is too
> large.\n");
"definition"
> + return;
> + }
> /* get the block size of general definitions */
> block_size = get_blocksize(p_defs);
> /* get the number of child device */
> @@ -1070,7 +1075,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;
> }
> --
> 2.3.6
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
