All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amd/powerplay: check if table_info is NULL before dereferencing it
@ 2016-11-15 12:54 ` Colin King
  0 siblings, 0 replies; 7+ messages in thread
From: Colin King @ 2016-11-15 12:54 UTC (permalink / raw)
  To: Alex Deucher, Christian König, David Airlie, Rex Zhu,
	Tom St Denis, Huang Rui, Nils Wallménius, Baoyou Xie,
	dri-devel
  Cc: linux-kernel

From: Colin Ian King <colin.king@canonical.com>

table_info is being dereferenced before a null check, which implies
a potential null pointer deference error.  Fix this by moving the null
check of table_info to the start of smu7_get_evv_voltages to avoid
potential null pointer deferencing.

Found with static analysis by CoverityScan, CID 1377752

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
index 28e748d..6798067 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
@@ -1473,6 +1473,8 @@ static int smu7_get_evv_voltages(struct pp_hwmgr *hwmgr)
 			(struct phm_ppt_v1_information *)hwmgr->pptable;
 	struct phm_ppt_v1_clock_voltage_dependency_table *sclk_table = NULL;
 
+	if (table_info == NULL)
+		return -EINVAL;
 
 	for (i = 0; i < SMU7_MAX_LEAKAGE_COUNT; i++) {
 		vv_id = ATOM_VIRTUAL_VOLTAGE_ID0 + i;
@@ -1483,8 +1485,6 @@ static int smu7_get_evv_voltages(struct pp_hwmgr *hwmgr)
 						table_info->vddgfx_lookup_table, vv_id, &sclk)) {
 				if (phm_cap_enabled(hwmgr->platform_descriptor.platformCaps,
 							PHM_PlatformCaps_ClockStretcher)) {
-					if (table_info == NULL)
-						return -EINVAL;
 					sclk_table = table_info->vdd_dep_on_sclk;
 
 					for (j = 1; j < sclk_table->count; j++) {
@@ -1517,8 +1517,6 @@ static int smu7_get_evv_voltages(struct pp_hwmgr *hwmgr)
 					table_info->vddc_lookup_table, vv_id, &sclk)) {
 				if (phm_cap_enabled(hwmgr->platform_descriptor.platformCaps,
 						PHM_PlatformCaps_ClockStretcher)) {
-					if (table_info == NULL)
-						return -EINVAL;
 					sclk_table = table_info->vdd_dep_on_sclk;
 
 					for (j = 1; j < sclk_table->count; j++) {
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH] drm/amd/powerplay: check if table_info is NULL before dereferencing it
@ 2016-11-15 12:54 ` Colin King
  0 siblings, 0 replies; 7+ messages in thread
From: Colin King @ 2016-11-15 12:54 UTC (permalink / raw)
  To: Alex Deucher, Christian König, David Airlie, Rex Zhu,
	Tom St Denis, Huang Rui, Nils Wallménius, Baoyou Xie,
	dri-devel
  Cc: linux-kernel

From: Colin Ian King <colin.king@canonical.com>

table_info is being dereferenced before a null check, which implies
a potential null pointer deference error.  Fix this by moving the null
check of table_info to the start of smu7_get_evv_voltages to avoid
potential null pointer deferencing.

Found with static analysis by CoverityScan, CID 1377752

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
index 28e748d..6798067 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
@@ -1473,6 +1473,8 @@ static int smu7_get_evv_voltages(struct pp_hwmgr *hwmgr)
 			(struct phm_ppt_v1_information *)hwmgr->pptable;
 	struct phm_ppt_v1_clock_voltage_dependency_table *sclk_table = NULL;
 
