[bug report] drm: omapdrm: fb: Simplify mode command checks when creating framebuffer

[bug report] drm: omapdrm: fb: Simplify mode command checks when creating framebuffer

[Dan Carpenter]

The ancient commits from times of yore lead to the following static
checker warning:

	drivers/gpu/drm/omapdrm/omap_fb.c:429 omap_framebuffer_init()
	warn: untrusted unsigned subtract. 'omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i]' '0-u64max minus 0-u32max(user_rl)'

drivers/gpu/drm/omapdrm/omap_fb.c
    368 struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
    369                 const struct drm_format_info *info,
    370                 const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos)
    371 {
    372         struct omap_framebuffer *omap_fb = NULL;
    373         struct drm_framebuffer *fb = NULL;
    374         unsigned int pitch = mode_cmd->pitches[0];
    375         int ret, i;
    376 
    377         DBG("create framebuffer: dev=%p, mode_cmd=%p (%dx%d@%4.4s)",
    378                         dev, mode_cmd, mode_cmd->width, mode_cmd->height,
    379                         (char *)&mode_cmd->pixel_format);
    380 
    381         for (i = 0; i < ARRAY_SIZE(formats); i++) {
    382                 if (formats[i] == mode_cmd->pixel_format)
    383                         break;
    384         }
    385 
    386         if (i == ARRAY_SIZE(formats)) {
    387                 dev_dbg(dev->dev, "unsupported pixel format: %4.4s\n",
    388                         (char *)&mode_cmd->pixel_format);
    389                 ret = -EINVAL;
    390                 goto fail;
    391         }
    392 
    393         omap_fb = kzalloc(sizeof(*omap_fb), GFP_KERNEL);
    394         if (!omap_fb) {
    395                 ret = -ENOMEM;
    396                 goto fail;
    397         }
    398 
    399         fb = &omap_fb->base;
    400         omap_fb->format = info;
    401         mutex_init(&omap_fb->lock);
    402 
    403         /*
    404          * The code below assumes that no format use more than two planes, and
    405          * that the two planes of multiplane formats need the same number of
    406          * bytes per pixel.
    407          */
    408         if (info->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
    409                 dev_dbg(dev->dev, "pitches differ between planes 0 and 1\n");
    410                 ret = -EINVAL;
    411                 goto fail;
    412         }
    413 
    414         if (pitch % info->cpp[0]) {
    415                 dev_dbg(dev->dev,
    416                         "buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
    417                         pitch, info->cpp[0]);
    418                 ret = -EINVAL;
    419                 goto fail;
    420         }
    421 
    422         for (i = 0; i < info->num_planes; i++) {
    423                 struct plane *plane = &omap_fb->planes[i];
    424                 unsigned int vsub = i == 0 ? 1 : info->vsub;
    425                 unsigned int size;
    426 
    427                 size = pitch * mode_cmd->height / vsub;
    428 
--> 429                 if (size > omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i]) {

The mode_cmd->offsets[i] value comes from the user via the drm_ioctl()
and it hasn't been checked at all so far as I can see.  If it's larger
than omap_gem_mmap_size(bos[i]) then the result is unsigned so it's a
huge unsigned value which means that "size" can be anything.

It would be simple enough to add a check:

	if (mode_cmd->offsets[i] > omap_gem_mmap_size(bos[i]))
		return -EINVAL;

But I don't know the code very well so maybe there is a better place
to check for this?

    430                         dev_dbg(dev->dev,
    431                                 "provided buffer object is too small! %zu < %d\n",
    432                                 bos[i]->size - mode_cmd->offsets[i], size);
    433                         ret = -EINVAL;
    434                         goto fail;
    435                 }
    436 
    437                 fb->obj[i]    = bos[i];
    438                 plane->dma_addr  = 0;
    439         }
    440 
    441         drm_helper_mode_fill_fb_struct(dev, fb, info, mode_cmd);
    442 
    443         ret = drm_framebuffer_init(dev, fb, &omap_framebuffer_funcs);
    444         if (ret) {
    445                 dev_err(dev->dev, "framebuffer init failed: %d\n", ret);
    446                 goto fail;
    447         }
    448 
    449         DBG("create: FB ID: %d (%p)", fb->base.id, fb);
    450 
    451         return fb;
    452 
    453 fail:
    454         kfree(omap_fb);
    455 
    456         return ERR_PTR(ret);
    457 }

regards,
dan carpenter