| Message ID | 20200123125934.1401755-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: Detect overflow in calculating dumb buffer size | expand |
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
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
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
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)))
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(-)