From: "Deucher, Alexander" <Alexander.Deucher@amd.com>
To: "'Colin 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+BUa683WLSJ6DaLXJA
Date: Tue, 15 Nov 2016 15:49:44 +0000
Message-ID: <MWHPR12MB1694B6B916035A508D68F522F7BF0@MWHPR12MB1694.namprd12.prod.outlook.com>
References: <20161115125435.6236-1-colin.king@canonical.com>
In-Reply-To: <20161115125435.6236-1-colin.king@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 15:49:44.5930
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR12MB1341
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 uAFG5Vqn029190
Content-Length: 2832
Lines: 76

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

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