[PATCH 0/5] Fix mediatek-drm coverity issues

[PATCH 0/5] Fix mediatek-drm coverity issues

[Jason-JH.Lin]

Add this patch series to fix these mediatek-drm coverity issues.

Jason-JH.Lin (5):
  drm/mediatek: Remove freeing not dynamic allocated memory
  drm/mediatek: Add cnt checking for coverity issue
  drm/mediatek: Add initialization for mtk_gem_obj
  drm/mediatek: Add casting before assign
  drm/mediatek: Fix dereference before null check

 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  |  1 -
 drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  4 ++--
 drivers/gpu/drm/mediatek/mtk_drm_gem.c   |  4 ++--
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 29 +++++++++++-------------
 4 files changed, 17 insertions(+), 21 deletions(-)

-- 
2.18.0

[PATCH 1/5] drm/mediatek: Remove freeing not dynamic allocated memory

[Jason-JH.Lin]

Fixing the coverity issue of:
mtk_drm_cmdq_pkt_destroy frees address of mtk_crtc->cmdq_handle

So remove the free function.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
Fixes: 7627122fd1c0 ("drm/mediatek: Add cmdq_handle in mtk_crtc")
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 4bc45cdbddf1..c7b03e564095 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -148,7 +148,6 @@ static void mtk_drm_cmdq_pkt_destroy(struct cmdq_pkt *pkt)
 	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt->buf_size,
 			 DMA_TO_DEVICE);
 	kfree(pkt->va_base);
-	kfree(pkt);
 }
 #endif
 
-- 
2.18.0

[PATCH 2/5] drm/mediatek: Add cnt checking for coverity issue

[Jason-JH.Lin]

CERT-C Characters and Strings (CERT STR31-C)
all_drm_priv[cnt] evaluates to an address that could be at negative
offset of an array.

In mtk_drm_get_all_drm_priv():
Guarantee that storage for strings has sufficient space for character
data and the null terminator.

So change cnt to unsigned int and check its max value.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
Fixes: 1ef7ed48356c ("drm/mediatek: Modify mediatek-drm for mt8195 multi mmsys support")
---
 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 86255a066faf..fcfa10332166 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -378,7 +378,7 @@ static bool mtk_drm_get_all_drm_priv(struct device *dev)
 	const struct of_device_id *of_id;
 	struct device_node *node;
 	struct device *drm_dev;
-	int cnt = 0;
+	unsigned int cnt = 0;
 	int i, j;
 
 	for_each_child_of_node(phandle->parent, node) {
@@ -397,7 +397,7 @@ static bool mtk_drm_get_all_drm_priv(struct device *dev)
 			continue;
 
 		all_drm_priv[cnt] = dev_get_drvdata(drm_dev);
-		if (all_drm_priv[cnt] && all_drm_priv[cnt]->mtk_drm_bound)
+		if (cnt < MAX_CRTC && all_drm_priv[cnt] && all_drm_priv[cnt]->mtk_drm_bound)
 			cnt++;
 	}
 
-- 
2.18.0

[PATCH 3/5] drm/mediatek: Add initialization for mtk_gem_obj

[Jason-JH.Lin]

Calling mtk_gem_obj = kzalloc() which returns uninitialized memory,
because mtk_gem_obj is uninitialized.

It may cause using uninitialized value mtk_gem_obj->base.resv
when calling drm_gem_object_init().

So add initialization for mtk_gem_obj.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
---
 drivers/gpu/drm/mediatek/mtk_drm_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
