RE: [PATCH] drm/amd/powerplay: check if table_info is NULL before dereferencing it

[Deucher, Alexander]

> -----Original Message-----
> From: Colin Ian King [mailto:colin.king@xxxxxxxxxxxxx]
> 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@xxxxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxx]
> >> 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@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx
> >> Subject: [PATCH] drm/amd/powerplay: check if table_info is NULL before
> >> dereferencing it
> >>
> >> From: Colin Ian King <colin.king@xxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxx>
> >
> > 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
> >