Message-ID: <64aef893-50d5-be6f-75c0-b9c8293fcbfe@amd.com>
Date:   Thu, 9 Mar 2023 12:49:31 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH] drm/amdkfd: fix potential kgd_mem UAFs
Content-Language: en-US
To:     Chia-I Wu <olvaffe@gmail.com>, dri-devel@lists.freedesktop.org
Cc:     Alex Deucher <alexander.deucher@amd.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, amd-gfx@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
References: <20230308213724.3396058-1-olvaffe@gmail.com>
From:   Felix Kuehling <felix.kuehling@amd.com>
In-Reply-To: <20230308213724.3396058-1-olvaffe@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?R0pKbHVFQytXUXFaTmpFNVhFZmZIdDVmUXZFYnA2bkowenMzWmJ3aTBra0Qz?=
 =?utf-8?B?U3R1QUxzOXpEbzFlbEY5N0dqY05FL2laMFhBVEFZcnp0c3p5RE9YR3lBRFpB?=
 =?utf-8?B?QTg3RXFkRG4yZ05ud1VzaEo5VkdxNGRHTGZXYnZmNlZ0Zm8va2NCZ0RjM0s3?=
 =?utf-8?B?U0l3NXo4MVdnWllObS8zOHVBempYejEyVTRDSk0vZmNIZWtmdVBJYSs3d3hi?=
 =?utf-8?B?YlJDaW44VHV2WWphWk04alBqT3RvM0JuNGFPYVd4WURqUzZCRmdVZTFhSUZk?=
 =?utf-8?B?VE1MdW82bjdndC83citvd2ZPemFLSDRtdURXTWVhb1NqQ1lkNTdZYlR3NzQy?=
 =?utf-8?B?UVgrNElPWW1KR3F3MVU2V00zclIvaWNlckl4UGx6Ly9vNnFTVVFxdGpNUklT?=
 =?utf-8?B?VXI3OVYzMFhvbWxQZEIyUzhBcWRZYXRhdGY3THNFb29HaGR6ejYrNHFGUWpQ?=
 =?utf-8?B?cXY2d3VmcnJ3RUVyUzlZZXVTdjVQSEtONUF0V3hRdUhuNU9Gb0Y0dytWUHFT?=
 =?utf-8?B?a09GNkZVNHhLemw1cnMyaW93NXk3dEREMEFEYmZVUmtZTE1WQzRMZHZlUE1p?=
 =?utf-8?B?UVo2dlNIQkgyM2w0NmQvSGcva3lHQTRHOHFJMnNMR2QyanFPUExnNVQwM2Zi?=
 =?utf-8?B?WHBJUm11OXV4ZGdFbnk1bVVUdWR6NUFSZ3NFdExCYVk0cWN5WS9teW5iR1R3?=
 =?utf-8?B?NExucms2L0prejc3My9EdTRjeXVCSmlMb0MyNThROXBxZENCVzFPdlJGTklX?=
 =?utf-8?B?cGFBcDNVaHBPMG4vYUJQOExZMWloOFBlRlYwMG5JNWtTenVyVU1uTitZR3Rh?=
 =?utf-8?B?UUhJbFluM3JnQzd3VlVXR29EZHVWanQ2ZGhiTVQ3TjVzMXBwZ2xhYXNTbmpV?=
 =?utf-8?B?VW1CN2liRmxFZkhNb0hZeitOcVk4L0tXRCs2dXRrc01LQ0puM3NDQmtJeHE4?=
 =?utf-8?B?c2w3QThaVWFZY1JHdWg1NFR1am1ZdmtwU3FUS3BxcWZuUWd3OCtKNnBPYjNU?=
 =?utf-8?B?ME1JNWJYa2V1SWtrMTNBalQ5L2EyVG4vYkFGcWtGVm8vSlVtaWd2d05pWTBI?=
 =?utf-8?B?U1RGK1N3WmFSaWxUancvb3d3djNubi9RdlN2N0ltS0x2ZGppSE9HYkM5elE0?=
 =?utf-8?B?ZUpWOXBMeWZuTkwrUE1DdVhEd3ppa3N4SjZpRGNvV3dxLzR3UzE4WWFnd3BD?=
 =?utf-8?B?amdIMWhYTDlWcTRlUTdNTTVkeGhoa29XS1RsL1A5ZWJjLzlMYnhiTFM1YURk?=
 =?utf-8?B?WDQ3aW84eUtXMEl1Y2hWa0dJUmc0bzc0djdLUlVMdVpLN3lySHFMLzlMZ3o5?=
 =?utf-8?B?aHU1OWhoOTB3WE5PUnNOUDViVlMvbmRHQWNSSGJWYVJWOElSQm5zTVZWa0to?=
 =?utf-8?B?bExzQzhrdDNFZ05JZUFIUTAxbSt6bCt5Q1dPRjZBOGc2Q0JGOHFTM2M3YUZx?=
 =?utf-8?B?SExOYkJvN09aZ0Q4UklZZ3ppd2EzT0dmaG12ay8zMHE2bTZLcVR4bzlBNmxS?=
 =?utf-8?B?Y0Q1NEVFbFRZTGJTcUhRTzJSUDg2aVcwZEdvVVJxYWhzUkFNeG8yNUswd01S?=
 =?utf-8?B?b25zSFFhMDNQSFNtTjhnOEhJYm1tRDdiN0VFZyszMXMwaDJmVW9ZRUVCZG1P?=
 =?utf-8?B?TUp6ekJUMjArM1ZwNDFDQm1FSm5pMnEzTWwzcm1IZXVkWEM0eWNmY081QlRa?=
 =?utf-8?B?ZUVRcHhQS0liQUdndG5xRmc2TkdOaDI1blpPRFN6L3VXa3dMM0orWHVEbGdN?=
 =?utf-8?B?WDZPSHRXQk4yTStZNXUyK3luVG9Ddk94c1NicHgzcFNucmtISFdueTJmaXBj?=
 =?utf-8?B?aVB0WFIxNHRVakpjU0NpS0FmSjVXQ3lxb0hZWWZyMDFDUUl0QkF6Vmo2MXgw?=
 =?utf-8?B?QmFoWkRkL1hNc0t3ajA4dVMrUmVPdDgxMTRzcFBITXdra3BWdUVXaGdvUE9F?=
 =?utf-8?B?MVlkVTJ1NGtOL09OaGJHVDYwNWpKblhLeFVpYS9wYUt6L1I0RHBjQmlRUHYz?=
 =?utf-8?B?NFhPa2RZYStGK1J4YzJiaVRnSy9lL2RtRXd5OXVVRU5yVXRFaEVpaCtZbG1h?=
 =?utf-8?B?d3NYbWlkRjRUUWhLWjRNS2hFU3lOYW9wbk5HRFhWL09YRU5pTVdneWt2THdR?=
 =?utf-8?Q?33yuVmdtqv0IJGAvABqBbNeyb?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 37ef6519-238b-46b3-7aa7-08db20c6a45c
