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

From: Deucher, Alexander
Date: Tue Nov 15 2016 - 11:19:05 EST


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