On Fri, Jun 24, 2022 at 04:53:25PM +0200, Paul Kocialkowski wrote:
> 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?
>
My favorite fix would be to declare "hoffset" as a unsigned int.
regards,
dan carpenter