+	if (table_info == NULL)
+		return -EINVAL;
 
 	for (i = 0; i < SMU7_MAX_LEAKAGE_COUNT; i++) {
 		vv_id = ATOM_VIRTUAL_VOLTAGE_ID0 + i;
@@ -1483,8 +1485,6 @@ static int smu7_get_evv_voltages(struct pp_hwmgr *hwmgr)
 						table_info->vddgfx_lookup_table, vv_id, &sclk)) {
 				if (phm_cap_enabled(hwmgr->platform_descriptor.platformCaps,
 							PHM_PlatformCaps_ClockStretcher)) {
-					if (table_info == NULL)
-						return -EINVAL;
 					sclk_table = table_info->vdd_dep_on_sclk;
 
 					for (j = 1; j < sclk_table->count; j++) {
@@ -1517,8 +1517,6 @@ static int smu7_get_evv_voltages(struct pp_hwmgr *hwmgr)
 					table_info->vddc_lookup_table, vv_id, &sclk)) {
 				if (phm_cap_enabled(hwmgr->platform_descriptor.platformCaps,
 						PHM_PlatformCaps_ClockStretcher)) {
-					if (table_info == NULL)
-						return -EINVAL;
 					sclk_table = table_info->vdd_dep_on_sclk;
 
 					for (j = 1; j < sclk_table->count; j++) {
-- 
2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* RE: [PATCH] drm/amd/powerplay: check if table_info is NULL before dereferencing it
  2016-11-15 12:54 ` Colin King
@ 2016-11-15 15:49   ` Deucher, Alexander
  -1 siblings, 0 replies; 7+ messages in thread
From: Deucher, Alexander @ 2016-11-15 15:49 UTC (permalink / raw)
  To: 'Colin King',
	Koenig, Christian, David Airlie, Zhu, Rex, StDenis, Tom, Huang,
	Ray, Nils Wallménius, Baoyou Xie, dri-devel
  Cc: linux-kernel

> -----Original Message-----
> From: Colin King [mailto:colin.king@canonical.com]
> Sent: Tuesday, November 15, 2016 7:55 AM
> To: Deucher, Alexander; Koenig, Christian; David Airlie; Zhu, Rex; StDenis,
> Tom; Huang, Ray; Nils Wallménius; Baoyou Xie; dri-
> devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Subject: [PATCH] drm/amd/powerplay: check if table_info is NULL before
> dereferencing it
> 
> From: Colin Ian King <colin.king@canonical.com>
> 
> table_info is being dereferenced before a null check, which implies
> a potential null pointer deference error.  Fix this by moving the null
> check of table_info to the start of smu7_get_evv_voltages to avoid
> potential null pointer deferencing.
> 
> Found with static analysis by CoverityScan, CID 1377752
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

NACK, this effectively reverts the patch.  This causes the function to exit early on asics where the table it NULL which is not what we want.  Whether the table exists or not is dependent on the table version and the feature caps for the chip which are checked before the table is dereferenced.  The NULL check in the top if clause is not strictly necessary and could be removed.

Alex

> ---
>  drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> index 28e748d..6798067 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> @@ -1473,6 +1473,8 @@ static int smu7_get_evv_voltages(struct pp_hwmgr
> *hwmgr)
>  			(struct phm_ppt_v1_information *)hwmgr->pptable;
>  	struct phm_ppt_v1_clock_voltage_dependency_table *sclk_table =
> NULL;
> 
> +	if (table_info == NULL)
> +		return -EINVAL;
> 
>  	for (i = 0; i < SMU7_MAX_LEAKAGE_COUNT; i++) {
>  		vv_id = ATOM_VIRTUAL_VOLTAGE_ID0 + i;
> @@ -1483,8 +1485,6 @@ static int smu7_get_evv_voltages(struct pp_hwmgr
> *hwmgr)
>  						table_info-
> >vddgfx_lookup_table, vv_id, &sclk)) {
>  				if (phm_cap_enabled(hwmgr-
> >platform_descriptor.platformCaps,
> 
> 	PHM_PlatformCaps_ClockStretcher)) {
> -					if (table_info == NULL)
> -						return -EINVAL;
>  					sclk_table = table_info-
> >vdd_dep_on_sclk;
> 
>  					for (j = 1; j < sclk_table->count; j++) {
> @@ -1517,8 +1517,6 @@ static int smu7_get_evv_voltages(struct pp_hwmgr
> *hwmgr)
>  					table_info->vddc_lookup_table,
> vv_id, &sclk)) {
>  				if (phm_cap_enabled(hwmgr-
> >platform_descriptor.platformCaps,
> 
> 	PHM_PlatformCaps_ClockStretcher)) {
> -					if (table_info == NULL)
> -						return -EINVAL;
>  					sclk_table = table_info-
> >vdd_dep_on_sclk;
> 
>  					for (j = 1; j < sclk_table->count; j++) {
> --
> 2.10.2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] drm/amd/powerplay: check if table_info is NULL before dereferencing it
@ 2016-11-15 15:49   ` Deucher, Alexander
  0 siblings, 0 replies; 7+ messages in thread
From: Deucher, Alexander @ 2016-11-15 15:49 UTC (permalink / raw)
  To: 'Colin King',
	Koenig, Christian, David Airlie, Zhu, Rex, StDenis, Tom, Huang,
	Ray, Nils Wallménius, Baoyou Xie, dri-devel
  Cc: linux-kernel

> -----Original Message-----
> From: Colin King [mailto:colin.king@canonical.com]
> Sent: Tuesday, November 15, 2016 7:55 AM
> To: Deucher, Alexander; Koenig, Christian; David Airlie; Zhu, Rex; StDenis,
> Tom; Huang, Ray; Nils Wallménius; Baoyou Xie; dri-
> devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Subject: [PATCH] drm/amd/powerplay: check if table_info is NULL before
> dereferencing it
> 
> From: Colin Ian King <colin.king@canonical.com>
> 
> table_info is being dereferenced before a null check, which implies
> a potential null pointer deference error.  Fix this by moving the null
> check of table_info to the start of smu7_get_evv_voltages to avoid
> potential null pointer deferencing.
> 
> Found with static analysis by CoverityScan, CID 1377752
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

NACK, this effectively reverts the patch.  This causes the function to exit early on asics where the table it NULL which is not what we want.  Whether the table exists or not is dependent on the table version and the feature caps for the chip which are checked before the table is dereferenced.  The NULL check in the top if clause is not strictly necessary and could be removed.

Alex

> ---
>  drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> index 28e748d..6798067 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> @@ -1473,6 +1473,8 @@ static int smu7_get_evv_voltages(struct pp_hwmgr
> *hwmgr)
>  			(struct phm_ppt_v1_information *)hwmgr->pptable;
>  	struct phm_ppt_v1_clock_voltage_dependency_table *sclk_table =
> NULL;
> 
> +	if (table_info == NULL)
> +		return -EINVAL;
> 
>  	for (i = 0; i < SMU7_MAX_LEAKAGE_COUNT; i++) {
>  		vv_id = ATOM_VIRTUAL_VOLTAGE_ID0 + i;
> @@ -1483,8 +1485,6 @@ static int smu7_get_evv_voltages(struct pp_hwmgr
> *hwmgr)
>  						table_info-
> >vddgfx_lookup_table, vv_id, &sclk)) {
>  				if (phm_cap_enabled(hwmgr-
> >platform_descriptor.platformCaps,
> 
> 	PHM_PlatformCaps_ClockStretcher)) {
> -					if (table_info == NULL)
> -						return -EINVAL;
>  					sclk_table = table_info-
> >vdd_dep_on_sclk;
> 
>  					for (j = 1; j < sclk_table->count; j++) {
> @@ -1517,8 +1517,6 @@ static int smu7_get_evv_voltages(struct pp_hwmgr
> *hwmgr)
>  					table_info->vddc_lookup_table,
> vv_id, &sclk)) {
>  				if (phm_cap_enabled(hwmgr-
> >platform_descriptor.platformCaps,
> 
> 	PHM_PlatformCaps_ClockStretcher)) {
> -					if (table_info == NULL)
> -						return -EINVAL;
>  					sclk_table = table_info-
> >vdd_dep_on_sclk;
> 
>  					for (j = 1; j < sclk_table->count; j++) {
> --
> 2.10.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/amd/powerplay: check if table_info is NULL before dereferencing it
  2016-11-15 15:49   ` Deucher, Alexander
@ 2016-11-15 16:09     ` Colin Ian King
  -1 siblings, 0 replies; 7+ messages in thread
From: Colin Ian King @ 2016-11-15 16:09 UTC (permalink / raw)
  To: Deucher, Alexander, Koenig, Christian, David Airlie, Zhu, Rex,
	StDenis, Tom, Huang, Ray, Nils Wallménius, Baoyou Xie,
	dri-devel
  Cc: linux-kernel

On 15/11/16 15:49, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: Colin King [mailto:colin.king@canonical.com]
>> Sent: Tuesday, November 15, 2016 7:55 AM
>> To: Deucher, Alexander; Koenig, Christian; David Airlie; Zhu, Rex; StDenis,
>> Tom; Huang, Ray; Nils Wallménius; Baoyou Xie; dri-
>> devel@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
>> Subject: [PATCH] drm/amd/powerplay: check if table_info is NULL before
>> dereferencing it
>>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> table_info is being dereferenced before a null check, which implies
>> a potential null pointer deference error.  Fix this by moving the null
>> check of table_info to the start of smu7_get_evv_voltages to avoid
>> potential null pointer deferencing.
>>
>> Found with static analysis by CoverityScan, CID 1377752
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> 
> NACK, this effectively reverts the patch.  This causes the function to exit early on asics where the table it NULL which is not what we want.  Whether the table exists or not is dependent on the table version and the feature caps for the chip which are checked before the table is dereferenced.  The NULL check in the top if clause is not strictly necessary and could be removed.
> 
> Alex

OK, understood.  The part I'm missing is that we dereference table_info
at the following statement:

if ((hwmgr->pp_table_version == PP_TABLE_V1)
    && !phm_get_sclk_for_voltage_evv(hwmgr,
                        table_info->vddgfx_lookup_table, vv_id, &sclk))

and later check if it is NULL. So, I can't see where table_info is being
set other than the start of the function, so, either it can be null and
hence we have a null ptr deference, or it's never null, in which case
the check for NULL is redundant.

Colin
> 
>> ---
>>  drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> index 28e748d..6798067 100644
>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> @@ -1473,6 +1473,8 @@ static int smu7_get_evv_voltages(struct pp_hwmgr 
>> *hwmgr)
>>  			(struct phm_ppt_v1_information *)hwmgr->pptable;
>>  	struct phm_ppt_v1_clock_voltage_dependency_table *sclk_table =
>> NULL;
>>
>> +	if (table_info == NULL)
>> +		return -EINVAL;
>>
>>  	for (i = 0; i < SMU7_MAX_LEAKAGE_COUNT; i++) {
>>  		vv_id = ATOM_VIRTUAL_VOLTAGE_ID0 + i;
>> @@ -1483,8 +1485,6 @@ static int smu7_get_evv_voltages(struct pp_hwmgr
>> *hwmgr)
>>  						table_info-
>>> vddgfx_lookup_table, vv_id, &sclk)) {
>>  				if (phm_cap_enabled(hwmgr-
>>> platform_descriptor.platformCaps,
>>
>> 	PHM_PlatformCaps_ClockStretcher)) {
>> -					if (table_info == NULL)
>> -						return -EINVAL;
>>  					sclk_table = table_info-
>>> vdd_dep_on_sclk;
>>
>>  					for (j = 1; j < sclk_table->count; j++) {
>> @@ -1517,8 +1517,6 @@ static int smu7_get_evv_voltages(struct pp_hwmgr
>> *hwmgr)
>>  					table_info->vddc_lookup_table,
>> vv_id, &sclk)) {
>>  				if (phm_cap_enabled(hwmgr-
>>> platform_descriptor.platformCaps,
>>
>> 	PHM_PlatformCaps_ClockStretcher)) {
>> -					if (table_info == NULL)
>> -						return -EINVAL;
>>  					sclk_table = table_info-
>>> vdd_dep_on_sclk;
>>
>>  					for (j = 1; j < sclk_table->count; j++) {
>> --
>> 2.10.2
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/amd/powerplay: check if table_info is NULL before dereferencing it
@ 2016-11-15 16:09     ` Colin Ian King
  0 siblings, 0 replies; 7+ messages in thread
From: Colin Ian King @ 2016-11-15 16:09 UTC (permalink / raw)
  To: Deucher, Alexander, Koenig, Christian, David Airlie, Zhu, Rex,
	StDenis, Tom, Huang, Ray, Nils Wallménius, Baoyou Xie,
	dri-devel
  Cc: linux-kernel

On 15/11/16 15:49, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: Colin King [mailto:colin.king@canonical.com]
>> Sent: Tuesday, November 15, 2016 7:55 AM
>> To: Deucher, Alexander; Koenig, Christian; David Airlie; Zhu, Rex; StDenis,
>> Tom; Huang, Ray; Nils Wallménius; Baoyou Xie; dri-
>> devel@lists.freedesktop.org
>> Cc: linux-kernel@vger.kernel.org
>> Subject: [PATCH] drm/amd/powerplay: check if table_info is NULL before
>> dereferencing it
>>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> table_info is being dereferenced before a null check, which implies
>> a potential null pointer deference error.  Fix this by moving the null
>> check of table_info to the start of smu7_get_evv_voltages to avoid
>> potential null pointer deferencing.
>>
>> Found with static analysis by CoverityScan, CID 1377752
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> 
> NACK, this effectively reverts the patch.  This causes the function to exit early on asics where the table it NULL which is not what we want.  Whether the table exists or not is dependent on the table version and the feature caps for the chip which are checked before the table is dereferenced.  The NULL check in the top if clause is not strictly necessary and could be removed.
> 
> Alex

OK, understood.  The part I'm missing is that we dereference table_info
at the following statement:

if ((hwmgr->pp_table_version == PP_TABLE_V1)
    && !phm_get_sclk_for_voltage_evv(hwmgr,
                        table_info->vddgfx_lookup_table, vv_id, &sclk))

and later check if it is NULL. So, I can't see where table_info is being
set other than the start of the function, so, either it can be null and
hence we have a null ptr deference, or it's never null, in which case
the check for NULL is redundant.

Colin
> 
>> ---
>>  drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> index 28e748d..6798067 100644
>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> @@ -1473,6 +1473,8 @@ static int smu7_get_evv_voltages(struct pp_hwmgr 
>> *hwmgr)
>>  			(struct phm_ppt_v1_information *)hwmgr->pptable;
>>  	struct phm_ppt_v1_clock_voltage_dependency_table *sclk_table =
>> NULL;
>>
>> +	if (table_info == NULL)
>> +		return -EINVAL;
>>
>>  	for (i = 0; i < SMU7_MAX_LEAKAGE_COUNT; i++) {
>>  		vv_id = ATOM_VIRTUAL_VOLTAGE_ID0 + i;
>> @@ -1483,8 +1485,6 @@ static int smu7_get_evv_voltages(struct pp_hwmgr
>> *hwmgr)
>>  						table_info-
>>> vddgfx_lookup_table, vv_id, &sclk)) {
>>  				if (phm_cap_enabled(hwmgr-
>>> platform_descriptor.platformCaps,
>>
>> 	PHM_PlatformCaps_ClockStretcher)) {
>> -					if (table_info == NULL)
>> -						return -EINVAL;
>>  					sclk_table = table_info-
>>> vdd_dep_on_sclk;
>>
>>  					for (j = 1; j < sclk_table->count; j++) {
>> @@ -1517,8 +1517,6 @@ static int smu7_get_evv_voltages(struct pp_hwmgr
>> *hwmgr)
>>  					table_info->vddc_lookup_table,
>> vv_id, &sclk)) {
>>  				if (phm_cap_enabled(hwmgr-
>>> platform_descriptor.platformCaps,
>>
>> 	PHM_PlatformCaps_ClockStretcher)) {
>> -					if (table_info == NULL)
>> -						return -EINVAL;
>>  					sclk_table = table_info-
>>> vdd_dep_on_sclk;
>>
>>  					for (j = 1; j < sclk_table->count; j++) {
>> --
>> 2.10.2
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] drm/amd/powerplay: check if table_info is NULL before dereferencing it
  2016-11-15 16:09     ` Colin Ian King
  (?)
@ 2016-11-15 16:18     ` Deucher, Alexander
  -1 siblings, 0 replies; 7+ messages in thread
From: Deucher, Alexander @ 2016-11-15 16:18 UTC (permalink / raw)
  To: 'Colin Ian King',
	Koenig, Christian, David Airlie, Zhu, Rex, StDenis, Tom, Huang,
	Ray, Nils Wallménius, Baoyou Xie, dri-devel
  Cc: linux-kernel

> -----Original Message-----
> From: Colin Ian King [mailto:colin.king@canonical.com]
> Sent: Tuesday, November 15, 2016 11:09 AM
> To: Deucher, Alexander; Koenig, Christian; David Airlie; Zhu, Rex; StDenis,
> Tom; Huang, Ray; Nils Wallménius; Baoyou Xie; dri-
> devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] drm/amd/powerplay: check if table_info is NULL before
> dereferencing it
> 
> On 15/11/16 15:49, Deucher, Alexander wrote:
> >> -----Original Message-----
> >> From: Colin King [mailto:colin.king@canonical.com]
> >> Sent: Tuesday, November 15, 2016 7:55 AM
> >> To: Deucher, Alexander; Koenig, Christian; David Airlie; Zhu, Rex; StDenis,
> >> Tom; Huang, Ray; Nils Wallménius; Baoyou Xie; dri-
> >> devel@lists.freedesktop.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Subject: [PATCH] drm/amd/powerplay: check if table_info is NULL before
> >> dereferencing it
> >>
> >> From: Colin Ian King <colin.king@canonical.com>
> >>
> >> table_info is being dereferenced before a null check, which implies
> >> a potential null pointer deference error.  Fix this by moving the null
> >> check of table_info to the start of smu7_get_evv_voltages to avoid
> >> potential null pointer deferencing.
> >>
> >> Found with static analysis by CoverityScan, CID 1377752
> >>
> >> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >
> > NACK, this effectively reverts the patch.  This causes the function to exit
> early on asics where the table it NULL which is not what we want.  Whether
> the table exists or not is dependent on the table version and the feature
> caps for the chip which are checked before the table is dereferenced.  The
> NULL check in the top if clause is not strictly necessary and could be
> removed.
> >
> > Alex
> 
> OK, understood.  The part I'm missing is that we dereference table_info
> at the following statement:
> 
> if ((hwmgr->pp_table_version == PP_TABLE_V1)
>     && !phm_get_sclk_for_voltage_evv(hwmgr,
>                         table_info->vddgfx_lookup_table, vv_id, &sclk))
> 
> and later check if it is NULL. So, I can't see where table_info is being
> set other than the start of the function, so, either it can be null and
> hence we have a null ptr deference, or it's never null, in which case
> the check for NULL is redundant.

Yes, that check is redundant.  That is was I was referring to above.

Alex

> 
> Colin
> >
> >> ---
> >>  drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> >> b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> >> index 28e748d..6798067 100644
> >> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> >> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> >> @@ -1473,6 +1473,8 @@ static int smu7_get_evv_voltages(struct
> pp_hwmgr
> >> *hwmgr)
> >>  			(struct phm_ppt_v1_information *)hwmgr->pptable;
> >>  	struct phm_ppt_v1_clock_voltage_dependency_table *sclk_table =
> >> NULL;
> >>
> >> +	if (table_info == NULL)
> >> +		return -EINVAL;
> >>
> >>  	for (i = 0; i < SMU7_MAX_LEAKAGE_COUNT; i++) {
> >>  		vv_id = ATOM_VIRTUAL_VOLTAGE_ID0 + i;
> >> @@ -1483,8 +1485,6 @@ static int smu7_get_evv_voltages(struct
> pp_hwmgr
> >> *hwmgr)
> >>  						table_info-
> >>> vddgfx_lookup_table, vv_id, &sclk)) {
> >>  				if (phm_cap_enabled(hwmgr-
> >>> platform_descriptor.platformCaps,
> >>
> >> 	PHM_PlatformCaps_ClockStretcher)) {
> >> -					if (table_info == NULL)
> >> -						return -EINVAL;
> >>  					sclk_table = table_info-
> >>> vdd_dep_on_sclk;
> >>
> >>  					for (j = 1; j < sclk_table->count; j++) {
> >> @@ -1517,8 +1517,6 @@ static int smu7_get_evv_voltages(struct
> pp_hwmgr
> >> *hwmgr)
> >>  					table_info->vddc_lookup_table,
> >> vv_id, &sclk)) {
> >>  				if (phm_cap_enabled(hwmgr-
> >>> platform_descriptor.platformCaps,
> >>
> >> 	PHM_PlatformCaps_ClockStretcher)) {
> >> -					if (table_info == NULL)
> >> -						return -EINVAL;
> >>  					sclk_table = table_info-
> >>> vdd_dep_on_sclk;
> >>
> >>  					for (j = 1; j < sclk_table->count; j++) {
> >> --
> >> 2.10.2
> >

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-11-15 16:19 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-15 12:54 [PATCH] drm/amd/powerplay: check if table_info is NULL before dereferencing it Colin King
2016-11-15 12:54 ` Colin King
2016-11-15 15:49 ` Deucher, Alexander
2016-11-15 15:49   ` Deucher, Alexander
2016-11-15 16:09   ` Colin Ian King
2016-11-15 16:09     ` Colin Ian King
2016-11-15 16:18     ` Deucher, Alexander

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.