RE: [PATCH] drm/amd/display: fix the system memory page fault because of copy overflow

RE: [PATCH] drm/amd/display: fix the system memory page fault because of copy overflow

[Su, Jinzhou (Joe)]

[AMD Official Use Only - Internal Distribution Only]

Reviewed-by: Jinzhou.Su <Jinzhou.Su@amd.com>

Regards,
Joe

-----Original Message-----
From: Huang, Ray <Ray.Huang@amd.com> 
Sent: Saturday, January 16, 2021 2:47 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Su, Jinzhou (Joe) <Jinzhou.Su@amd.com>; Zhu, Changfeng <Changfeng.Zhu@amd.com>; Huang, Ray <Ray.Huang@amd.com>; Lee Jones <lee.jones@linaro.org>
Subject: [PATCH] drm/amd/display: fix the system memory page fault because of copy overflow

The buffer is allocated with the size of pointer and copy with the size of data structure. Then trigger the system memory page fault. Use the orignal data structure to get the object size.

Fixes: a8e30005b drm/amd/display/dc/core/dc_link: Move some local data from the stack to the heap

Signed-off-by: Huang Rui <ray.huang@amd.com>
Cc: Lee Jones <lee.jones@linaro.org>
---
 drivers/gpu/drm/amd/display/dc/core/dc_link.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
