New subject: [patch] drm/amdgpu: signedness bug in amdgpu_cs_parser_init()

23 Sep 2015

The amdgpu_cs_parser_init() function doesn't clean up after itself but
instead the caller uses a free everything function amdgpu_cs_parser_fini()
on failure.  This style of error handling is often buggy.  In this
example, we call "drm_free_large(parser->chunks[i].kdata);" when it is
an unintialized pointer or when "parser->chunks" is NULL.
I fixed this bug by adding unwind code so that it frees everything that
it allocates.
I also mode some other very minor changes:
1) Renamed "r" to "ret".
2) Moved the chunk_array allocation to the start of the function.
3) Removed some initializers which are no longer needed.
Reported-by: Ilja Van Sprundel ivansprundel@ioactive.com
Signed-off-by: Dan Carpenter dan.carpenter@oracle.com

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 3b355ae..abb257d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -154,42 +154,41 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
 {
    union drm_amdgpu_cs *cs = data;
    uint64_t *chunk_array_user;
-	uint64_t *chunk_array = NULL;
+	uint64_t *chunk_array;
    struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
    unsigned size, i;
-	int r = 0;
+	int ret;
-	if (!cs->in.num_chunks)
-		goto out;
+	if (cs->in.num_chunks == 0)
+		return 0;
+
+	chunk_array = kmalloc_array(cs->in.num_chunks, sizeof(uint64_t), GFP_KERNEL);
+	if (!chunk_array)
+		return -ENOMEM;
p->ctx = amdgpu_ctx_get(fpriv, cs->in.ctx_id);
    if (!p->ctx) {
-		r = -EINVAL;
-		goto out;
+		ret = -EINVAL;
+		goto free_chunk;
    }
+
    p->bo_list = amdgpu_bo_list_get(fpriv, cs->in.bo_list_handle);
/* get chunks */
    INIT_LIST_HEAD(&p->validated);
-	chunk_array = kmalloc_array(cs->in.num_chunks, sizeof(uint64_t), GFP_KERNEL);
-	if (chunk_array == NULL) {
-		r = -ENOMEM;
-		goto out;
-	}
-
    chunk_array_user = (uint64_t __user *)(cs->in.chunks);
    if (copy_from_user(chunk_array, chunk_array_user,
    		   sizeof(uint64_t)*cs->in.num_chunks)) {
-		r = -EFAULT;
-		goto out;
+		ret = -EFAULT;
+		goto put_bo_list;
    }
p->nchunks = cs->in.num_chunks;
    p->chunks = kmalloc_array(p->nchunks, sizeof(struct amdgpu_cs_chunk),
    		    GFP_KERNEL);
-	if (p->chunks == NULL) {
-		r = -ENOMEM;
-		goto out;
+	if (!p->chunks) {
+		ret = -ENOMEM;
+		goto put_bo_list;
    }
for (i = 0; i < p->nchunks; i++) {
@@ -200,8 +199,9 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
    	chunk_ptr = (void __user *)chunk_array[i];
    	if (copy_from_user(&user_chunk, chunk_ptr,
    			       sizeof(struct drm_amdgpu_cs_chunk))) {
-			r = -EFAULT;
-			goto out;
+			ret = -EFAULT;
+			i--;
+			goto free_partial_kdata;
    	}
    	p->chunks[i].chunk_id = user_chunk.chunk_id;
    	p->chunks[i].length_dw = user_chunk.length_dw;
@@ -212,13 +212,14 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
p->chunks[i].kdata = drm_malloc_ab(size, sizeof(uint32_t));
    	if (p->chunks[i].kdata == NULL) {
-			r = -ENOMEM;
-			goto out;
+			ret = -ENOMEM;
+			i--;
+			goto free_partial_kdata;
    	}
    	size *= sizeof(uint32_t);
    	if (copy_from_user(p->chunks[i].kdata, cdata, size)) {
-			r = -EFAULT;
-			goto out;
+			ret = -EFAULT;
+			goto free_partial_kdata;
    	}
switch (p->chunks[i].chunk_id) {
@@ -238,15 +239,15 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
    			gobj = drm_gem_object_lookup(p->adev->ddev,
    						     p->filp, handle);
    			if (gobj == NULL) {
-					r = -EINVAL;
-					goto out;
+					ret = -EINVAL;
+					goto free_partial_kdata;
    			}
p->uf.bo = gem_to_amdgpu_bo(gobj);
    			p->uf.offset = fence_data->offset;
    		} else {
-				r = -EINVAL;
-				goto out;
+				ret = -EINVAL;
+				goto free_partial_kdata;
    		}
    		break;
@@ -254,19 +255,35 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
    		break;
default:
-			r = -EINVAL;
-			goto out;
+			ret = -EINVAL;
+			goto free_partial_kdata;
    	}
    }
p->ibs = kcalloc(p->num_ibs, sizeof(struct amdgpu_ib), GFP_KERNEL);
-	if (!p->ibs)
-		r = -ENOMEM;
+	if (!p->ibs) {
+		ret = -ENOMEM;
+		goto free_all_kdata;
+	}
-out:
    kfree(chunk_array);
-	return r;
+	return 0;
+
+free_all_kdata:
+	i = p->nchunks - 1;
+free_partial_kdata:
+	for (; i >= 0; i--)
+		drm_free_large(p->chunks[i].kdata);
+	kfree(p->chunks);
+put_bo_list:
+	if (p->bo_list)
+		amdgpu_bo_list_put(p->bo_list);
+	amdgpu_ctx_put(p->ctx);
+free_chunk:
+	kfree(chunk_array);
+
+	return ret;
 }
/* Returns how many bytes TTM can move per IB.
@@ -804,7 +821,7 @@ int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
    r = amdgpu_cs_parser_init(parser, data);
    if (r) {
    	DRM_ERROR("Failed to initialize parser !\n");
-		amdgpu_cs_parser_fini(parser, r, false);
+		kfree(parser);
    	up_read(&adev->exclusive_lock);
    	r = amdgpu_cs_handle_lockup(adev, r);
    	return r;

    

[patch 1/4] drm/amdgpu: unwind properly in amdgpu_cs_parser_init()