Re: [PATCH v2] drm/dumb-buffers: Integer overflow in drm_mode_create_ioctl()

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: David Airlie <airlied@linux.ie>,
	kernel-janitors@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/dumb-buffers: Integer overflow in drm_mode_create_ioctl()
Date: Wed, 16 May 2018 15:56:55 +0100	[thread overview]
Message-ID: <152648261507.11239.9847993632251714966@mail.alporthouse.com> (raw)
In-Reply-To: <20180516145257.kct63hzesuey6jbc@mwanda>

Quoting Dan Carpenter (2018-05-16 15:52:57)
> On Wed, May 16, 2018 at 03:26:07PM +0100, Chris Wilson wrote:
> > Quoting Dan Carpenter (2018-05-16 15:00:26)
> > > There is a comment here which says that DIV_ROUND_UP() and that's where
> > > the problem comes from.  Say you pick:
> > > 
> > >         args->bpp = UINT_MAX - 7;
> > >         args->width = 4;
> > >         args->height = 1;
> > > 
> > > The integer overflow in DIV_ROUND_UP() means "cpp" is UINT_MAX / 8 and
> > > because of how we picked args->width that means cpp < UINT_MAX / 4.
> > > 
> > > I've fixed it by preventing the integer overflow in DIV_ROUND_UP().  I
> > > removed the check for !cpp because it's not possible after this change.
> > > I also changed all the 0xffffffffU references to U32_MAX.
> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > I agree with Daniel that the !cpp check after DIV_ROUND_UP was
> > sufficient to guard the current code, but switching to a more idiomatic
> > style of overflow checking has its benefits too. Plus I like having
> > U32_MAX to compare the type ranges against.
> > 
> 
> I'm not totally sure what it means to say that the integer overflow is
> sufficient.  The overflow check is definitely buggy but if you mean that
> it isn't exploitable then you're probably right.  Anyway, let's say you
> use use the values I provided in my changelog.  Then I believe we can
> reach vgem_gem_dumb_create():

But we are talking about

	cpp = (U32_MAX + 8) / 8

which is 0. So the !cpp does catch the overflow.

Or am I completely off in unsigned wraparound?
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel