[bug report] drm: Add support for the LogiCVC display controller

[Paul Kocialkowski]

Hello Dan,

On Tue 14 Jun 22, 15:07, Dan Carpenter wrote:
> Hello Paul Kocialkowski,
> 
> The patch efeeaefe9be5: "drm: Add support for the LogiCVC display
> controller" from May 20, 2022, leads to the following Smatch static
> checker warning:
> 
> 	drivers/gpu/drm/logicvc/logicvc_layer.c:320 logicvc_layer_buffer_find_setup()
> 	warn: impossible condition '(hoffset > (((((1))) << (16)) - 1)) => (0-u16max > u16max)'
> 
> drivers/gpu/drm/logicvc/logicvc_layer.c
>     258 int logicvc_layer_buffer_find_setup(struct logicvc_drm *logicvc,
>     259                                     struct logicvc_layer *layer,
>     260                                     struct drm_plane_state *state,
>     261                                     struct logicvc_layer_buffer_setup *setup)
>     262 {
>     263         struct drm_device *drm_dev = &logicvc->drm_dev;
>     264         struct drm_framebuffer *fb = state->fb;
>     265         /* All the supported formats have a single data plane. */
>     266         u32 layer_bytespp = fb->format->cpp[0];
>     267         u32 layer_stride = layer_bytespp * logicvc->config.row_stride;
>     268         u32 base_offset = layer->config.base_offset * layer_stride;
>     269         u32 buffer_offset = layer->config.buffer_offset * layer_stride;
>     270         u8 buffer_sel = 0;
>     271         u16 voffset = 0;
>     272         u16 hoffset = 0;
>     273         phys_addr_t fb_addr;
>     274         u32 fb_offset;
>     275         u32 gap;
>     276 
>     277         if (!logicvc->reserved_mem_base) {
>     278                 drm_err(drm_dev, "No reserved memory base was registered!\n");
>     279                 return -ENOMEM;
>     280         }
>     281 
>     282         fb_addr = drm_fb_cma_get_gem_addr(fb, state, 0);
>     283         if (fb_addr < logicvc->reserved_mem_base) {
>     284                 drm_err(drm_dev,
>     285                         "Framebuffer memory below reserved memory base!\n");
>     286                 return -EINVAL;
>     287         }
>     288 
>     289         fb_offset = (u32) (fb_addr - logicvc->reserved_mem_base);
>     290 
>     291         if (fb_offset < base_offset) {
>     292                 drm_err(drm_dev,
>     293                         "Framebuffer offset below layer base offset!\n");
>     294                 return -EINVAL;
>     295         }
>     296 
>     297         gap = fb_offset - base_offset;
>     298 
>     299         /* Use the possible video buffers selection. */
>     300         if (gap && buffer_offset) {
>     301                 buffer_sel = gap / buffer_offset;
>     302                 if (buffer_sel > LOGICVC_BUFFER_SEL_MAX)
>     303                         buffer_sel = LOGICVC_BUFFER_SEL_MAX;
>     304 
>     305                 gap -= buffer_sel * buffer_offset;
>     306         }
>     307 
>     308         /* Use the vertical offset. */
>     309         if (gap && layer_stride && logicvc->config.layers_configurable) {
>     310                 voffset = gap / layer_stride;
>     311                 if (voffset > LOGICVC_LAYER_VOFFSET_MAX)
>     312                         voffset = LOGICVC_LAYER_VOFFSET_MAX;
>     313 
>     314                 gap -= voffset * layer_stride;
>     315         }
>     316 
>     317         /* Use the horizontal offset. */
>     318         if (gap && layer_bytespp && logicvc->config.layers_configurable) {
>     319                 hoffset = gap / layer_bytespp;
> 
> Can "gap / layer_bytespp" ever be more than USHRT_MAX?  Because if so
> that won't fit into "hoffset"

Well there is nothing that really restricts the size of the gap, so yes this
could happen. At this stage the gap should have been reduced already but we
never really know.

Would it make sense to add a check that gap / layer_bytespp <= USHRT_MAX
in that if statement?

Thanks for the catch.

Paul

> --> 320                 if (hoffset > LOGICVC_DIMENSIONS_MAX)
>     321                         hoffset = LOGICVC_DIMENSIONS_MAX;
>     322 
>     323                 gap -= hoffset * layer_bytespp;
>     324         }
>     325 
>     326         if (gap) {
>     327                 drm_err(drm_dev,
>     328                         "Unable to find layer %d buffer setup for 0x%x byte gap\n",
>     329                         layer->index, fb_offset - base_offset);
>     330                 return -EINVAL;
>     331         }
>     332 
>     333         drm_dbg_kms(drm_dev, "Found layer %d buffer setup for 0x%x byte gap:\n",
>     334                     layer->index, fb_offset - base_offset);
>     335 
>     336         drm_dbg_kms(drm_dev, "- buffer_sel = 0x%x chunks of 0x%x bytes\n",
>     337                     buffer_sel, buffer_offset);
>     338         drm_dbg_kms(drm_dev, "- voffset = 0x%x chunks of 0x%x bytes\n", voffset,
>     339                     layer_stride);
>     340         drm_dbg_kms(drm_dev, "- hoffset = 0x%x chunks of 0x%x bytes\n", hoffset,
>     341                     layer_bytespp);
>     342 
>     343         if (setup) {
>     344                 setup->buffer_sel = buffer_sel;
>     345                 setup->voffset = voffset;
>     346                 setup->hoffset = hoffset;
>     347         }
>     348 
>     349         return 0;
>     350 }
> 
> regards,
> dan carpenter

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220624/568ab6dd/attachment.sig>