[2/2] drm/i915: Check that the DMA address for stolen fits within dma_addr_t

Submitted by Chris Wilson on Jan. 27, 2017, 4:55 p.m.

Details

Message ID 20170127165531.28135-2-chris@chris-wilson.co.uk
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GFX

Browsing this patch as part of:
"Series without cover letter" rev 1 in Intel GFX
<< prev patch [2/2] next patch >>

Commit Message

Chris Wilson Jan. 27, 2017, 4:55 p.m.
Just sanity check that the value we deduce from the stolen memory
register fits within the kernel's dma_addr_t and doesn't overflow.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 42bbc4b04fd6..4f1f3090c0ed 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -211,6 +211,14 @@  static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
 			ggtt_start &= PGTBL_ADDRESS_LO_MASK;
 		ggtt_end = ggtt_start + ggtt_total_entries(ggtt) * 4;
 
+		if (ggtt_end <= ggtt_start ||
+		    overflows_type(ggtt_end, dma_addr_t)) {
+			DRM_ERROR("DMA address for reserved igfx memory [%llx - %llx] does not fit within the kernel's %db dma_addr_t\n",
+				  ggtt_start, ggtt_end,
+				  (int)sizeof(dma_addr_t) * 8);
+			return 0;
+		}
+
 		if (ggtt_start >= stolen[0].start && ggtt_start < stolen[0].end)
 			stolen[0].end = ggtt_start;
 		if (ggtt_end > stolen[1].start && ggtt_end <= stolen[1].end)

Comments

On Fri, Jan 27, 2017 at 04:55:31PM +0000, Chris Wilson wrote:
> Just sanity check that the value we deduce from the stolen memory
> register fits within the kernel's dma_addr_t and doesn't overflow.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 42bbc4b04fd6..4f1f3090c0ed 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -211,6 +211,14 @@ static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
>  			ggtt_start &= PGTBL_ADDRESS_LO_MASK;
>  		ggtt_end = ggtt_start + ggtt_total_entries(ggtt) * 4;
>  
> +		if (ggtt_end <= ggtt_start ||
> +		    overflows_type(ggtt_end, dma_addr_t)) {
> +			DRM_ERROR("DMA address for reserved igfx memory [%llx - %llx] does not fit within the kernel's %db dma_addr_t\n",
> +				  ggtt_start, ggtt_end,
> +				  (int)sizeof(dma_addr_t) * 8);
> +			return 0;
> +		}

This would only check if the ggtt location fits into dma_addr_t.
We don't need that since we never touch the ggtt directly with
either the CPU or GPU. So I think this piece of code should keep
on using u64 as it needs to be able to hold the 36 address we read
from the hardware/firmware.

> +
>  		if (ggtt_start >= stolen[0].start && ggtt_start < stolen[0].end)
>  			stolen[0].end = ggtt_start;
>  		if (ggtt_end > stolen[1].start && ggtt_end <= stolen[1].end)
> -- 
> 2.11.0
On Fri, Jan 27, 2017 at 07:05:57PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 27, 2017 at 04:55:31PM +0000, Chris Wilson wrote:
> > Just sanity check that the value we deduce from the stolen memory
> > register fits within the kernel's dma_addr_t and doesn't overflow.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_stolen.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index 42bbc4b04fd6..4f1f3090c0ed 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -211,6 +211,14 @@ static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
> >  			ggtt_start &= PGTBL_ADDRESS_LO_MASK;
> >  		ggtt_end = ggtt_start + ggtt_total_entries(ggtt) * 4;
> >  
> > +		if (ggtt_end <= ggtt_start ||
> > +		    overflows_type(ggtt_end, dma_addr_t)) {
> > +			DRM_ERROR("DMA address for reserved igfx memory [%llx - %llx] does not fit within the kernel's %db dma_addr_t\n",
> > +				  ggtt_start, ggtt_end,
> > +				  (int)sizeof(dma_addr_t) * 8);
> > +			return 0;
> > +		}
> 
> This would only check if the ggtt location fits into dma_addr_t.
> We don't need that since we never touch the ggtt directly with
> either the CPU or GPU. So I think this piece of code should keep
> on using u64 as it needs to be able to hold the 36 address we read
> from the hardware/firmware.

Ok, I was just looking for u64 in the vicinity of dma_addr_t. So we are
just checking that the ggtt doesn't overlap stolen, not creating stolen
addresses from ggtt. Yup, we don't need to check against dma_addr_t
here - that I can leave for future register checking.

The sanity check for stolen would actually just be: