From: "Deucher, Alexander" <Alexander.Deucher@amd.com>
To: "'Colin Ian King'" <colin.king@canonical.com>,
        "Koenig, Christian" <Christian.Koenig@amd.com>,
        David Airlie <airlied@linux.ie>, "Zhu, Rex" <Rex.Zhu@amd.com>,
        "StDenis, Tom" <Tom.StDenis@amd.com>, "Huang, Ray" <Ray.Huang@amd.com>,
        =?utf-8?B?TmlscyBXYWxsbcOpbml1cw==?= <nils.wallmenius@gmail.com>,
        Baoyou Xie <baoyou.xie@linaro.org>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] drm/amd/powerplay: check if table_info is NULL before
 dereferencing it
Thread-Topic: [PATCH] drm/amd/powerplay: check if table_info is NULL before
 dereferencing it
Thread-Index: AQHSPz9ynHmEUylN6k+BUa683WLSJ6DaLXJAgAAJRYCAAAJaoA==
Date: Tue, 15 Nov 2016 16:18:53 +0000
Message-ID: <MWHPR12MB1694EB0A2913CAC4ACAD92F2F7BF0@MWHPR12MB1694.namprd12.prod.outlook.com>
References: <20161115125435.6236-1-colin.king@canonical.com>
 <MWHPR12MB1694B6B916035A508D68F522F7BF0@MWHPR12MB1694.namprd12.prod.outlook.com>
 <3adcaa1f-6ded-0a56-2f44-c830e5a2d198@canonical.com>
In-Reply-To: <3adcaa1f-6ded-0a56-2f44-c830e5a2d198@canonical.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Nov 2016 16:18:53.5062
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR12MB1309
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id uAFGJBms029276
Content-Length: 4165
Lines: 115

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