> -----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 > > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