index a25b28d3ee90..9b8f72ed12e4 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
@@ -33,7 +33,7 @@ static const struct drm_gem_object_funcs mtk_drm_gem_object_funcs = {
 static struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device *dev,
 						unsigned long size)
 {
-	struct mtk_drm_gem_obj *mtk_gem_obj;
+	struct mtk_drm_gem_obj *mtk_gem_obj = NULL;
 	int ret;
 
 	size = round_up(size, PAGE_SIZE);
-- 
2.18.0

[PATCH 4/5] drm/mediatek: Add casting before assign

[Jason-JH.Lin]

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

[PATCH 5/5] drm/mediatek: Fix dereference before null check

[Jason-JH.Lin]

Null-checking state suggests that it may be null, but it has already
been dereferenced on drm_atomic_get_new_plane_state(state, plane).

The parameter state will never be NULL currently, so just remove the
state is NULL flow in this function.

Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
Fixes: 5ddb0bd4ddc3 ("drm/atomic: Pass the full state to planes async atomic check and update")
---
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index a1337f386bbf..e14b2920d242 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -103,8 +103,7 @@ static void mtk_drm_plane_destroy_state(struct drm_plane *plane,
 static int mtk_plane_atomic_async_check(struct drm_plane *plane,
 					struct drm_atomic_state *state)
 {
-	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
-										 plane);
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
 	struct drm_crtc_state *crtc_state;
 	int ret;
 
@@ -122,11 +121,7 @@ static int mtk_plane_atomic_async_check(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
-	if (state)
-		crtc_state = drm_atomic_get_existing_crtc_state(state,
-								new_plane_state->crtc);
-	else /* Special case for asynchronous cursor updates. */
-		crtc_state = new_plane_state->crtc->state;
+	crtc_state = drm_atomic_get_existing_crtc_state(state, new_plane_state->crtc);
 
 	return drm_atomic_helper_check_plane_state(plane->state, crtc_state,
 						   DRM_PLANE_NO_SCALING,
-- 
2.18.0

Re: [PATCH 1/5] drm/mediatek: Remove freeing not dynamic allocated memory

[CK Hu (胡俊光)]

[-- Attachment #1: Type: text/html, Size: 2400 bytes --]

[-- Attachment #2: Type: text/plain, Size: 1008 bytes --]

Hi, Jason:

On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> Fixing the coverity issue of:
> mtk_drm_cmdq_pkt_destroy frees address of mtk_crtc->cmdq_handle
> 
> So remove the free function.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> Fixes: 7627122fd1c0 ("drm/mediatek: Add cmdq_handle in mtk_crtc")
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 4bc45cdbddf1..c7b03e564095 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -148,7 +148,6 @@ static void mtk_drm_cmdq_pkt_destroy(struct
> cmdq_pkt *pkt)
>  	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt-
> >buf_size,
>  			 DMA_TO_DEVICE);
>  	kfree(pkt->va_base);
> -	kfree(pkt);


Also fix the same problem in mtk_drm_cmdq_pkt_create().

Regards,
CK

>  }
>  #endif
>  

Re: [PATCH 2/5] drm/mediatek: Add cnt checking for coverity issue

[CK Hu (胡俊光)]

[-- Attachment #1: Type: text/html, Size: 3487 bytes --]

[-- Attachment #2: Type: text/plain, Size: 1669 bytes --]

Hi, Jason:

On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> CERT-C Characters and Strings (CERT STR31-C)
> all_drm_priv[cnt] evaluates to an address that could be at negative
> offset of an array.
> 
> In mtk_drm_get_all_drm_priv():
> Guarantee that storage for strings has sufficient space for character
> data and the null terminator.
> 
> So change cnt to unsigned int and check its max value.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> Fixes: 1ef7ed48356c ("drm/mediatek: Modify mediatek-drm for mt8195
> multi mmsys support")
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 86255a066faf..fcfa10332166 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -378,7 +378,7 @@ static bool mtk_drm_get_all_drm_priv(struct
> device *dev)
>  	const struct of_device_id *of_id;
>  	struct device_node *node;
>  	struct device *drm_dev;
> -	int cnt = 0;
> +	unsigned int cnt = 0;
>  	int i, j;
>  
>  	for_each_child_of_node(phandle->parent, node) {
> @@ -397,7 +397,7 @@ static bool mtk_drm_get_all_drm_priv(struct
> device *dev)
>  			continue;
>  
>  		all_drm_priv[cnt] = dev_get_drvdata(drm_dev);
> -		if (all_drm_priv[cnt] && all_drm_priv[cnt]-
> >mtk_drm_bound)
> +		if (cnt < MAX_CRTC && all_drm_priv[cnt] &&
> all_drm_priv[cnt]->mtk_drm_bound)
>  			cnt++;


I would like to add below statement here:

		if (cnt == MAX_CRTC)
			break;

Regards,
CK

>  	}
>  

Re: [PATCH 3/5] drm/mediatek: Add initialization for mtk_gem_obj

[CK Hu (胡俊光)]

[-- Attachment #1: Type: text/html, Size: 2748 bytes --]

[-- Attachment #2: Type: text/plain, Size: 1235 bytes --]

Hi, Jason:

On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> Calling mtk_gem_obj = kzalloc() which returns uninitialized memory,
> because mtk_gem_obj is uninitialized.
> 
> It may cause using uninitialized value mtk_gem_obj->base.resv
> when calling drm_gem_object_init().
> 
> So add initialization for mtk_gem_obj.

So weird, nacked by me.

Regards,
CK

> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> MT8173.")
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> index a25b28d3ee90..9b8f72ed12e4 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> @@ -33,7 +33,7 @@ static const struct drm_gem_object_funcs
> mtk_drm_gem_object_funcs = {
>  static struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device
> *dev,
>  						unsigned long size)
>  {
> -	struct mtk_drm_gem_obj *mtk_gem_obj;
> +	struct mtk_drm_gem_obj *mtk_gem_obj = NULL;
>  	int ret;
>  
>  	size = round_up(size, PAGE_SIZE);

Re: [PATCH 4/5] drm/mediatek: Add casting before assign

[CK Hu (胡俊光)]

[-- Attachment #1: Type: text/html, Size: 6971 bytes --]

[-- Attachment #2: Type: text/plain, Size: 3887 bytes --]

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;

Re: [PATCH 5/5] drm/mediatek: Fix dereference before null check

[CK Hu (胡俊光)]

[-- Attachment #1: Type: text/html, Size: 3825 bytes --]

[-- Attachment #2: Type: text/plain, Size: 1985 bytes --]

Hi, Jason:

On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> Null-checking state suggests that it may be null, but it has already
> been dereferenced on drm_atomic_get_new_plane_state(state, plane).
> 
> The parameter state will never be NULL currently, so just remove the
> state is NULL flow in this function.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> Fixes: 5ddb0bd4ddc3 ("drm/atomic: Pass the full state to planes async
> atomic check and update")
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index a1337f386bbf..e14b2920d242 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -103,8 +103,7 @@ static void mtk_drm_plane_destroy_state(struct
> drm_plane *plane,
>  static int mtk_plane_atomic_async_check(struct drm_plane *plane,
>  					struct drm_atomic_state *state)
>  {
> -	struct drm_plane_state *new_plane_state =
> drm_atomic_get_new_plane_state(state,
> -									
> 	 plane);
> +	struct drm_plane_state *new_plane_state =
> drm_atomic_get_new_plane_state(state, plane);

This is not related to this patch, so move to another patch.

Regards,
CK

>  	struct drm_crtc_state *crtc_state;
>  	int ret;
>  
> @@ -122,11 +121,7 @@ static int mtk_plane_atomic_async_check(struct
> drm_plane *plane,
>  	if (ret)
>  		return ret;
>  
> -	if (state)
> -		crtc_state = drm_atomic_get_existing_crtc_state(state,
> -								new_pla
> ne_state->crtc);
> -	else /* Special case for asynchronous cursor updates. */
> -		crtc_state = new_plane_state->crtc->state;
> +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> new_plane_state->crtc);
>  
>  	return drm_atomic_helper_check_plane_state(plane->state,
> crtc_state,
>  						   DRM_PLANE_NO_SCALING
> ,

Re: [PATCH 1/5] drm/mediatek: Remove freeing not dynamic allocated memory

[Jason-JH Lin (林睿祥)]

[-- Attachment #1: Type: text/html, Size: 3028 bytes --]

[-- Attachment #2: Type: text/plain, Size: 1263 bytes --]

Hi CK,

Thanks for the reviews.

On Mon, 2023-06-12 at 08:06 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> > Fixing the coverity issue of:
> > mtk_drm_cmdq_pkt_destroy frees address of mtk_crtc->cmdq_handle
> > 
> > So remove the free function.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > Fixes: 7627122fd1c0 ("drm/mediatek: Add cmdq_handle in mtk_crtc")
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 4bc45cdbddf1..c7b03e564095 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -148,7 +148,6 @@ static void mtk_drm_cmdq_pkt_destroy(struct
> > cmdq_pkt *pkt)
> >  	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt-
> > > buf_size,
> > 
> >  			 DMA_TO_DEVICE);
> >  	kfree(pkt->va_base);
> > -	kfree(pkt);
> 
> 
> Also fix the same problem in mtk_drm_cmdq_pkt_create().
> 
> Regards,
> CK

OK, I'll also fix them. Thanks for the reminder.

Regards,
Jason-JH.Lin
> 
> >  }
> >  #endif
> >  

Re: [PATCH 2/5] drm/mediatek: Add cnt checking for coverity issue

[Jason-JH Lin (林睿祥)]

[-- Attachment #1: Type: text/html, Size: 4295 bytes --]

[-- Attachment #2: Type: text/plain, Size: 1959 bytes --]

Hi CK,

Thanks for the reviews.

On Mon, 2023-06-12 at 08:21 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> > CERT-C Characters and Strings (CERT STR31-C)
> > all_drm_priv[cnt] evaluates to an address that could be at negative
> > offset of an array.
> > 
> > In mtk_drm_get_all_drm_priv():
> > Guarantee that storage for strings has sufficient space for
> > character
> > data and the null terminator.
> > 
> > So change cnt to unsigned int and check its max value.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > Fixes: 1ef7ed48356c ("drm/mediatek: Modify mediatek-drm for mt8195
> > multi mmsys support")
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > index 86255a066faf..fcfa10332166 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > @@ -378,7 +378,7 @@ static bool mtk_drm_get_all_drm_priv(struct
> > device *dev)
> >  	const struct of_device_id *of_id;
> >  	struct device_node *node;
> >  	struct device *drm_dev;
> > -	int cnt = 0;
> > +	unsigned int cnt = 0;
> >  	int i, j;
> >  
> >  	for_each_child_of_node(phandle->parent, node) {
> > @@ -397,7 +397,7 @@ static bool mtk_drm_get_all_drm_priv(struct
> > device *dev)
> >  			continue;
> >  
> >  		all_drm_priv[cnt] = dev_get_drvdata(drm_dev);
> > -		if (all_drm_priv[cnt] && all_drm_priv[cnt]-
> > > mtk_drm_bound)
> > 
> > +		if (cnt < MAX_CRTC && all_drm_priv[cnt] &&
> > all_drm_priv[cnt]->mtk_drm_bound)
> >  			cnt++;
> 
> 
> I would like to add below statement here:
> 
> 		if (cnt == MAX_CRTC)
> 			break;
> 
> Regards,
> CK

OK, I'll change to this statement.


Regards,
Jason-JH.Lin
> 
> >  	}
> >  

Re: [PATCH 3/5] drm/mediatek: Add initialization for mtk_gem_obj

[Jason-JH Lin (林睿祥)]

[-- Attachment #1: Type: text/html, Size: 4337 bytes --]

[-- Attachment #2: Type: text/plain, Size: 2141 bytes --]

Hi CK,

Thanks for the reviews.

On Mon, 2023-06-12 at 08:27 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> > Calling mtk_gem_obj = kzalloc() which returns uninitialized memory,
> > because mtk_gem_obj is uninitialized.
> > 
> > It may cause using uninitialized value mtk_gem_obj->base.resv
> > when calling drm_gem_object_init().
> > 
> > So add initialization for mtk_gem_obj.
> 
> So weird, nacked by me.
> 
> Regards,
> CK
> 
I've checked the coverity issue:
1. alloc_fn: Calling kzalloc which returns uninitialized memory. 
2. assign: Assigning: mtk_gem_obj = kzalloc(424UL, 3264U), which points
to uninitialized data.
   mtk_gem_obj = kzalloc(sizeof(*mtk_gem_obj), GFP_KERNEL);

/include/linux/slab.h
733: static inline void *kzalloc(size_t size, gfp_t flags)
734: {
1. uninit_buffer: Call to kmalloc(size, flags | 0x100U) returns an
uninitialized buffer. [Note: The source code implementation of the
function has been overridden by a builtin model.]
2. return: Returning kmalloc(size, flags | 0x100U).


I think it may be a misjudgment to kzalloc returning an uninitialized
memory, so I'll drop this path.

Regards,
Jason-JH.Lin

> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> > MT8173.")
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_gem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > index a25b28d3ee90..9b8f72ed12e4 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > @@ -33,7 +33,7 @@ static const struct drm_gem_object_funcs
> > mtk_drm_gem_object_funcs = {
> >  static struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device
> > *dev,
> >  						unsigned long size)
> >  {
> > -	struct mtk_drm_gem_obj *mtk_gem_obj;
> > +	struct mtk_drm_gem_obj *mtk_gem_obj = NULL;
> >  	int ret;
> >  
> >  	size = round_up(size, PAGE_SIZE);

Re: [PATCH 4/5] drm/mediatek: Add casting before assign

[Jason-JH Lin (林睿祥)]

[-- Attachment #1: Type: text/html, Size: 8525 bytes --]

[-- Attachment #2: Type: text/plain, Size: 4467 bytes --]

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;

Re: [PATCH 5/5] drm/mediatek: Fix dereference before null check

[Jason-JH Lin (林睿祥)]

[-- Attachment #1: Type: text/html, Size: 4656 bytes --]

[-- Attachment #2: Type: text/plain, Size: 2282 bytes --]

Hi CK,

Thanks for the reviews.

On Mon, 2023-06-12 at 09:10 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Fri, 2023-04-07 at 14:46 +0800, Jason-JH.Lin wrote:
> > Null-checking state suggests that it may be null, but it has
> > already
> > been dereferenced on drm_atomic_get_new_plane_state(state, plane).
> > 
> > The parameter state will never be NULL currently, so just remove
> > the
> > state is NULL flow in this function.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > Fixes: 5ddb0bd4ddc3 ("drm/atomic: Pass the full state to planes
> > async
> > atomic check and update")
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > index a1337f386bbf..e14b2920d242 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > @@ -103,8 +103,7 @@ static void mtk_drm_plane_destroy_state(struct
> > drm_plane *plane,
> >  static int mtk_plane_atomic_async_check(struct drm_plane *plane,
> >  					struct drm_atomic_state *state)
> >  {
> > -	struct drm_plane_state *new_plane_state =
> > drm_atomic_get_new_plane_state(state,
> > -									
> > 	 plane);
> > +	struct drm_plane_state *new_plane_state =
> > drm_atomic_get_new_plane_state(state, plane);
> 
> This is not related to this patch, so move to another patch.
> 
> Regards,
> CK

OK, I'll drop the modification here.

Regards,
Jason-JH.Lin
> 
> >  	struct drm_crtc_state *crtc_state;
> >  	int ret;
> >  
> > @@ -122,11 +121,7 @@ static int mtk_plane_atomic_async_check(struct
> > drm_plane *plane,
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (state)
> > -		crtc_state = drm_atomic_get_existing_crtc_state(state,
> > -								new_pla
> > ne_state->crtc);
> > -	else /* Special case for asynchronous cursor updates. */
> > -		crtc_state = new_plane_state->crtc->state;
> > +	crtc_state = drm_atomic_get_existing_crtc_state(state,
> > new_plane_state->crtc);
> >  
> >  	return drm_atomic_helper_check_plane_state(plane->state,
> > crtc_state,
> >  						   DRM_PLANE_NO_SCALING
> > ,