index 69573d67056d..73178978ae74 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
@@ -1380,7 +1380,7 @@ static bool dc_link_construct(struct dc_link *link,
 
 	DC_LOGGER_INIT(dc_ctx->logger);
 
-	info = kzalloc(sizeof(info), GFP_KERNEL);
+	info = kzalloc(sizeof(struct integrated_info), GFP_KERNEL);
 	if (!info)
 		goto create_fail;
 
@@ -1545,7 +1545,7 @@ static bool dc_link_construct(struct dc_link *link,
 	}
 
 	if (bios->integrated_info)
-		memcpy(info, bios->integrated_info, sizeof(*info));
+		memcpy(info, bios->integrated_info, sizeof(struct integrated_info));
 
 	/* Look for channel mapping corresponding to connector and device tag */
 	for (i = 0; i < MAX_NUMBER_OF_EXT_DISPLAY_PATH; i++) {
--
2.25.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: fix the system memory page fault because of copy overflow

[Lee Jones]

On Sat, 16 Jan 2021, Huang Rui wrote:

> The buffer is allocated with the size of pointer and copy with the size of
> data structure. Then trigger the system memory page fault. Use the
> orignal data structure to get the object size.
> 
> Fixes: a8e30005b drm/amd/display/dc/core/dc_link: Move some local data
> from the stack to the heap
> 
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/gpu/drm/amd/display/dc/core/dc_link.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> index 69573d67056d..73178978ae74 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> @@ -1380,7 +1380,7 @@ static bool dc_link_construct(struct dc_link *link,
>  
>  	DC_LOGGER_INIT(dc_ctx->logger);
>  
> -	info = kzalloc(sizeof(info), GFP_KERNEL);

Ah sorry, this should be (*info).

> +	info = kzalloc(sizeof(struct integrated_info), GFP_KERNEL);

Using the full name like this is usually discouraged.

>  	if (!info)
>  		goto create_fail;
>  
> @@ -1545,7 +1545,7 @@ static bool dc_link_construct(struct dc_link *link,
>  	}
>  
>  	if (bios->integrated_info)
> -		memcpy(info, bios->integrated_info, sizeof(*info));

This should be correct.

> +		memcpy(info, bios->integrated_info, sizeof(struct integrated_info));
>  
>  	/* Look for channel mapping corresponding to connector and device tag */
>  	for (i = 0; i < MAX_NUMBER_OF_EXT_DISPLAY_PATH; i++) {

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: fix the system memory page fault because of copy overflow

[Christian König]

Am 15.01.21 um 19:46 schrieb Huang Rui:
> The buffer is allocated with the size of pointer and copy with the size of
> data structure. Then trigger the system memory page fault. Use the
> orignal data structure to get the object size.
>
> Fixes: a8e30005b drm/amd/display/dc/core/dc_link: Move some local data
> from the stack to the heap
>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/gpu/drm/amd/display/dc/core/dc_link.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> index 69573d67056d..73178978ae74 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> @@ -1380,7 +1380,7 @@ static bool dc_link_construct(struct dc_link *link,
>   
>   	DC_LOGGER_INIT(dc_ctx->logger);
>   
> -	info = kzalloc(sizeof(info), GFP_KERNEL);
> +	info = kzalloc(sizeof(struct integrated_info), GFP_KERNEL);

That should probably be sizeof(*info) instead, we usually try to avoid 
sizeof(struct ...) in the kernel.

There are some automated scripts in place which will send you a patch to 
change it otherwise.

>   	if (!info)
>   		goto create_fail;
>   
> @@ -1545,7 +1545,7 @@ static bool dc_link_construct(struct dc_link *link,
>   	}
>   
>   	if (bios->integrated_info)
> -		memcpy(info, bios->integrated_info, sizeof(*info));
> +		memcpy(info, bios->integrated_info, sizeof(struct integrated_info));

This can then also stay as it is.

Apart from that good catch.

Regards,
Christian.

>   
>   	/* Look for channel mapping corresponding to connector and device tag */
>   	for (i = 0; i < MAX_NUMBER_OF_EXT_DISPLAY_PATH; i++) {

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amd/display: fix the system memory page fault because of copy overflow

[Chen, Jiansong (Simon)]

[AMD Public Use]

Hi Rui,
Seems the change has violated the kernel coding style😊, please help check.
https://www.kernel.org/doc/html/latest/process/coding-style.html

Allocating memory
......
The preferred form for passing a size of a struct is the following:

p = kmalloc(sizeof(*p), ...);
The alternative form where struct name is spelled out hurts readability and introduces an opportunity for a bug when the pointer variable type is changed but the corresponding sizeof that is passed to a memory allocator is not.

Regards,
Jiansong
-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Huang Rui
Sent: Saturday, January 16, 2021 2:47 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Su, Jinzhou (Joe) <Jinzhou.Su@amd.com>; Huang, Ray <Ray.Huang@amd.com>; Lee Jones <lee.jones@linaro.org>; Zhu, Changfeng <Changfeng.Zhu@amd.com>
Subject: [PATCH] drm/amd/display: fix the system memory page fault because of copy overflow

The buffer is allocated with the size of pointer and copy with the size of data structure. Then trigger the system memory page fault. Use the orignal data structure to get the object size.

Fixes: a8e30005b drm/amd/display/dc/core/dc_link: Move some local data from the stack to the heap

Signed-off-by: Huang Rui <ray.huang@amd.com>
Cc: Lee Jones <lee.jones@linaro.org>
---
 drivers/gpu/drm/amd/display/dc/core/dc_link.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
index 69573d67056d..73178978ae74 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
@@ -1380,7 +1380,7 @@ static bool dc_link_construct(struct dc_link *link,

 DC_LOGGER_INIT(dc_ctx->logger);

-info = kzalloc(sizeof(info), GFP_KERNEL);
+info = kzalloc(sizeof(struct integrated_info), GFP_KERNEL);
 if (!info)
 goto create_fail;

@@ -1545,7 +1545,7 @@ static bool dc_link_construct(struct dc_link *link,
 }

 if (bios->integrated_info)
-memcpy(info, bios->integrated_info, sizeof(*info));
+memcpy(info, bios->integrated_info, sizeof(struct integrated_info));

 /* Look for channel mapping corresponding to connector and device tag */
 for (i = 0; i < MAX_NUMBER_OF_EXT_DISPLAY_PATH; i++) {
--
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJiansong.Chen%40amd.com%7Caa1f0e0196584ac4145208d8b942ff50%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637463044695608478%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OhlBMm03tg0JUctjpEtO88hL1Dnu5wxt7Keuojm61NQ%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: fix the system memory page fault because of copy overflow

[Lee Jones]

On Fri, 15 Jan 2021, Christian König wrote:

> Am 15.01.21 um 19:46 schrieb Huang Rui:
> > The buffer is allocated with the size of pointer and copy with the size of
> > data structure. Then trigger the system memory page fault. Use the
> > orignal data structure to get the object size.
> > 
> > Fixes: a8e30005b drm/amd/display/dc/core/dc_link: Move some local data
> > from the stack to the heap
> > 
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > ---
> >   drivers/gpu/drm/amd/display/dc/core/dc_link.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > index 69573d67056d..73178978ae74 100644
> > --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > @@ -1380,7 +1380,7 @@ static bool dc_link_construct(struct dc_link *link,
> >   	DC_LOGGER_INIT(dc_ctx->logger);
> > -	info = kzalloc(sizeof(info), GFP_KERNEL);
> > +	info = kzalloc(sizeof(struct integrated_info), GFP_KERNEL);
> 
> That should probably be sizeof(*info) instead, we usually try to avoid
> sizeof(struct ...) in the kernel.
> 
> There are some automated scripts in place which will send you a patch to
> change it otherwise.
> 
> >   	if (!info)
> >   		goto create_fail;
> > @@ -1545,7 +1545,7 @@ static bool dc_link_construct(struct dc_link *link,
> >   	}
> >   	if (bios->integrated_info)
> > -		memcpy(info, bios->integrated_info, sizeof(*info));
> > +		memcpy(info, bios->integrated_info, sizeof(struct integrated_info));
> 
> This can then also stay as it is.
> 
> Apart from that good catch.

Yes, agreed.

Sorry for the fuss.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/display: fix the system memory page fault because of copy overflow

[Huang Rui]

The buffer is allocated with the size of pointer and copy with the size of
data structure. Then trigger the system memory page fault. Use the
orignal data structure to get the object size.

Fixes: a8e30005b drm/amd/display/dc/core/dc_link: Move some local data
from the stack to the heap

Signed-off-by: Huang Rui <ray.huang@amd.com>
Cc: Lee Jones <lee.jones@linaro.org>
---
 drivers/gpu/drm/amd/display/dc/core/dc_link.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
index 69573d67056d..73178978ae74 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
@@ -1380,7 +1380,7 @@ static bool dc_link_construct(struct dc_link *link,
 
 	DC_LOGGER_INIT(dc_ctx->logger);
 
-	info = kzalloc(sizeof(info), GFP_KERNEL);
+	info = kzalloc(sizeof(struct integrated_info), GFP_KERNEL);
 	if (!info)
 		goto create_fail;
 
@@ -1545,7 +1545,7 @@ static bool dc_link_construct(struct dc_link *link,
 	}
 
 	if (bios->integrated_info)
-		memcpy(info, bios->integrated_info, sizeof(*info));
+		memcpy(info, bios->integrated_info, sizeof(struct integrated_info));
 
 	/* Look for channel mapping corresponding to connector and device tag */
 	for (i = 0; i < MAX_NUMBER_OF_EXT_DISPLAY_PATH; i++) {
-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: fix the system memory page fault because of copy overflow

[Huang Rui]

On Fri, Jan 15, 2021 at 07:22:43PM +0800, Christian König wrote:
> Am 15.01.21 um 19:46 schrieb Huang Rui:
> > The buffer is allocated with the size of pointer and copy with the size of
> > data structure. Then trigger the system memory page fault. Use the
> > orignal data structure to get the object size.
> >
> > Fixes: a8e30005b drm/amd/display/dc/core/dc_link: Move some local data
> > from the stack to the heap
> >
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > ---
> >   drivers/gpu/drm/amd/display/dc/core/dc_link.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > index 69573d67056d..73178978ae74 100644
> > --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > @@ -1380,7 +1380,7 @@ static bool dc_link_construct(struct dc_link *link,
> >   
> >   	DC_LOGGER_INIT(dc_ctx->logger);
> >   
> > -	info = kzalloc(sizeof(info), GFP_KERNEL);
> > +	info = kzalloc(sizeof(struct integrated_info), GFP_KERNEL);
> 
> That should probably be sizeof(*info) instead, we usually try to avoid 
> sizeof(struct ...) in the kernel.
> 
> There are some automated scripts in place which will send you a patch to 
> change it otherwise.

No problem, just check in the patch before because of urgent fix. I will
file another patch to modify this.

Thanks,
Ray

> 
> >   	if (!info)
> >   		goto create_fail;
> >   
> > @@ -1545,7 +1545,7 @@ static bool dc_link_construct(struct dc_link *link,
> >   	}
> >   
> >   	if (bios->integrated_info)
> > -		memcpy(info, bios->integrated_info, sizeof(*info));
> > +		memcpy(info, bios->integrated_info, sizeof(struct integrated_info));
> 
> This can then also stay as it is.
> 
> Apart from that good catch.
> 
> Regards,
> Christian.
> 
> >   
> >   	/* Look for channel mapping corresponding to connector and device tag */
> >   	for (i = 0; i < MAX_NUMBER_OF_EXT_DISPLAY_PATH; i++) {
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: fix the system memory page fault because of copy overflow

[Huang Rui]

On Fri, Jan 15, 2021 at 07:26:23PM +0800, Chen, Jiansong (Simon) wrote:
> [AMD Public Use]
> 
> Hi Rui,
> Seems the change has violated the kernel coding style😊, please help check.
> https://www.kernel.org/doc/html/latest/process/coding-style.html
> 
> Allocating memory
> ......
> The preferred form for passing a size of a struct is the following:
> 
> p = kmalloc(sizeof(*p), ...);
> The alternative form where struct name is spelled out hurts readability and introduces an opportunity for a bug when the pointer variable type is changed but the corresponding sizeof that is passed to a memory allocator is not.

Yes, I see. Thanks.

Ray

> 
> Regards,
> Jiansong
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Huang Rui
> Sent: Saturday, January 16, 2021 2:47 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Su, Jinzhou (Joe) <Jinzhou.Su@amd.com>; Huang, Ray <Ray.Huang@amd.com>; Lee Jones <lee.jones@linaro.org>; Zhu, Changfeng <Changfeng.Zhu@amd.com>
> Subject: [PATCH] drm/amd/display: fix the system memory page fault because of copy overflow
> 
> The buffer is allocated with the size of pointer and copy with the size of data structure. Then trigger the system memory page fault. Use the orignal data structure to get the object size.
> 
> Fixes: a8e30005b drm/amd/display/dc/core/dc_link: Move some local data from the stack to the heap
> 
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/gpu/drm/amd/display/dc/core/dc_link.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> index 69573d67056d..73178978ae74 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> @@ -1380,7 +1380,7 @@ static bool dc_link_construct(struct dc_link *link,
> 
>  DC_LOGGER_INIT(dc_ctx->logger);
> 
> -info = kzalloc(sizeof(info), GFP_KERNEL);
> +info = kzalloc(sizeof(struct integrated_info), GFP_KERNEL);
>  if (!info)
>  goto create_fail;
> 
> @@ -1545,7 +1545,7 @@ static bool dc_link_construct(struct dc_link *link,
>  }
> 
>  if (bios->integrated_info)
> -memcpy(info, bios->integrated_info, sizeof(*info));
> +memcpy(info, bios->integrated_info, sizeof(struct integrated_info));
> 
>  /* Look for channel mapping corresponding to connector and device tag */
>  for (i = 0; i < MAX_NUMBER_OF_EXT_DISPLAY_PATH; i++) {
> --
> 2.25.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7CJiansong.Chen%40amd.com%7Caa1f0e0196584ac4145208d8b942ff50%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637463044695608478%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OhlBMm03tg0JUctjpEtO88hL1Dnu5wxt7Keuojm61NQ%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx