From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 6/7] drm/crtc: add sanity checks to create_dumb() Date: Tue, 21 Jan 2014 13:42:16 +0200 Message-ID: <20140121114216.GB9454@intel.com> References: <1390245989-13280-1-git-send-email-dh.herrmann@gmail.com> <1390245989-13280-6-git-send-email-dh.herrmann@gmail.com> <20140121094943.GP15089@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id D210FFA817 for ; Tue, 21 Jan 2014 03:42:19 -0800 (PST) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: David Herrmann Cc: Daniel Vetter , "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org On Tue, Jan 21, 2014 at 12:17:35PM +0100, David Herrmann wrote: > Hi > = > On Tue, Jan 21, 2014 at 10:49 AM, Daniel Vetter wrote: > > On Mon, Jan 20, 2014 at 08:26:28PM +0100, David Herrmann wrote: > >> Lets make sure some basic expressions are always true: > >> bpp !=3D NULL > >> width !=3D NULL > >> height !=3D NULL > >> stride =3D bpp * width < 2^32 > >> size =3D stride * height < 2^32 > >> PAGE_ALIGN(size) < 2^32 > >> > >> At least the udl driver doesn't check for multiplication-overflows, so > >> lets just make sure it will never happen. These checks allow drivers t= o do > >> any 32bit math without having to test for mult-overflows themselves. > >> > >> The two divisions might hurt performance a bit, but dumb_create() is o= nly > >> used for scanout-buffers, so that should be fine. We could use 64bit m= ath > >> to avoid the divisions, but that may be slow on 32bit machines.. Or ma= ybe > >> there should just be a "safe_mult32()" helper, which currently doesn't > >> exist (I think?). > >> > >> Signed-off-by: David Herrmann > >> --- > >> drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++ > >> 1 file changed, 15 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > >> index 266a01d..ff647fa 100644 > >> --- a/drivers/gpu/drm/drm_crtc.c > >> +++ b/drivers/gpu/drm/drm_crtc.c > >> @@ -3738,9 +3738,24 @@ int drm_mode_create_dumb_ioctl(struct drm_devic= e *dev, > >> void *data, struct drm_file *file_priv) > >> { > >> struct drm_mode_create_dumb *args =3D data; > >> + u32 Bpp, stride, size; > > > > s/Bpp/bpp/ > = > It's actually "Bytes per pixel", not "Bits per pixel". We generally > use "bpp" as "bits per pixel" so I'd like to avoid the confusion. But > yeah, upper-case names are unusual so I can also use bytes_pp or sth > similar? cpp is fairly commonly used for bytes per pixel, if you want to stick to something short but still recognizable. > = > >> > >> if (!dev->driver->dumb_create) > >> return -ENOSYS; > >> + if (!args->width || !args->height || !args->bpp) > >> + return -EINVAL; > >> + > >> + /* overflow checks for 32bit size calculations */ > >> + Bpp =3D (args->bpp + 7) / 8; > > > > Again DIV_ROUND_UP > = > yepp, fixed. > = > > > >> + if (Bpp > 0xffffffffU / args->width) > >> + return -EINVAL; > >> + stride =3D Bpp * args->width; > >> + if (args->height > 0xffffffffU / stride) > >> + return -EINVAL; > >> + size =3D args->height * stride; > >> + if (PAGE_ALIGN(size) < size) > > > > Maybe check for 0 here and add a small comment that this checks > > wrap-around. > = > Hm, the comment above those if()s already says "overflow checks", and > it should be obvious that all three are overflow checks, so I don't > think we need another comment (especially because I didn't add any > empty lines between them). > = > I will change it to "if (!PAGE_ALIGN(size))". The "x + off < x" is an > addition-overflow-check so I think it doesn't apply here. > = > Thanks > David > = > > > >> + return -EINVAL; > >> + > >> return dev->driver->dumb_create(file_priv, dev, args); > >> } > >> > >> -- > >> 1.8.5.3 > >> > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- = Ville Syrj=E4l=E4 Intel OTC