linux-kernel - Re: [PATCH] drm/amdkfd: rework criu_restore

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b98981f4-358b-ebdd-dade-4027c57a1a7e@amd.com>
Date:   Fri, 18 Feb 2022 13:35:41 -0500
From:   Felix Kuehling <felix.kuehling@....com>
To:     trix@...hat.com, alexander.deucher@....com,
        christian.koenig@....com, Xinhui.Pan@....com, airlied@...ux.ie,
        daniel@...ll.ch, nathan@...nel.org, ndesaulniers@...gle.com,
        david.yatsin@....com, rajneesh.bhardwaj@....com
Cc:     amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH] drm/amdkfd: rework criu_restore_bos error handling

Am 2022-02-18 um 12:39 schrieb trix@...hat.com:
> From: Tom Rix <trix@...hat.com>
>
> Clang static analysis reports this problem
> kfd_chardev.c:2327:2: warning: 1st function call argument
>    is an uninitialized value
>    kvfree(bo_privs);
>    ^~~~~~~~~~~~~~~~
>
> If the copy_from_users(bo_buckets, ...) fails, there is a jump to
> the generic error handler at exit:.  The freeing of bo_privs and
> unwinding of the dmabuf_fd loop do not need to be done.
>
> Add some specific labels for the early failures.
> Reorder the frees to be the reverse of their allocs.
>
> Move the initialize of 'i' back to the loop.
> The problem with the early frees predates the loop
> unwinding problem.

I think the existing error handling strategy in this function is fine. 
Having only one exit label avoids potential issues when using the wrong 
label. Freeing NULL pointers is not a problem. The loop becomes a noop 
if i==0 (this was fixed by you in a previous patch). The only real 
problem I see is that bo_privs is not initialized. So this should really 
be a one-line or maybe two-line fix:

	struct kfd_criu_bo_bucket *bo_buckets = NULL;
	struct kfd_criu_bo_priv_data *bo_privs = NULL;

Regards,
   Felix


>
> Fixes: 73fa13b6a511 ("drm/amdkfd: CRIU Implement KFD restore ioctl")
> Signed-off-by: Tom Rix <trix@...hat.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 965af2a08bc0..1d5f41ac3832 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -2102,7 +2102,7 @@ static int criu_restore_bos(struct kfd_process *p,
>   	const bool criu_resume = true;
>   	bool flush_tlbs = false;
>   	int ret = 0, j = 0;
> -	uint32_t i = 0;
> +	uint32_t i;
>   
>   	if (*priv_offset + (args->num_bos * sizeof(*bo_privs)) > max_priv_data_size)
>   		return -EINVAL;
> @@ -2119,13 +2119,13 @@ static int criu_restore_bos(struct kfd_process *p,
>   	if (ret) {
>   		pr_err("Failed to copy BOs information from user\n");
>   		ret = -EFAULT;
> -		goto exit;
> +		goto free_buckets;
>   	}
>   
>   	bo_privs = kvmalloc_array(args->num_bos, sizeof(*bo_privs), GFP_KERNEL);
>   	if (!bo_privs) {
>   		ret = -ENOMEM;
> -		goto exit;
> +		goto free_buckets;
>   	}
>   
>   	ret = copy_from_user(bo_privs, (void __user *)args->priv_data + *priv_offset,
> @@ -2133,12 +2133,12 @@ static int criu_restore_bos(struct kfd_process *p,
>   	if (ret) {
>   		pr_err("Failed to copy BOs information from user\n");
>   		ret = -EFAULT;
> -		goto exit;
> +		goto free_privs;
>   	}
>   	*priv_offset += args->num_bos * sizeof(*bo_privs);
>   
>   	/* Create and map new BOs */
> -	for (; i < args->num_bos; i++) {
> +	for (i = 0; i < args->num_bos; i++) {
>   		struct kfd_criu_bo_bucket *bo_bucket;
>   		struct kfd_criu_bo_priv_data *bo_priv;
>   		struct kfd_dev *dev;
> @@ -2323,8 +2323,11 @@ static int criu_restore_bos(struct kfd_process *p,
>   		if (bo_buckets[i].alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_VRAM)
>   			close_fd(bo_buckets[i].dmabuf_fd);
>   	}
> -	kvfree(bo_buckets);
> +free_privs:
>   	kvfree(bo_privs);
> +free_buckets:
> +	kvfree(bo_buckets);
> +
>   	return ret;
>   }
>