| Message ID | 1390245989-13280-6-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 20, 2014 at 08:26:28PM +0100, David Herrmann wrote: > Lets make sure some basic expressions are always true: > bpp != NULL > width != NULL > height != NULL > stride = bpp * width < 2^32 > size = 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 to do > any 32bit math without having to test for mult-overflows themselves. > > The two divisions might hurt performance a bit, but dumb_create() is only > used for scanout-buffers, so that should be fine. We could use 64bit math > to avoid the divisions, but that may be slow on 32bit machines.. Or maybe > there should just be a "safe_mult32()" helper, which currently doesn't > exist (I think?). > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > --- > 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_device *dev, > void *data, struct drm_file *file_priv) > { > struct drm_mode_create_dumb *args = data; > + u32 Bpp, stride, size; s/Bpp/bpp/ > > if (!dev->driver->dumb_create) > return -ENOSYS; > + if (!args->width || !args->height || !args->bpp) > + return -EINVAL; > + > + /* overflow checks for 32bit size calculations */ > + Bpp = (args->bpp + 7) / 8; Again DIV_ROUND_UP > + if (Bpp > 0xffffffffU / args->width) > + return -EINVAL; > + stride = Bpp * args->width; > + if (args->height > 0xffffffffU / stride) > + return -EINVAL; > + size = args->height * stride; > + if (PAGE_ALIGN(size) < size) Maybe check for 0 here and add a small comment that this checks wrap-around. -Daniel > + return -EINVAL; > + > return dev->driver->dumb_create(file_priv, dev, args); > } > > -- > 1.8.5.3 >
Hi On Tue, Jan 21, 2014 at 10:49 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Jan 20, 2014 at 08:26:28PM +0100, David Herrmann wrote: >> Lets make sure some basic expressions are always true: >> bpp != NULL >> width != NULL >> height != NULL >> stride = bpp * width < 2^32 >> size = 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 to do >> any 32bit math without having to test for mult-overflows themselves. >> >> The two divisions might hurt performance a bit, but dumb_create() is only >> used for scanout-buffers, so that should be fine. We could use 64bit math >> to avoid the divisions, but that may be slow on 32bit machines.. Or maybe >> there should just be a "safe_mult32()" helper, which currently doesn't >> exist (I think?). >> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >> --- >> 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_device *dev, >> void *data, struct drm_file *file_priv) >> { >> struct drm_mode_create_dumb *args = 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? >> >> if (!dev->driver->dumb_create) >> return -ENOSYS; >> + if (!args->width || !args->height || !args->bpp) >> + return -EINVAL; >> + >> + /* overflow checks for 32bit size calculations */ >> + Bpp = (args->bpp + 7) / 8; > > Again DIV_ROUND_UP yepp, fixed. > >> + if (Bpp > 0xffffffffU / args->width) >> + return -EINVAL; >> + stride = Bpp * args->width; >> + if (args->height > 0xffffffffU / stride) >> + return -EINVAL; >> + size = 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
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 <daniel@ffwll.ch> wrote: > > On Mon, Jan 20, 2014 at 08:26:28PM +0100, David Herrmann wrote: > >> Lets make sure some basic expressions are always true: > >> bpp != NULL > >> width != NULL > >> height != NULL > >> stride = bpp * width < 2^32 > >> size = 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 to do > >> any 32bit math without having to test for mult-overflows themselves. > >> > >> The two divisions might hurt performance a bit, but dumb_create() is only > >> used for scanout-buffers, so that should be fine. We could use 64bit math > >> to avoid the divisions, but that may be slow on 32bit machines.. Or maybe > >> there should just be a "safe_mult32()" helper, which currently doesn't > >> exist (I think?). > >> > >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > >> --- > >> 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_device *dev, > >> void *data, struct drm_file *file_priv) > >> { > >> struct drm_mode_create_dumb *args = 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 = (args->bpp + 7) / 8; > > > > Again DIV_ROUND_UP > > yepp, fixed. > > > > >> + if (Bpp > 0xffffffffU / args->width) > >> + return -EINVAL; > >> + stride = Bpp * args->width; > >> + if (args->height > 0xffffffffU / stride) > >> + return -EINVAL; > >> + size = 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
Hi On Tue, Jan 21, 2014 at 12:42 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > 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 <daniel@ffwll.ch> wrote: >> > On Mon, Jan 20, 2014 at 08:26:28PM +0100, David Herrmann wrote: >> >> Lets make sure some basic expressions are always true: >> >> bpp != NULL >> >> width != NULL >> >> height != NULL >> >> stride = bpp * width < 2^32 >> >> size = 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 to do >> >> any 32bit math without having to test for mult-overflows themselves. >> >> >> >> The two divisions might hurt performance a bit, but dumb_create() is only >> >> used for scanout-buffers, so that should be fine. We could use 64bit math >> >> to avoid the divisions, but that may be slow on 32bit machines.. Or maybe >> >> there should just be a "safe_mult32()" helper, which currently doesn't >> >> exist (I think?). >> >> >> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >> >> --- >> >> 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_device *dev, >> >> void *data, struct drm_file *file_priv) >> >> { >> >> struct drm_mode_create_dumb *args = 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. Because "c" comes after "b"? Or is there any other logic here? But sounds good, will send a v2 shortly. Thanks David
On Tue, Jan 21, 2014 at 12:52:36PM +0100, David Herrmann wrote: > Hi > > On Tue, Jan 21, 2014 at 12:42 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > 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 <daniel@ffwll.ch> wrote: > >> > On Mon, Jan 20, 2014 at 08:26:28PM +0100, David Herrmann wrote: > >> >> Lets make sure some basic expressions are always true: > >> >> bpp != NULL > >> >> width != NULL > >> >> height != NULL > >> >> stride = bpp * width < 2^32 > >> >> size = 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 to do > >> >> any 32bit math without having to test for mult-overflows themselves. > >> >> > >> >> The two divisions might hurt performance a bit, but dumb_create() is only > >> >> used for scanout-buffers, so that should be fine. We could use 64bit math > >> >> to avoid the divisions, but that may be slow on 32bit machines.. Or maybe > >> >> there should just be a "safe_mult32()" helper, which currently doesn't > >> >> exist (I think?). > >> >> > >> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > >> >> --- > >> >> 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_device *dev, > >> >> void *data, struct drm_file *file_priv) > >> >> { > >> >> struct drm_mode_create_dumb *args = 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. > > Because "c" comes after "b"? Or is there any other logic here? But > sounds good, will send a v2 shortly. chars (octets) per pixel. -Chris
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_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_create_dumb *args = data; + u32 Bpp, stride, size; if (!dev->driver->dumb_create) return -ENOSYS; + if (!args->width || !args->height || !args->bpp) + return -EINVAL; + + /* overflow checks for 32bit size calculations */ + Bpp = (args->bpp + 7) / 8; + if (Bpp > 0xffffffffU / args->width) + return -EINVAL; + stride = Bpp * args->width; + if (args->height > 0xffffffffU / stride) + return -EINVAL; + size = args->height * stride; + if (PAGE_ALIGN(size) < size) + return -EINVAL; + return dev->driver->dumb_create(file_priv, dev, args); }
Lets make sure some basic expressions are always true: bpp != NULL width != NULL height != NULL stride = bpp * width < 2^32 size = 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 to do any 32bit math without having to test for mult-overflows themselves. The two divisions might hurt performance a bit, but dumb_create() is only used for scanout-buffers, so that should be fine. We could use 64bit math to avoid the divisions, but that may be slow on 32bit machines.. Or maybe there should just be a "safe_mult32()" helper, which currently doesn't exist (I think?). Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)