drm/i915/gem: Detect overflow in calculating dumb buffer size
diff mbox series

Message ID 20200123125934.1401755-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915/gem: Detect overflow in calculating dumb buffer size
Related show

Commit Message

Chris Wilson Jan. 23, 2020, 12:59 p.m. UTC
To multiply 2 u32 numbers to generate a u64 in C requires a bit of
forewarning for the compiler.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Ville Syrjälä Jan. 23, 2020, 1:27 p.m. UTC | #1
On Thu, Jan 23, 2020 at 12:59:34PM +0000, Chris Wilson wrote:
> To multiply 2 u32 numbers to generate a u64 in C requires a bit of
> forewarning for the compiler.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0a20083321a3..ff79da5657f8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -265,7 +265,10 @@ i915_gem_dumb_create(struct drm_file *file,
>  						    DRM_FORMAT_MOD_LINEAR))
>  		args->pitch = ALIGN(args->pitch, 4096);
>  
> -	args->size = args->pitch * args->height;
> +	if (args->pitch < args->width)
> +		return -EINVAL;
> +
> +	args->size = mul_u32_u32(args->pitch, args->height);

I thought something would have checked these against the mode_config
fb limits already. But can't see code like that anywhere. Maybe we
should just do that in the core?

>  
>  	mem_type = INTEL_MEMORY_SYSTEM;
>  	if (HAS_LMEM(to_i915(dev)))
> -- 
> 2.25.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Jan. 23, 2020, 1:39 p.m. UTC | #2
Quoting Ville Syrjälä (2020-01-23 13:27:07)
> On Thu, Jan 23, 2020 at 12:59:34PM +0000, Chris Wilson wrote:
> > To multiply 2 u32 numbers to generate a u64 in C requires a bit of
> > forewarning for the compiler.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 0a20083321a3..ff79da5657f8 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -265,7 +265,10 @@ i915_gem_dumb_create(struct drm_file *file,
> >                                                   DRM_FORMAT_MOD_LINEAR))
> >               args->pitch = ALIGN(args->pitch, 4096);
> >  
> > -     args->size = args->pitch * args->height;
> > +     if (args->pitch < args->width)
> > +             return -EINVAL;
> > +
> > +     args->size = mul_u32_u32(args->pitch, args->height);
> 
> I thought something would have checked these against the mode_config
> fb limits already. But can't see code like that anywhere. Maybe we
> should just do that in the core?

While it is in uapi/drm_mode.h, is there any restriction that the dumb
buffer has to be used with a framebuffer? Not that I have a good use
case, just wondering if we need to be so proscriptive.

We create something that is compatible but presume we will need later
validation against HW.
-Chris
Ville Syrjälä Jan. 23, 2020, 1:58 p.m. UTC | #3
On Thu, Jan 23, 2020 at 01:39:03PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjälä (2020-01-23 13:27:07)
> > On Thu, Jan 23, 2020 at 12:59:34PM +0000, Chris Wilson wrote:
> > > To multiply 2 u32 numbers to generate a u64 in C requires a bit of
> > > forewarning for the compiler.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Ramalingam C <ramalingam.c@intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 0a20083321a3..ff79da5657f8 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -265,7 +265,10 @@ i915_gem_dumb_create(struct drm_file *file,
> > >                                                   DRM_FORMAT_MOD_LINEAR))
> > >               args->pitch = ALIGN(args->pitch, 4096);
> > >  
> > > -     args->size = args->pitch * args->height;
> > > +     if (args->pitch < args->width)
> > > +             return -EINVAL;
> > > +
> > > +     args->size = mul_u32_u32(args->pitch, args->height);
> > 
> > I thought something would have checked these against the mode_config
> > fb limits already. But can't see code like that anywhere. Maybe we
> > should just do that in the core?
> 
> While it is in uapi/drm_mode.h, is there any restriction that the dumb
> buffer has to be used with a framebuffer? Not that I have a good use
> case, just wondering if we need to be so proscriptive.

I think the general concensus has been that anything else is an abuse
of the interface (not that it has stopped people from doing it IIRC).

But maybe there's some good use for it that I can't think up.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> We create something that is compatible but presume we will need later
> validation against HW.
> -Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0a20083321a3..ff79da5657f8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -265,7 +265,10 @@  i915_gem_dumb_create(struct drm_file *file,
 						    DRM_FORMAT_MOD_LINEAR))
 		args->pitch = ALIGN(args->pitch, 4096);
 
-	args->size = args->pitch * args->height;
+	if (args->pitch < args->width)
+		return -EINVAL;
+
+	args->size = mul_u32_u32(args->pitch, args->height);
 
 	mem_type = INTEL_MEMORY_SYSTEM;
 	if (HAS_LMEM(to_i915(dev)))