Re: [PATCH 5/5] drm/i915: Use unsigned for overflow checks in execbuf

Chris Wilson
2013-09-19T10:44:59Z

On Thu, Sep 19, 2013 at 12:18:36PM +0200, Daniel Vetter wrote:
> There's actually no real risk since we already check for stricter
> constraints earlier (using UINT_MAX / sizeof (struct
> drm_i915_gem_exec_object2) as the limit). But in eb_create we use
> signed integers, which steals a factor of 2. Luckily struct
> drm_i915_gem_exec_object2 for this to not matter.
>
> Still, be consistent and use unsigned integers.
>
> Similar use unsinged integers when checking for overflows in the
> relocation entry processing.
>
> I've also added a new subtests to igt/gem_reloc_overflow to also
> test for overflowing args->buffer_count values.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index a733118..cd98aeb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -45,18 +45,19 @@ struct eb_vmas {
> static struct eb_vmas *
> eb_create(struct drm_i915_gem_execbuffer2 *args, struct i915_address_space *vm)
> {
> + unsigned size, count;
> struct eb_vmas *eb = NULL;
>
> if (args->flags & I915_EXEC_HANDLE_LUT) {
> - int size = args->buffer_count;

These were locally scoped so that the reader could be sure that we
didn't leak information from one branch to the other. The variables
exist only to break the computation up into manageable chunks, not
because they are useful outside of those branches.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre