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

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

[Dan Carpenter]

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;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Christian König]

On 23.09.2015 12:59, Dan Carpenter wrote:
> 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>

The whole set looks sane to me, patches are Reviewed-by: Christian König 
<christian.koenig@amd.com>

Regards,
Christian.

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alex Deucher]

On Wed, Sep 23, 2015 at 10:16 AM, Christian König
<christian.koenig@amd.com> wrote:
> On 23.09.2015 12:59, Dan Carpenter wrote:
>>
>> 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>
>
>
> The whole set looks sane to me, patches are Reviewed-by: Christian König
> <christian.koenig@amd.com>

Applied.  thanks!

Alex

>
> Regards,
> Christian.
>
>
>>
>> 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;
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Dan Carpenter]

On Wed, Sep 23, 2015 at 01:13:21PM -0400, Alex Deucher wrote:
> Applied.  thanks!

Ugh.  I'm sorry, I have a signedness bug in this patch.  That was really
sloppy of me.  Should a send a v2 or a fixup?

regards,
dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Deucher, Alexander]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, September 24, 2015 3:57 AM
> To: Alex Deucher
> Cc: Koenig, Christian; David Airlie; Ilja Van Sprundel; security@kernel.org;
> Olsak, Marek; Maling list - DRI developers; Deucher, Alexander; Liu, Monk
> Subject: Re: [patch 1/4] drm/amdgpu: unwind properly in
> amdgpu_cs_parser_init()
> 
> On Wed, Sep 23, 2015 at 01:13:21PM -0400, Alex Deucher wrote:
> > Applied.  thanks!
> 
> Ugh.  I'm sorry, I have a signedness bug in this patch.  That was really
> sloppy of me.  Should a send a v2 or a fixup?

Please send a fixup as I've already applied it.

Thanks,

Alex

> 
> regards,
> dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Dan Carpenter]

The "i" variable should be signed or it leads to a crash in the error
handling code.

Fixes: 1d263474c441 ('drm/amdgpu: unwind properly in amdgpu_cs_parser_init()')
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 749420f..cb3c274 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -156,7 +156,8 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
 	uint64_t *chunk_array_user;
 	uint64_t *chunk_array;
 	struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
-	unsigned size, i;
+	unsigned size;
+	int i;
 	int ret;
 
 	if (cs->in.num_chunks = 0)

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

[Dan Carpenter]

The "i" variable should be signed or it leads to a crash in the error
handling code.

Fixes: 1d263474c441 ('drm/amdgpu: unwind properly in amdgpu_cs_parser_init()')
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 749420f..cb3c274 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -156,7 +156,8 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
 	uint64_t *chunk_array_user;
 	uint64_t *chunk_array;
 	struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
-	unsigned size, i;
+	unsigned size;
+	int i;
 	int ret;
 
 	if (cs->in.num_chunks == 0)

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

[Alex Deucher]

On Fri, Sep 25, 2015 at 7:36 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> The "i" variable should be signed or it leads to a crash in the error
> handling code.
>
> Fixes: 1d263474c441 ('drm/amdgpu: unwind properly in amdgpu_cs_parser_init()')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied.  thanks!

Alex

>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 749420f..cb3c274 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -156,7 +156,8 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
>         uint64_t *chunk_array_user;
>         uint64_t *chunk_array;
>         struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
> -       unsigned size, i;
> +       unsigned size;
> +       int i;
>         int ret;
>
>         if (cs->in.num_chunks = 0)
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alex Deucher]

On Fri, Sep 25, 2015 at 7:36 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> The "i" variable should be signed or it leads to a crash in the error
> handling code.
>
> Fixes: 1d263474c441 ('drm/amdgpu: unwind properly in amdgpu_cs_parser_init()')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied.  thanks!

Alex

>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 749420f..cb3c274 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -156,7 +156,8 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data)
>         uint64_t *chunk_array_user;
>         uint64_t *chunk_array;
>         struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
> -       unsigned size, i;
> +       unsigned size;
> +       int i;
>         int ret;
>
>         if (cs->in.num_chunks == 0)
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[patch 2/4] drm/amdgpu: integer overflow in amdgpu_info_ioctl()

[Dan Carpenter]

The "alloc_size" calculation can overflow leading to memory corruption.

Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
The amdgpu_asic_read_register() functions seem likely to be slow.  They
iterate through all the registers to find the correct register to read.

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 2236793..8c735f5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -390,7 +390,7 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
 				    min((size_t)size, sizeof(vram_gtt))) ? -EFAULT : 0;
 	}
 	case AMDGPU_INFO_READ_MMR_REG: {
-		unsigned n, alloc_size = info->read_mmr_reg.count * 4;
+		unsigned n, alloc_size;
 		uint32_t *regs;
 		unsigned se_num = (info->read_mmr_reg.instance >>
 				   AMDGPU_INFO_MMR_SE_INDEX_SHIFT) &
@@ -406,9 +406,10 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
 		if (sh_num == AMDGPU_INFO_MMR_SH_INDEX_MASK)
 			sh_num = 0xffffffff;
 
-		regs = kmalloc(alloc_size, GFP_KERNEL);
+		regs = kmalloc_array(info->read_mmr_reg.count, sizeof(*regs), GFP_KERNEL);
 		if (!regs)
 			return -ENOMEM;
+		alloc_size = info->read_mmr_reg.count * sizeof(*regs);
 
 		for (i = 0; i < info->read_mmr_reg.count; i++)
 			if (amdgpu_asic_read_register(adev, se_num, sh_num,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[patch 3/4] drm/amdgpu: info leak in amdgpu_gem_metadata_ioctl()

[Dan Carpenter]

There is no limit on args->data.data_size_bytes so we could read beyond
the end of the args->data.data[] array.

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_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 5839fab..dac14de 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -426,6 +426,10 @@ int amdgpu_gem_metadata_ioctl(struct drm_device *dev, void *data,
 					   &args->data.data_size_bytes,
 					   &args->data.flags);
 	} else if (args->op == AMDGPU_GEM_METADATA_OP_SET_METADATA) {
+		if (args->data.data_size_bytes > sizeof(args->data.data)) {
+			r = -EINVAL;
+			goto unreserve;
+		}
 		r = amdgpu_bo_set_tiling_flags(robj, args->data.tiling_info);
 		if (!r)
 			r = amdgpu_bo_set_metadata(robj, args->data.data,
@@ -433,6 +437,7 @@ int amdgpu_gem_metadata_ioctl(struct drm_device *dev, void *data,
 						   args->data.flags);
 	}
 
+unreserve:
 	amdgpu_bo_unreserve(robj);
 out:
 	drm_gem_object_unreference_unlocked(gobj);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[patch 4/4] drm/amdgpu: integer overflow in amdgpu_mode_dumb_create()

[Dan Carpenter]

args->size is a u64.  arg->pitch and args->height are u32.  The
multiplication will overflow instead of using the high 32 bits as
intended.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index dac14de..2023055 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -656,7 +656,7 @@ int amdgpu_mode_dumb_create(struct drm_file *file_priv,
 	int r;
 
 	args->pitch = amdgpu_align_pitch(adev, args->width, args->bpp, 0) * ((args->bpp + 1) / 8);
-	args->size = args->pitch * args->height;
+	args->size = (u64)args->pitch * args->height;
 	args->size = ALIGN(args->size, PAGE_SIZE);
 
 	r = amdgpu_gem_object_create(adev, args->size, 0,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel