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