Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <c55e9866-83f6-a3a0-2ad3-40090e978b40@amd.com>
Date:   Tue, 5 Apr 2022 13:00:42 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH] drm/amdkfd: Fix potential NULL pointer dereference
Content-Language: en-US
To:     Grigory Vasilyev <h0tc0d3@gmail.com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>,
        Melissa Wen <mwen@igalia.com>
Cc:     "Pan, Xinhui" <Xinhui.Pan@amd.com>, linux-kernel@vger.kernel.org,
        amd-gfx@lists.freedesktop.org, David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org,
        Alex Deucher <alexander.deucher@amd.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
References: <20220404222132.12740-1-h0tc0d3@gmail.com>
From:   Felix Kuehling <felix.kuehling@amd.com>
In-Reply-To: <20220404222132.12740-1-h0tc0d3@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RUIyRXB5dzNTK2loOHA5ZEJoVWNYQi8xQ0YrZ0Jwc0hDZG03U3llSERTVnp4?=
 =?utf-8?B?Y1ZzUmhHVENUcHJsdEovaWZIL3paTDhCdEZwa0dQTXl2YWVQd3c3UE5iQnNk?=
 =?utf-8?B?Zm42bUVvd3lieERxdW5yU00zVUNydXJDd0xSV0dKWVVrNTNtbk9PZFM5cXhX?=
 =?utf-8?B?anEyOFhxcS9NQzI3NFlNK2lQeGRFektQdFNPWGJqZW1yNUxqVDZTOHpodW4w?=
 =?utf-8?B?cHhDRi9HQjkyT1lHR0ZNSkZNSU5ZT3pTSUc4RkpWSGQzVlRoY0pCODkxaDV0?=
 =?utf-8?B?YWdVeGZ4ZG1DdnIrekdDbWlaWFE4OER3RDhlNmg2RlJHc2VXMVBpZzNUdTQw?=
 =?utf-8?B?RVByR0NLdjZFSDlXdno1aEgwYUJmZzk5aWNkZEp0M2hHTmZhNU1WYXhacHJn?=
 =?utf-8?B?blgzRjg4UExReWlveUo0THdqeVZVRnYwUkJ1OU9QU0VpVVZ1SHFlUmVHOFhL?=
 =?utf-8?B?Tm0vdUtWSnk0MGNCRGp3RnorVGtsdnJ6WmhoaS9UNmRCUi9lZ0JjeHZhbU1v?=
 =?utf-8?B?NVJ5bWZUWTJkelg2ZEFCc3k2UEgxaWhhRlFwWE1tMCt1ZSs2MmVTT2FNZ25t?=
 =?utf-8?B?d0N6OVMyZHJLSFBlOHZBZEJrWTlrZlBQMmd2dzA3MXJ0aVhwbFBvNWNhVmUy?=
 =?utf-8?B?T1N6eEhQM0FVcWtKdmk4ek0xN21UcEROSWRiR1I0d21uZ2x5YUZvc2Q5dmVp?=
 =?utf-8?B?cm0vVHhaMUcwVGZRM3JlVDZUaGN3RWRYRFJZK0p4ZkQ0WmtOa0FHQWxybVlT?=
 =?utf-8?B?OXJpS1o2T1NJYkh5dHBPZjM0UUU2R3libStUbDBicGpJc08rc1hWaUc0Yk01?=
 =?utf-8?B?ZXdhSThYZGl3SjVXdk9URHd3Z2JTNDBidVZDQ1hjajJaM0MyMkpGcHZERXlO?=
 =?utf-8?B?b24zampsa1pTNm1iV3J5dmVjLzJTeG9YOW9XUVpuZ2pibGRuSllDVDNoU2J2?=
 =?utf-8?B?SXFtQUhwb0l4SnJsNGhTUVNRYVVneEhibEZ2UHBYb0lRTVN3Ni9pb1BiT2ZI?=
 =?utf-8?B?M2dSMkFJU3l6Z2JpUmltcFQyNm53QVk4bkVhTVg0RjhxSkJweXozU2c5dkEv?=
 =?utf-8?B?Skp2OFExUldLcXkrN1FwTUIvbGJYalR6Vko3VnljSVJuY2FtV3AxR1lLSWVK?=
 =?utf-8?B?UnZhOVNJK2VVaHphMVlJS3JLMGF1ZURhR1Q4M0lvYW0vbVo3Uk9rMlJoR1VI?=
 =?utf-8?B?Q0tmWlk5dytjMnJwa0xIOHFMbVd5L1FORTRlblQ1bEJBOHBNR0Q2N01uVkps?=
 =?utf-8?B?YTFRVjE0VTk2U0pLZHJmZWp6UHRPbXcwUG5HalQvdi9iZUFZa1VqYXpzWjI2?=
 =?utf-8?B?TFhETGlNbDRmbW84YmNnUHBQY2w5TDcrTFQxcUk4dldXRDhLUVVYRW9haDBq?=
 =?utf-8?B?Vi9SNmNLeXA2bjgyNmwvUUQ0U0ZaUjBCRFNHNnE4bzlOeWRaejUrblEzUmRl?=
 =?utf-8?B?anpMTzN1alltOVNXNUZOM0Q4TGhDYS9DVUZuYS9QbTc4ODFFQXpOekFRemhD?=
 =?utf-8?B?T1dWRmlvK0JqVmxNTDBhbENqR2pzY0I3a3VXT2orKzVqY2o0Wk9TODJyeE5P?=
 =?utf-8?B?YjZvVzJCdlZveVMvYTZZS2p0V1IxY1Zja29Dbit4MEs4dUV4bHErMi94WndJ?=
 =?utf-8?B?NVIvNEhjd1RueTFMVElxUVN0WE9xNktsMHZFZXhRZUhZcnpmMHVBS2FYSXBi?=
 =?utf-8?B?MHp1VWg5eTkrbUhwRTRLUDJoVkx0K3RONTdDT1FFUFIwbjZpS0V4NkYyUnpy?=
 =?utf-8?B?M1Q0K3FINzgvN3BsNmplYjVlayszYzRGMzJYZ1Fjc3d2RVNxN3RaQkdzZlFY?=
 =?utf-8?B?czZuWmdCMnNBS0JycVNLMFVoL1hGa3VFdUtJUFJLNTZjUEJvNFFGNGtscXRE?=
 =?utf-8?B?NC9YMWljVUJUTmJqVjdQbUd6enlaeUlhU2dudXRIRThGTkJoYWI0UjZIcWdN?=
 =?utf-8?B?RmN1YW8ycFRUUGtwYm9SNnZ5UTlBQ21ObkJ4eFQxV20vV2lTVWdELzdHcUQ4?=
 =?utf-8?B?eHhKcVBtekpkTWJKemNqRmRNRzEzVWVvS3JuSWIxRGFTczQzWkxxa09YREpa?=
 =?utf-8?B?MXpwQ1YwTDBqcTZEb3o4c1NXMDdSNWl6NXVCNmxKVlJaTlVVektlT2xYL3E5?=
 =?utf-8?B?eFI0NGZobUlPUVJUcXdzdmdMODJ3Vjlqc3NvNGdYWGxFNTR2ZmwvMFdiZzdJ?=
 =?utf-8?B?VU55dGFkWVRVeXVWQ3pEUUovTlduSTRrSDR4UDdDcy9XMjFzQ0huTy9GazEz?=
 =?utf-8?B?WGZGQnRIUFR4akRJQWJnWHl3amFQV2FJUyttV0phY0dONEhycUtvVVY5RFhU?=
 =?utf-8?B?UDlzanVycTJQME52OVV2SitPbU9MUmRqYlFlRFVNeFhyMDlGZ0JtZz09?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e58d55df-24a7-477a-d4f9-08da1725d2dc
