diff mbox series

[v2,3/4] drm/mediatek: Add casting before assign

Message ID 20230613113210.24949-4-jason-jh.lin@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Fix mediatek-drm coverity issues | expand

Commit Message

Jason-JH Lin (林睿祥) June 13, 2023, 11:32 a.m. UTC 
1. Add casting before assign to avoid the unintentional integer
   overflow or unintended sign extension.
2. Add a int varriable for multiplier calculation instead of calculating
   different types multiplier with dma_addr_t varriable directly.

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 | 22 +++++++++++++---------
 2 files changed, 14 insertions(+), 10 deletions(-)

Comments

AngeloGioacchino Del Regno June 14, 2023, 8:43 a.m. UTC | #1 
Il 13/06/23 13:32, Jason-JH.Lin ha scritto:
> 1. Add casting before assign to avoid the unintentional integer
>     overflow or unintended sign extension.
> 2. Add a int varriable for multiplier calculation instead of calculating
>     different types multiplier with dma_addr_t varriable directly.
> 
> 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 | 22 +++++++++++++---------
>   2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> index a25b28d3ee90..0c7878bc0b37 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 = (__u64)args->pitch * args->height;

We could avoid explicit casting here if you do

	args->size = args->pitch;
	args->size *= 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..1cd41454d545 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -145,6 +145,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
>   	dma_addr_t addr;
>   	dma_addr_t hdr_addr = 0;
>   	unsigned int hdr_pitch = 0;
> +	int offset;

...but offset can never be negative, can it?
in that case, this should be unsigned int.

>   
>   	gem = fb->obj[0];
>   	mtk_gem = to_mtk_gem_obj(gem);
> @@ -154,8 +155,10 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
>   	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;
> +		offset = (new_state->src.x1 >> 16) * fb->format->cpp[0];
> +		addr += offset;
> +		offset = (new_state->src.y1 >> 16) * pitch;
> +		addr += offset;
>   	} else {
>   		int width_in_blocks = ALIGN(fb->width, AFBC_DATA_BLOCK_WIDTH)
>   				      / AFBC_DATA_BLOCK_WIDTH;
> @@ -163,21 +166,22 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
>   				       / AFBC_DATA_BLOCK_HEIGHT;
>   		int x_offset_in_blocks = (new_state->src.x1 >> 16) / AFBC_DATA_BLOCK_WIDTH;
>   		int y_offset_in_blocks = (new_state->src.y1 >> 16) / AFBC_DATA_BLOCK_HEIGHT;
> -		int hdr_size;
> +		int hdr_size, hdr_offset;
>   
>   		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];
>   
>   		hdr_size = ALIGN(hdr_pitch * height_in_blocks, AFBC_HEADER_ALIGNMENT);
> +		hdr_offset = hdr_pitch * y_offset_in_blocks +
> +			AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;
> +		hdr_addr = addr + hdr_offset;
>   
> -		hdr_addr = addr + hdr_pitch * y_offset_in_blocks +
> -			   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);
> +		offset = pitch * y_offset_in_blocks +
> +			 AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *
> +			 fb->format->cpp[0] * (x_offset_in_blocks + 1);
> +		addr = addr + hdr_size + offset;
>   	}
>   
>   	mtk_plane_state->pending.enable = true;
Jason-JH Lin (林睿祥) June 18, 2023, 8:42 a.m. UTC | #2 
Hi Angelo,

Thanks for the reviews.

On Wed, 2023-06-14 at 10:43 +0200, AngeloGioacchino Del Regno wrote:
External email : Please do not click links or open attachments until you have verified the sender or the content.

Il 13/06/23 13:32, Jason-JH.Lin ha scritto:

> 1. Add casting before assign to avoid the unintentional integer

>     overflow or unintended sign extension.

> 2. Add a int varriable for multiplier calculation instead of calculating

>     different types multiplier with dma_addr_t varriable directly.

>

> 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 | 22 +++++++++++++---------

>   2 files changed, 14 insertions(+), 10 deletions(-)

>

> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c

> index a25b28d3ee90..0c7878bc0b37 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 = (__u64)args->pitch * args->height;


We could avoid explicit casting here if you do


args->size = args->pitch;

args->size *= args->height;


OK, Thanks for your advice.

I'll take this.


>

>   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..1cd41454d545 100644

> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c

> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c

> @@ -145,6 +145,7 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,

>   dma_addr_t addr;

>   dma_addr_t hdr_addr = 0;

>   unsigned int hdr_pitch = 0;

> +int offset;


...but offset can never be negative, can it?

in that case, this should be unsigned int.


src.x1 and src.y1 are 'int', so they can be negative.

But I'm not sure does anyone would set the negative to them.


I think using 'unsigned int' for offset would be very big after multiply with negative src.x1 or src.y1.

So I just use 'int' here to avoid that case.


Do you think I should use 'unsigned int' for offset and add the boundary checking for addr?


Regard,

Jason-JH.Lin


>

>   gem = fb->obj[0];

>   mtk_gem = to_mtk_gem_obj(gem);

> @@ -154,8 +155,10 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,

>   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;

> +offset = (new_state->src.x1 >> 16) * fb->format->cpp[0];

> +addr += offset;

> +offset = (new_state->src.y1 >> 16) * pitch;

> +addr += offset;

>   } else {

>   int width_in_blocks = ALIGN(fb->width, AFBC_DATA_BLOCK_WIDTH)

>         / AFBC_DATA_BLOCK_WIDTH;

> @@ -163,21 +166,22 @@ static void mtk_plane_update_new_state(struct drm_plane_state *new_state,

>          / AFBC_DATA_BLOCK_HEIGHT;

>   int x_offset_in_blocks = (new_state->src.x1 >> 16) / AFBC_DATA_BLOCK_WIDTH;

>   int y_offset_in_blocks = (new_state->src.y1 >> 16) / AFBC_DATA_BLOCK_HEIGHT;

> -int hdr_size;

> +int hdr_size, hdr_offset;

>

>   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];

>

>   hdr_size = ALIGN(hdr_pitch * height_in_blocks, AFBC_HEADER_ALIGNMENT);

> +hdr_offset = hdr_pitch * y_offset_in_blocks +

> +AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;

> +hdr_addr = addr + hdr_offset;

>

> -hdr_addr = addr + hdr_pitch * y_offset_in_blocks +

> -   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);

> +offset = pitch * y_offset_in_blocks +

> + AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *

> + fb->format->cpp[0] * (x_offset_in_blocks + 1);

> +addr = addr + hdr_size + offset;

>   }

>

>   mtk_plane_state->pending.enable = true;
diff mbox series

Patch

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
index a25b28d3ee90..0c7878bc0b37 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 = (__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..1cd41454d545 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -145,6 +145,7 @@  static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
 	dma_addr_t addr;
 	dma_addr_t hdr_addr = 0;
 	unsigned int hdr_pitch = 0;
+	int offset;
 
 	gem = fb->obj[0];
 	mtk_gem = to_mtk_gem_obj(gem);
@@ -154,8 +155,10 @@  static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
 	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;
+		offset = (new_state->src.x1 >> 16) * fb->format->cpp[0];
+		addr += offset;
+		offset = (new_state->src.y1 >> 16) * pitch;
+		addr += offset;
 	} else {
 		int width_in_blocks = ALIGN(fb->width, AFBC_DATA_BLOCK_WIDTH)
 				      / AFBC_DATA_BLOCK_WIDTH;
@@ -163,21 +166,22 @@  static void mtk_plane_update_new_state(struct drm_plane_state *new_state,
 				       / AFBC_DATA_BLOCK_HEIGHT;
 		int x_offset_in_blocks = (new_state->src.x1 >> 16) / AFBC_DATA_BLOCK_WIDTH;
 		int y_offset_in_blocks = (new_state->src.y1 >> 16) / AFBC_DATA_BLOCK_HEIGHT;
-		int hdr_size;
+		int hdr_size, hdr_offset;
 
 		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];
 
 		hdr_size = ALIGN(hdr_pitch * height_in_blocks, AFBC_HEADER_ALIGNMENT);
+		hdr_offset = hdr_pitch * y_offset_in_blocks +
+			AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;
+		hdr_addr = addr + hdr_offset;
 
-		hdr_addr = addr + hdr_pitch * y_offset_in_blocks +
-			   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);
+		offset = pitch * y_offset_in_blocks +
+			 AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *
+			 fb->format->cpp[0] * (x_offset_in_blocks + 1);
+		addr = addr + hdr_size + offset;
 	}
 
 	mtk_plane_state->pending.enable = true;