On 15/11/16 15:49, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: Colin King [mailto:colin.king at 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 at lists.freedesktop.org
>> Cc: linux-kernel at vger.kernel.org
>> Subject: [PATCH] drm/amd/powerplay: check if table_info is NULL before
>> dereferencing it
>>
>> From: Colin Ian King <colin.king at 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 at 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
>
