| Message ID | 20180911165457.12651-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix a potential integer overflow with framebuffers extending past 4 GiB | expand |
Quoting Ville Syrjala (2018-09-11 17:54:57) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > If we have framebuffers that are >= 4GiB in size we will overflow > the fb size check in intel_fill_fb_info(). > > Currently that is only possible with NV12 and CCS as offsets[1] > may be anything between 0 and 0xffffffff. ofsets[0] is currently > required to be 0 so we can't hit the overflow with any single > plane format (thanks to max fb size of 8kx8k and max stride of > 32 KiB). > > In the future we may allow almost any framebuffer to exceed 4GiB > in size so we really should fix the overflow. Not that the overflow > is particularly dangerous. It's mostly just a sanity check against > insane userspace. The display engine can't write to memory anyway > so I suppose in the worst case we might anger the hw by attempting > scanout past the end of the ggtt, or we might scan out some data > that we're not supposed to see from other parts of the ggtt. > > Note that triggering this overflow depends on the driver > aligning the fb height to the next tile boundary to push the > calculated size above 4GiB. With linear buffers the effective > tile height is one so that never happens, and the core already > has a check for 32bit overflow of offsets[]+pitches[]*height. > > Testcase: igt/kms_big_fb/x-tiled-addfb-size-offset-overflow > Testcase: igt/kms_big_fb/y-tiled-addfb-size-offset-overflow > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2b77d9350a3a..2b474d049074 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2636,9 +2636,10 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv, > max_size = max(max_size, offset + size); > } > > - if (max_size * tile_size > obj->base.size) { > - DRM_DEBUG_KMS("fb too big for bo (need %u bytes, have %zu bytes)\n", > - max_size * tile_size, obj->base.size); > + if (mul_u32_u32(max_size, tile_size) > obj->base.size) { > + DRM_DEBUG_KMS("fb too big for bo (need %llu bytes, have %zu bytes)\n", > + (unsigned long long) mul_u32_u32(max_size, tile_size), mul_u32_u32 returns u64 i.e. unsigned long long; %llu is the one true format specifier for u64 (Linus decree #103789) Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Wed, Sep 12, 2018 at 09:13:07AM +0100, Chris Wilson wrote: > Quoting Ville Syrjala (2018-09-11 17:54:57) > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > If we have framebuffers that are >= 4GiB in size we will overflow > > the fb size check in intel_fill_fb_info(). > > > > Currently that is only possible with NV12 and CCS as offsets[1] > > may be anything between 0 and 0xffffffff. ofsets[0] is currently > > required to be 0 so we can't hit the overflow with any single > > plane format (thanks to max fb size of 8kx8k and max stride of > > 32 KiB). > > > > In the future we may allow almost any framebuffer to exceed 4GiB > > in size so we really should fix the overflow. Not that the overflow > > is particularly dangerous. It's mostly just a sanity check against > > insane userspace. The display engine can't write to memory anyway > > so I suppose in the worst case we might anger the hw by attempting > > scanout past the end of the ggtt, or we might scan out some data > > that we're not supposed to see from other parts of the ggtt. > > > > Note that triggering this overflow depends on the driver > > aligning the fb height to the next tile boundary to push the > > calculated size above 4GiB. With linear buffers the effective > > tile height is one so that never happens, and the core already > > has a check for 32bit overflow of offsets[]+pitches[]*height. > > > > Testcase: igt/kms_big_fb/x-tiled-addfb-size-offset-overflow > > Testcase: igt/kms_big_fb/y-tiled-addfb-size-offset-overflow > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 2b77d9350a3a..2b474d049074 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2636,9 +2636,10 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv, > > max_size = max(max_size, offset + size); > > } > > > > - if (max_size * tile_size > obj->base.size) { > > - DRM_DEBUG_KMS("fb too big for bo (need %u bytes, have %zu bytes)\n", > > - max_size * tile_size, obj->base.size); > > + if (mul_u32_u32(max_size, tile_size) > obj->base.size) { > > + DRM_DEBUG_KMS("fb too big for bo (need %llu bytes, have %zu bytes)\n", > > + (unsigned long long) mul_u32_u32(max_size, tile_size), > > mul_u32_u32 returns u64 i.e. unsigned long long; %llu is the one true > format specifier for u64 (Linus decree #103789) Well whaddyaknow, so it is. Never realized that for some reason. > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2b77d9350a3a..2b474d049074 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2636,9 +2636,10 @@ intel_fill_fb_info(struct drm_i915_private *dev_priv, max_size = max(max_size, offset + size); } - if (max_size * tile_size > obj->base.size) { - DRM_DEBUG_KMS("fb too big for bo (need %u bytes, have %zu bytes)\n", - max_size * tile_size, obj->base.size); + if (mul_u32_u32(max_size, tile_size) > obj->base.size) { + DRM_DEBUG_KMS("fb too big for bo (need %llu bytes, have %zu bytes)\n", + (unsigned long long) mul_u32_u32(max_size, tile_size), + obj->base.size); return -EINVAL; }