| Message ID | 20190208184254.24123-2-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] drm/i915/opregion: fix version check | expand |
On Fri, Feb 08, 2019 at 08:42:53PM +0200, Jani Nikula wrote: > Starting from opregion version 2.1 (roughly corresponding to ICL+) the > RVDA field is relative from the beginning of opregion, not absolute > address. > > Fix the error path while at it. > > v2: Make relative vs. absolute conditional on the opregion version, > bumped for the purpose. Turned out there are machines relying on > absolute RVDA in the wild. > > v3: Fix the version checks > > Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB") > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_opregion.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c > index f1b841580521..5e00ee9270b5 100644 > --- a/drivers/gpu/drm/i915/intel_opregion.c > +++ b/drivers/gpu/drm/i915/intel_opregion.c > @@ -123,7 +123,8 @@ struct opregion_asle { > u64 fdss; > u32 fdsp; > u32 stat; > - u64 rvda; /* Physical address of raw vbt data */ > + u64 rvda; /* Physical (2.0) or relative from opregion (2.1+) > + * address of raw VBT data. */ > u32 rvds; /* Size of raw vbt data */ > u8 rsvd[58]; > } __packed; > @@ -964,9 +965,24 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) > > if (opregion->header->over.major >= 2 && opregion->asle && > opregion->asle->rvda && opregion->asle->rvds) { > - opregion->rvda = memremap(opregion->asle->rvda, > - opregion->asle->rvds, > + resource_size_t rvda = opregion->asle->rvda; > + > + /* > + * opregion 2.0: rvda is the physical VBT address. > + * > + * opregion 2.1+: rvda is unsigned, relative offset from > + * opregion base, and should never point within opregion. > + */ > + if (opregion->header->over.major > 2 || > + opregion->header->over.minor >= 1) { What happens with version 1.1? > + WARN_ON(rvda < OPREGION_SIZE); > + > + rvda += asls; > + } > + > + opregion->rvda = memremap(rvda, opregion->asle->rvds, > MEMREMAP_WB); > + > vbt = opregion->rvda; > vbt_size = opregion->asle->rvds; > if (intel_bios_is_valid_vbt(vbt, vbt_size)) { > @@ -976,6 +992,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) > goto out; > } else { > DRM_DEBUG_KMS("Invalid VBT in ACPI OpRegion (RVDA)\n"); > + memunmap(opregion->rvda); > + opregion->rvda = NULL; > } > } > > -- > 2.20.1
On Fri, 08 Feb 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Feb 08, 2019 at 08:42:53PM +0200, Jani Nikula wrote: >> Starting from opregion version 2.1 (roughly corresponding to ICL+) the >> RVDA field is relative from the beginning of opregion, not absolute >> address. >> >> Fix the error path while at it. >> >> v2: Make relative vs. absolute conditional on the opregion version, >> bumped for the purpose. Turned out there are machines relying on >> absolute RVDA in the wild. >> >> v3: Fix the version checks >> >> Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB") >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Imre Deak <imre.deak@intel.com> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/intel_opregion.c | 24 +++++++++++++++++++++--- >> 1 file changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c >> index f1b841580521..5e00ee9270b5 100644 >> --- a/drivers/gpu/drm/i915/intel_opregion.c >> +++ b/drivers/gpu/drm/i915/intel_opregion.c >> @@ -123,7 +123,8 @@ struct opregion_asle { >> u64 fdss; >> u32 fdsp; >> u32 stat; >> - u64 rvda; /* Physical address of raw vbt data */ >> + u64 rvda; /* Physical (2.0) or relative from opregion (2.1+) >> + * address of raw VBT data. */ >> u32 rvds; /* Size of raw vbt data */ >> u8 rsvd[58]; >> } __packed; >> @@ -964,9 +965,24 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) >> >> if (opregion->header->over.major >= 2 && opregion->asle && Push this line to short term memory. >> opregion->asle->rvda && opregion->asle->rvds) { >> - opregion->rvda = memremap(opregion->asle->rvda, >> - opregion->asle->rvds, >> + resource_size_t rvda = opregion->asle->rvda; >> + >> + /* >> + * opregion 2.0: rvda is the physical VBT address. >> + * >> + * opregion 2.1+: rvda is unsigned, relative offset from >> + * opregion base, and should never point within opregion. >> + */ >> + if (opregion->header->over.major > 2 || >> + opregion->header->over.minor >= 1) { > > What happens with version 1.1? Pop. ;) BR, Jani. > >> + WARN_ON(rvda < OPREGION_SIZE); >> + >> + rvda += asls; >> + } >> + >> + opregion->rvda = memremap(rvda, opregion->asle->rvds, >> MEMREMAP_WB); >> + >> vbt = opregion->rvda; >> vbt_size = opregion->asle->rvds; >> if (intel_bios_is_valid_vbt(vbt, vbt_size)) { >> @@ -976,6 +992,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) >> goto out; >> } else { >> DRM_DEBUG_KMS("Invalid VBT in ACPI OpRegion (RVDA)\n"); >> + memunmap(opregion->rvda); >> + opregion->rvda = NULL; >> } >> } >> >> -- >> 2.20.1
On Fri, Feb 08, 2019 at 09:00:59PM +0200, Jani Nikula wrote: > On Fri, 08 Feb 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Fri, Feb 08, 2019 at 08:42:53PM +0200, Jani Nikula wrote: > >> Starting from opregion version 2.1 (roughly corresponding to ICL+) the > >> RVDA field is relative from the beginning of opregion, not absolute > >> address. > >> > >> Fix the error path while at it. > >> > >> v2: Make relative vs. absolute conditional on the opregion version, > >> bumped for the purpose. Turned out there are machines relying on > >> absolute RVDA in the wild. > >> > >> v3: Fix the version checks > >> > >> Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB") > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Cc: Imre Deak <imre.deak@intel.com> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_opregion.c | 24 +++++++++++++++++++++--- > >> 1 file changed, 21 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c > >> index f1b841580521..5e00ee9270b5 100644 > >> --- a/drivers/gpu/drm/i915/intel_opregion.c > >> +++ b/drivers/gpu/drm/i915/intel_opregion.c > >> @@ -123,7 +123,8 @@ struct opregion_asle { > >> u64 fdss; > >> u32 fdsp; > >> u32 stat; > >> - u64 rvda; /* Physical address of raw vbt data */ > >> + u64 rvda; /* Physical (2.0) or relative from opregion (2.1+) > >> + * address of raw VBT data. */ > >> u32 rvds; /* Size of raw vbt data */ > >> u8 rsvd[58]; > >> } __packed; > >> @@ -964,9 +965,24 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) > >> > >> if (opregion->header->over.major >= 2 && opregion->asle && > > Push this line to short term memory. Stack overflow. Series is Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >> opregion->asle->rvda && opregion->asle->rvds) { > >> - opregion->rvda = memremap(opregion->asle->rvda, > >> - opregion->asle->rvds, > >> + resource_size_t rvda = opregion->asle->rvda; > >> + > >> + /* > >> + * opregion 2.0: rvda is the physical VBT address. > >> + * > >> + * opregion 2.1+: rvda is unsigned, relative offset from > >> + * opregion base, and should never point within opregion. > >> + */ > >> + if (opregion->header->over.major > 2 || > >> + opregion->header->over.minor >= 1) { > > > > What happens with version 1.1? > > Pop. ;) > > BR, > Jani. > > > > >> + WARN_ON(rvda < OPREGION_SIZE); > >> + > >> + rvda += asls; > >> + } > >> + > >> + opregion->rvda = memremap(rvda, opregion->asle->rvds, > >> MEMREMAP_WB); > >> + > >> vbt = opregion->rvda; > >> vbt_size = opregion->asle->rvds; > >> if (intel_bios_is_valid_vbt(vbt, vbt_size)) { > >> @@ -976,6 +992,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) > >> goto out; > >> } else { > >> DRM_DEBUG_KMS("Invalid VBT in ACPI OpRegion (RVDA)\n"); > >> + memunmap(opregion->rvda); > >> + opregion->rvda = NULL; > >> } > >> } > >> > >> -- > >> 2.20.1 > > -- > Jani Nikula, Intel Open Source Graphics Center
On Fri, 08 Feb 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Feb 08, 2019 at 09:00:59PM +0200, Jani Nikula wrote: >> On Fri, 08 Feb 2019, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > On Fri, Feb 08, 2019 at 08:42:53PM +0200, Jani Nikula wrote: >> >> Starting from opregion version 2.1 (roughly corresponding to ICL+) the >> >> RVDA field is relative from the beginning of opregion, not absolute >> >> address. >> >> >> >> Fix the error path while at it. >> >> >> >> v2: Make relative vs. absolute conditional on the opregion version, >> >> bumped for the purpose. Turned out there are machines relying on >> >> absolute RVDA in the wild. >> >> >> >> v3: Fix the version checks >> >> >> >> Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB") >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> Cc: Imre Deak <imre.deak@intel.com> >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> --- >> >> drivers/gpu/drm/i915/intel_opregion.c | 24 +++++++++++++++++++++--- >> >> 1 file changed, 21 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c >> >> index f1b841580521..5e00ee9270b5 100644 >> >> --- a/drivers/gpu/drm/i915/intel_opregion.c >> >> +++ b/drivers/gpu/drm/i915/intel_opregion.c >> >> @@ -123,7 +123,8 @@ struct opregion_asle { >> >> u64 fdss; >> >> u32 fdsp; >> >> u32 stat; >> >> - u64 rvda; /* Physical address of raw vbt data */ >> >> + u64 rvda; /* Physical (2.0) or relative from opregion (2.1+) >> >> + * address of raw VBT data. */ >> >> u32 rvds; /* Size of raw vbt data */ >> >> u8 rsvd[58]; >> >> } __packed; >> >> @@ -964,9 +965,24 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) >> >> >> >> if (opregion->header->over.major >= 2 && opregion->asle && >> >> Push this line to short term memory. > > Stack overflow. > > Series is > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks, pushed patches 1&2 to dinq. BR, Jani. > >> >> >> opregion->asle->rvda && opregion->asle->rvds) { >> >> - opregion->rvda = memremap(opregion->asle->rvda, >> >> - opregion->asle->rvds, >> >> + resource_size_t rvda = opregion->asle->rvda; >> >> + >> >> + /* >> >> + * opregion 2.0: rvda is the physical VBT address. >> >> + * >> >> + * opregion 2.1+: rvda is unsigned, relative offset from >> >> + * opregion base, and should never point within opregion. >> >> + */ >> >> + if (opregion->header->over.major > 2 || >> >> + opregion->header->over.minor >= 1) { >> > >> > What happens with version 1.1? >> >> Pop. ;) >> >> BR, >> Jani. >> >> > >> >> + WARN_ON(rvda < OPREGION_SIZE); >> >> + >> >> + rvda += asls; >> >> + } >> >> + >> >> + opregion->rvda = memremap(rvda, opregion->asle->rvds, >> >> MEMREMAP_WB); >> >> + >> >> vbt = opregion->rvda; >> >> vbt_size = opregion->asle->rvds; >> >> if (intel_bios_is_valid_vbt(vbt, vbt_size)) { >> >> @@ -976,6 +992,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) >> >> goto out; >> >> } else { >> >> DRM_DEBUG_KMS("Invalid VBT in ACPI OpRegion (RVDA)\n"); >> >> + memunmap(opregion->rvda); >> >> + opregion->rvda = NULL; >> >> } >> >> } >> >> >> >> -- >> >> 2.20.1 >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c index f1b841580521..5e00ee9270b5 100644 --- a/drivers/gpu/drm/i915/intel_opregion.c +++ b/drivers/gpu/drm/i915/intel_opregion.c @@ -123,7 +123,8 @@ struct opregion_asle { u64 fdss; u32 fdsp; u32 stat; - u64 rvda; /* Physical address of raw vbt data */ + u64 rvda; /* Physical (2.0) or relative from opregion (2.1+) + * address of raw VBT data. */ u32 rvds; /* Size of raw vbt data */ u8 rsvd[58]; } __packed; @@ -964,9 +965,24 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) if (opregion->header->over.major >= 2 && opregion->asle && opregion->asle->rvda && opregion->asle->rvds) { - opregion->rvda = memremap(opregion->asle->rvda, - opregion->asle->rvds, + resource_size_t rvda = opregion->asle->rvda; + + /* + * opregion 2.0: rvda is the physical VBT address. + * + * opregion 2.1+: rvda is unsigned, relative offset from + * opregion base, and should never point within opregion. + */ + if (opregion->header->over.major > 2 || + opregion->header->over.minor >= 1) { + WARN_ON(rvda < OPREGION_SIZE); + + rvda += asls; + } + + opregion->rvda = memremap(rvda, opregion->asle->rvds, MEMREMAP_WB); + vbt = opregion->rvda; vbt_size = opregion->asle->rvds; if (intel_bios_is_valid_vbt(vbt, vbt_size)) { @@ -976,6 +992,8 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv) goto out; } else { DRM_DEBUG_KMS("Invalid VBT in ACPI OpRegion (RVDA)\n"); + memunmap(opregion->rvda); + opregion->rvda = NULL; } }
Starting from opregion version 2.1 (roughly corresponding to ICL+) the RVDA field is relative from the beginning of opregion, not absolute address. Fix the error path while at it. v2: Make relative vs. absolute conditional on the opregion version, bumped for the purpose. Turned out there are machines relying on absolute RVDA in the wild. v3: Fix the version checks Fixes: 04ebaadb9f2d ("drm/i915/opregion: handle VBT sizes bigger than 6 KB") Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Imre Deak <imre.deak@intel.com> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_opregion.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)