X-MS-Exchange-CrossTenant-AuthSource: BN9PR12MB5115.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Mar 2023 17:49:33.7756
 (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: E+zTs4C5XS3jIbCB8y7U3blHAnghiSsubUJ1B9yCPl+QEBaqQ0RZuGASa/7fySUEW5h65e7jV8aiB19M0S3eGg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR12MB6203
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 2023-03-08 um 16:37 schrieb Chia-I Wu:
> kgd_mem should be accessed with p->mutex locked, or it could have been
> freed by kfd_ioctl_free_memory_of_gpu.

Thank you for the patch. It's not just about accessing kgd_mem with 
p->mutex held. It's also about holding the mutex continuously. I'd 
update the description to be more explicit about the invariant being 
broken here:

kgd_mem pointers returned by kfd_process_device_translate_handle are 
only guaranteed to be valid while p->mutex is held. As soon as the mutex 
is unlocked, another thread can free the BO.

I can update the description and submit the patch.

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

Regards,
   Felix


>
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 6d291aa6386bd..3c630114210d6 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1293,14 +1293,14 @@ static int kfd_ioctl_map_memory_to_gpu(struct file *filep,
>   		args->n_success = i+1;
>   	}
>   
> -	mutex_unlock(&p->mutex);
> -
>   	err = amdgpu_amdkfd_gpuvm_sync_memory(dev->adev, (struct kgd_mem *) mem, true);
>   	if (err) {
>   		pr_debug("Sync memory failed, wait interrupted by user signal\n");
>   		goto sync_memory_failed;
>   	}
>   
> +	mutex_unlock(&p->mutex);
> +
>   	/* Flush TLBs after waiting for the page table updates to complete */
>   	for (i = 0; i < args->n_devices; i++) {
>   		peer_pdd = kfd_process_device_data_by_id(p, devices_arr[i]);
> @@ -1316,9 +1316,9 @@ static int kfd_ioctl_map_memory_to_gpu(struct file *filep,
>   bind_process_to_device_failed:
>   get_mem_obj_from_handle_failed:
>   map_memory_to_gpu_failed:
> +sync_memory_failed:
>   	mutex_unlock(&p->mutex);
>   copy_from_user_failed:
> -sync_memory_failed:
>   	kfree(devices_arr);
>   
>   	return err;
> @@ -1332,6 +1332,7 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep,
>   	void *mem;
>   	long err = 0;
>   	uint32_t *devices_arr = NULL, i;
> +	bool flush_tlb;
>   
>   	if (!args->n_devices) {
>   		pr_debug("Device IDs array empty\n");
> @@ -1384,16 +1385,19 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep,
>   		}
>   		args->n_success = i+1;
>   	}
> -	mutex_unlock(&p->mutex);
>   
> -	if (kfd_flush_tlb_after_unmap(pdd->dev)) {
> +	flush_tlb = kfd_flush_tlb_after_unmap(pdd->dev);
> +	if (flush_tlb) {
>   		err = amdgpu_amdkfd_gpuvm_sync_memory(pdd->dev->adev,
>   				(struct kgd_mem *) mem, true);
>   		if (err) {
>   			pr_debug("Sync memory failed, wait interrupted by user signal\n");
>   			goto sync_memory_failed;
>   		}
> +	}
> +	mutex_unlock(&p->mutex);
>   
> +	if (flush_tlb) {
>   		/* Flush TLBs after waiting for the page table updates to complete */
>   		for (i = 0; i < args->n_devices; i++) {
>   			peer_pdd = kfd_process_device_data_by_id(p, devices_arr[i]);
> @@ -1409,9 +1413,9 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep,
>   bind_process_to_device_failed:
>   get_mem_obj_from_handle_failed:
>   unmap_memory_from_gpu_failed:
> +sync_memory_failed:
>   	mutex_unlock(&p->mutex);
>   copy_from_user_failed:
> -sync_memory_failed:
>   	kfree(devices_arr);
>   	return err;
>   }