| Message ID | 20230407064657.12350-5-jason-jh.lin@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix mediatek-drm coverity issues | expand |
Hi, Jason: On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote: > Add casting before assign to avoid the unintentional integer overflow > or unintended sign extension. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > Fixes: 1a64a7aff8da ("drm/mediatek: Fix cursor plane no update") > --- > drivers/gpu/drm/mediatek/mtk_drm_gem.c | 2 +- > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 20 +++++++++++--------- > 2 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c > b/drivers/gpu/drm/mediatek/mtk_drm_gem.c > index 9b8f72ed12e4..537e83b95001 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c > @@ -121,7 +121,7 @@ int mtk_drm_gem_dumb_create(struct drm_file > *file_priv, struct drm_device *dev, > int ret; > > args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8); > - args->size = args->pitch * args->height; > + args->size = (unsigned long)args->pitch * args->height; The prototype is struct drm_mode_create_dump { __u32 height; __u32 pitch; __u64 size; }; Do you want to case to "__u64"? > > mtk_gem = mtk_drm_gem_create(dev, args->size, false); > if (IS_ERR(mtk_gem)) > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > index 31f9420aff6f..a1337f386bbf 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > @@ -140,7 +140,7 @@ static void mtk_plane_update_new_state(struct > drm_plane_state *new_state, > struct drm_framebuffer *fb = new_state->fb; > struct drm_gem_object *gem; > struct mtk_drm_gem_obj *mtk_gem; > - unsigned int pitch, format; > + unsigned int pitch, format, cpp; > u64 modifier; > dma_addr_t addr; > dma_addr_t hdr_addr = 0; > @@ -151,11 +151,12 @@ static void mtk_plane_update_new_state(struct > drm_plane_state *new_state, > addr = mtk_gem->dma_addr; > pitch = fb->pitches[0]; > format = fb->format->format; > + cpp = (unsigned int)fb->format->cpp[0]; > modifier = fb->modifier; > > if (modifier == DRM_FORMAT_MOD_LINEAR) { > - addr += (new_state->src.x1 >> 16) * fb->format->cpp[0]; > - addr += (new_state->src.y1 >> 16) * pitch; > + addr += (dma_addr_t)(new_state->src.x1 >> 16) * cpp; > + addr += (dma_addr_t)(new_state->src.y1 >> 16) * pitch; > } else { > int width_in_blocks = ALIGN(fb->width, > AFBC_DATA_BLOCK_WIDTH) > / AFBC_DATA_BLOCK_WIDTH; > @@ -167,17 +168,18 @@ static void mtk_plane_update_new_state(struct > drm_plane_state *new_state, > > hdr_pitch = width_in_blocks * AFBC_HEADER_BLOCK_SIZE; > pitch = width_in_blocks * AFBC_DATA_BLOCK_WIDTH * > - AFBC_DATA_BLOCK_HEIGHT * fb->format->cpp[0]; > + AFBC_DATA_BLOCK_HEIGHT * cpp; > > hdr_size = ALIGN(hdr_pitch * height_in_blocks, > AFBC_HEADER_ALIGNMENT); > > - hdr_addr = addr + hdr_pitch * y_offset_in_blocks + > - AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks; > + hdr_addr = addr + > + (dma_addr_t)hdr_pitch * y_offset_in_blocks + > + (dma_addr_t)AFBC_HEADER_BLOCK_SIZE * > x_offset_in_blocks; > /* The data plane is offset by 1 additional block. */ > addr = addr + hdr_size + > - pitch * y_offset_in_blocks + > - AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT * > - fb->format->cpp[0] * (x_offset_in_blocks + 1); > + (dma_addr_t)pitch * y_offset_in_blocks + > + (dma_addr_t)AFBC_DATA_BLOCK_WIDTH * > AFBC_DATA_BLOCK_HEIGHT * > + (dma_addr_t)cpp * (x_offset_in_blocks + 1); > } I would like you to add a variable 'u32 offset' for this calculating, and I think u32 is enough for this calculating and it's not necessary to do any casting. Regards, CK > > mtk_plane_state->pending.enable = true;
Hi CK, Thanks for the reviews. On Mon, 2023-06-12 at 09:05 +0000, CK Hu (胡俊光) wrote: > Hi, Jason: > > On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote: > > Add casting before assign to avoid the unintentional integer > > overflow > > or unintended sign extension. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > Fixes: 1a64a7aff8da ("drm/mediatek: Fix cursor plane no update") > > --- > > drivers/gpu/drm/mediatek/mtk_drm_gem.c | 2 +- > > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 20 +++++++++++--------- > > 2 files changed, 12 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c > > b/drivers/gpu/drm/mediatek/mtk_drm_gem.c > > index 9b8f72ed12e4..537e83b95001 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c > > @@ -121,7 +121,7 @@ int mtk_drm_gem_dumb_create(struct drm_file > > *file_priv, struct drm_device *dev, > > int ret; > > > > args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8); > > - args->size = args->pitch * args->height; > > + args->size = (unsigned long)args->pitch * args->height; > > The prototype is > > struct drm_mode_create_dump { > __u32 height; > __u32 pitch; > __u64 size; > }; > > Do you want to case to "__u64"? > Yes, it should be (unsigned long long), but I'll change it to: args->size = (__u64)args->pitch * args->height; > > > > mtk_gem = mtk_drm_gem_create(dev, args->size, false); > > if (IS_ERR(mtk_gem)) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > > index 31f9420aff6f..a1337f386bbf 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > > @@ -140,7 +140,7 @@ static void mtk_plane_update_new_state(struct > > drm_plane_state *new_state, > > struct drm_framebuffer *fb = new_state->fb; > > struct drm_gem_object *gem; > > struct mtk_drm_gem_obj *mtk_gem; > > - unsigned int pitch, format; > > + unsigned int pitch, format, cpp; > > u64 modifier; > > dma_addr_t addr; > > dma_addr_t hdr_addr = 0; > > @@ -151,11 +151,12 @@ static void mtk_plane_update_new_state(struct > > drm_plane_state *new_state, > > addr = mtk_gem->dma_addr; > > pitch = fb->pitches[0]; > > format = fb->format->format; > > + cpp = (unsigned int)fb->format->cpp[0]; > > modifier = fb->modifier; > > > > if (modifier == DRM_FORMAT_MOD_LINEAR) { > > - addr += (new_state->src.x1 >> 16) * fb->format->cpp[0]; > > - addr += (new_state->src.y1 >> 16) * pitch; > > + addr += (dma_addr_t)(new_state->src.x1 >> 16) * cpp; > > + addr += (dma_addr_t)(new_state->src.y1 >> 16) * pitch; > > } else { > > int width_in_blocks = ALIGN(fb->width, > > AFBC_DATA_BLOCK_WIDTH) > > / AFBC_DATA_BLOCK_WIDTH; > > @@ -167,17 +168,18 @@ static void mtk_plane_update_new_state(struct > > drm_plane_state *new_state, > > > > hdr_pitch = width_in_blocks * AFBC_HEADER_BLOCK_SIZE; > > pitch = width_in_blocks * AFBC_DATA_BLOCK_WIDTH * > > - AFBC_DATA_BLOCK_HEIGHT * fb->format->cpp[0]; > > + AFBC_DATA_BLOCK_HEIGHT * cpp; > > > > hdr_size = ALIGN(hdr_pitch * height_in_blocks, > > AFBC_HEADER_ALIGNMENT); > > > > - hdr_addr = addr + hdr_pitch * y_offset_in_blocks + > > - AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks; > > + hdr_addr = addr + > > + (dma_addr_t)hdr_pitch * y_offset_in_blocks + > > + (dma_addr_t)AFBC_HEADER_BLOCK_SIZE * > > x_offset_in_blocks; > > /* The data plane is offset by 1 additional block. */ > > addr = addr + hdr_size + > > - pitch * y_offset_in_blocks + > > - AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT * > > - fb->format->cpp[0] * (x_offset_in_blocks + 1); > > + (dma_addr_t)pitch * y_offset_in_blocks + > > + (dma_addr_t)AFBC_DATA_BLOCK_WIDTH * > > AFBC_DATA_BLOCK_HEIGHT * > > + (dma_addr_t)cpp * (x_offset_in_blocks + 1); > > } > > I would like you to add a variable 'u32 offset' for this calculating, > and I think u32 is enough for this calculating and it's not necessary > to do any casting. > > Regards, > CK I think x_offset_in_blocks and y_offset_in_blocks may be negative, so I'll use 'int offset' for this calculating. Regards, Jason-JH.Lin > > > > > mtk_plane_state->pending.enable = true;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c index 9b8f72ed12e4..537e83b95001 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c @@ -121,7 +121,7 @@ int mtk_drm_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev, int ret; args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8); - args->size = args->pitch * args->height; + args->size = (unsigned long)args->pitch * args->height; mtk_gem = mtk_drm_gem_create(dev, args->size, false); if (IS_ERR(mtk_gem)) diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c index 31f9420aff6f..a1337f386bbf 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c @@ -140,7 +140,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state, struct drm_framebuffer *fb = new_state->fb; struct drm_gem_object *gem; struct mtk_drm_gem_obj *mtk_gem; - unsigned int pitch, format; + unsigned int pitch, format, cpp; u64 modifier; dma_addr_t addr; dma_addr_t hdr_addr = 0; @@ -151,11 +151,12 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state, addr = mtk_gem->dma_addr; pitch = fb->pitches[0]; format = fb->format->format; + cpp = (unsigned int)fb->format->cpp[0]; modifier = fb->modifier; if (modifier == DRM_FORMAT_MOD_LINEAR) { - addr += (new_state->src.x1 >> 16) * fb->format->cpp[0]; - addr += (new_state->src.y1 >> 16) * pitch; + addr += (dma_addr_t)(new_state->src.x1 >> 16) * cpp; + addr += (dma_addr_t)(new_state->src.y1 >> 16) * pitch; } else { int width_in_blocks = ALIGN(fb->width, AFBC_DATA_BLOCK_WIDTH) / AFBC_DATA_BLOCK_WIDTH; @@ -167,17 +168,18 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state, hdr_pitch = width_in_blocks * AFBC_HEADER_BLOCK_SIZE; pitch = width_in_blocks * AFBC_DATA_BLOCK_WIDTH * - AFBC_DATA_BLOCK_HEIGHT * fb->format->cpp[0]; + AFBC_DATA_BLOCK_HEIGHT * cpp; hdr_size = ALIGN(hdr_pitch * height_in_blocks, AFBC_HEADER_ALIGNMENT); - hdr_addr = addr + hdr_pitch * y_offset_in_blocks + - AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks; + hdr_addr = addr + + (dma_addr_t)hdr_pitch * y_offset_in_blocks + + (dma_addr_t)AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks; /* The data plane is offset by 1 additional block. */ addr = addr + hdr_size + - pitch * y_offset_in_blocks + - AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT * - fb->format->cpp[0] * (x_offset_in_blocks + 1); + (dma_addr_t)pitch * y_offset_in_blocks + + (dma_addr_t)AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT * + (dma_addr_t)cpp * (x_offset_in_blocks + 1); } mtk_plane_state->pending.enable = true;
Add casting before assign to avoid the unintentional integer overflow or unintended sign extension. Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> Fixes: 1a64a7aff8da ("drm/mediatek: Fix cursor plane no update") --- drivers/gpu/drm/mediatek/mtk_drm_gem.c | 2 +- drivers/gpu/drm/mediatek/mtk_drm_plane.c | 20 +++++++++++--------- 2 files changed, 12 insertions(+), 10 deletions(-)