X-MS-Exchange-CrossTenant-AuthSource: BN9PR12MB5115.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Apr 2022 17:00:44.7606
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 9cSFjzYDu1rICe6cV4W256XyAHzXOktmnWKAirf4l0dzwAiJWbGaYYlUv9qi/nmE/ATXqmjPL9hz+Ne+iu1ONg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR12MB5474
X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 2022-04-04 um 18:21 schrieb Grigory Vasilyev:
> In the amdgpu_amdkfd_get_xgmi_bandwidth_mbytes function,
> the peer_adev pointer can be NULL and is passed to amdgpu_xgmi_get_num_links.
>
> In amdgpu_xgmi_get_num_links, peer_adev pointer is dereferenced
> without any checks: peer_adev->gmc.xgmi.node_id .

What's worse, peer_adev is uninitialized with an undefined value if src 
is NULL. So that code was definitely bogus.

However, I think your patch will result in incorrect results. Currently 
amdgpu_amdkfd_get_xgmi_bandwidth is always called with is_min=true if 
src==NULL, and with is_min=false if src!=NULL. The intention is, that we 
assume a single XGMI link in the case that src==NULL. That means the 
is_min parameter is redundant. What we should do instead is, assume that 
num_links==1 if src==NULL, and drop the is_min parameter.

That would keep things working the way they do now, and prevent 
potential problems in the future.

Regards,
   Felix


>
> Signed-off-by: Grigory Vasilyev <h0tc0d3@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index be1a55f8b8c5..1a1006b18016 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -541,11 +541,10 @@ int amdgpu_amdkfd_get_xgmi_bandwidth_mbytes(struct amdgpu_device *dst,
>   	struct amdgpu_device *adev = dst, *peer_adev;
>   	int num_links;
>   
> -	if (adev->asic_type != CHIP_ALDEBARAN)
> +	if (!src || adev->asic_type != CHIP_ALDEBARAN)
>   		return 0;
>   
> -	if (src)
> -		peer_adev = src;
> +	peer_adev = src;
>   
>   	/* num links returns 0 for indirect peers since indirect route is unknown. */
>   	num_links = is_min ? 1 : amdgpu_xgmi_get_num_links(adev, peer_adev);