* [patch 1/4] drm/amdgpu: unwind properly in amdgpu_cs_parser_init()
[not found] <13E61BCA7787794E89BDF39B8DE40C024D12E9F63F@ioaexchange.ioactive.local>
@ 2015-09-23 10:59 ` Dan Carpenter
2015-09-23 14:16 ` Christian König
2015-09-23 11:00 ` [patch 2/4] drm/amdgpu: integer overflow in amdgpu_info_ioctl() Dan Carpenter
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2015-09-23 10:59 UTC (permalink / raw)
To: David Airlie, Ilja Van Sprundel
Cc: security, Marek Olšák, dri-devel, Alex Deucher,
Christian König, monk.liu
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [patch 2/4] drm/amdgpu: integer overflow in amdgpu_info_ioctl()
[not found] <13E61BCA7787794E89BDF39B8DE40C024D12E9F63F@ioaexchange.ioactive.local>
2015-09-23 10:59 ` [patch 1/4] drm/amdgpu: unwind properly in amdgpu_cs_parser_init() Dan Carpenter
@ 2015-09-23 11:00 ` Dan Carpenter
2015-09-23 11:00 ` [patch 3/4] drm/amdgpu: info leak in amdgpu_gem_metadata_ioctl() Dan Carpenter
2015-09-23 11:00 ` [patch 4/4] drm/amdgpu: integer overflow in amdgpu_mode_dumb_create() Dan Carpenter
3 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2015-09-23 11:00 UTC (permalink / raw)
To: David Airlie, Ilja Van Sprundel
Cc: security, dri-devel, yanyang1, Alex Deucher, Ken Wang,
Christian König, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [patch 3/4] drm/amdgpu: info leak in amdgpu_gem_metadata_ioctl()
[not found] <13E61BCA7787794E89BDF39B8DE40C024D12E9F63F@ioaexchange.ioactive.local>
2015-09-23 10:59 ` [patch 1/4] drm/amdgpu: unwind properly in amdgpu_cs_parser_init() Dan Carpenter
2015-09-23 11:00 ` [patch 2/4] drm/amdgpu: integer overflow in amdgpu_info_ioctl() Dan Carpenter
@ 2015-09-23 11:00 ` Dan Carpenter
2015-09-23 11:00 ` [patch 4/4] drm/amdgpu: integer overflow in amdgpu_mode_dumb_create() Dan Carpenter
3 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2015-09-23 11:00 UTC (permalink / raw)
To: David Airlie, Ilja Van Sprundel
Cc: security, Marek Olšák, dri-devel, Alex Deucher,
Christian König, monk.liu
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [patch 4/4] drm/amdgpu: integer overflow in amdgpu_mode_dumb_create()
[not found] <13E61BCA7787794E89BDF39B8DE40C024D12E9F63F@ioaexchange.ioactive.local>
` (2 preceding siblings ...)
2015-09-23 11:00 ` [patch 3/4] drm/amdgpu: info leak in amdgpu_gem_metadata_ioctl() Dan Carpenter
@ 2015-09-23 11:00 ` Dan Carpenter
3 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2015-09-23 11:00 UTC (permalink / raw)
To: David Airlie
Cc: security, Marek Olšák, Ilja Van Sprundel, dri-devel,
Alex Deucher, Leo Liu, Christian König, monk.liu
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch 1/4] drm/amdgpu: unwind properly in amdgpu_cs_parser_init()
2015-09-23 10:59 ` [patch 1/4] drm/amdgpu: unwind properly in amdgpu_cs_parser_init() Dan Carpenter
@ 2015-09-23 14:16 ` Christian König
2015-09-23 17:13 ` Alex Deucher
0 siblings, 1 reply; 10+ messages in thread
From: Christian König @ 2015-09-23 14:16 UTC (permalink / raw)
To: Dan Carpenter, David Airlie, Ilja Van Sprundel
Cc: security, Marek Olšák, dri-devel, Alex Deucher, monk.liu
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch 1/4] drm/amdgpu: unwind properly in amdgpu_cs_parser_init()
2015-09-23 14:16 ` Christian König
@ 2015-09-23 17:13 ` Alex Deucher
2015-09-24 7:56 ` Dan Carpenter
0 siblings, 1 reply; 10+ messages in thread
From: Alex Deucher @ 2015-09-23 17:13 UTC (permalink / raw)
To: Christian König
Cc: security, Marek Olšák, Ilja Van Sprundel,
Maling list - DRI developers, monk.liu, Alex Deucher,
Dan Carpenter
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch 1/4] drm/amdgpu: unwind properly in amdgpu_cs_parser_init()
2015-09-23 17:13 ` Alex Deucher
@ 2015-09-24 7:56 ` Dan Carpenter
2015-09-24 12:56 ` Deucher, Alexander
0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2015-09-24 7:56 UTC (permalink / raw)
To: Alex Deucher
Cc: security, Marek Olšák, Ilja Van Sprundel,
Maling list - DRI developers, Alex Deucher, Christian König,
monk.liu
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [patch 1/4] drm/amdgpu: unwind properly in amdgpu_cs_parser_init()
2015-09-24 7:56 ` Dan Carpenter
@ 2015-09-24 12:56 ` Deucher, Alexander
2015-09-25 11:36 ` [patch] drm/amdgpu: signedness bug " Dan Carpenter
0 siblings, 1 reply; 10+ messages in thread
From: Deucher, Alexander @ 2015-09-24 12:56 UTC (permalink / raw)
To: Dan Carpenter, Alex Deucher
Cc: security, Olsak, Marek, Ilja Van Sprundel,
Maling list - DRI developers, Koenig, Christian, Liu, Monk
> -----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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [patch] drm/amdgpu: signedness bug in amdgpu_cs_parser_init()
2015-09-24 12:56 ` Deucher, Alexander
@ 2015-09-25 11:36 ` Dan Carpenter
2015-09-29 17:44 ` Alex Deucher
0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2015-09-25 11:36 UTC (permalink / raw)
To: David Airlie
Cc: Christian König, Chunming Zhou, Alex Deucher, Jammy Zhou,
monk.liu, Marek Olšák, dri-devel, kernel-janitors,
security, Ilja Van Sprundel
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)
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch] drm/amdgpu: signedness bug in amdgpu_cs_parser_init()
2015-09-25 11:36 ` [patch] drm/amdgpu: signedness bug " Dan Carpenter
@ 2015-09-29 17:44 ` Alex Deucher
0 siblings, 0 replies; 10+ messages in thread
From: Alex Deucher @ 2015-09-29 17:44 UTC (permalink / raw)
To: Dan Carpenter
Cc: security, Marek Olšák, Ilja Van Sprundel,
kernel-janitors, Maling list - DRI developers, Alex Deucher,
Christian König, monk.liu
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-09-29 17:44 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <13E61BCA7787794E89BDF39B8DE40C024D12E9F63F@ioaexchange.ioactive.local>
2015-09-23 10:59 ` [patch 1/4] drm/amdgpu: unwind properly in amdgpu_cs_parser_init() Dan Carpenter
2015-09-23 14:16 ` Christian König
2015-09-23 17:13 ` Alex Deucher
2015-09-24 7:56 ` Dan Carpenter
2015-09-24 12:56 ` Deucher, Alexander
2015-09-25 11:36 ` [patch] drm/amdgpu: signedness bug " Dan Carpenter
2015-09-29 17:44 ` Alex Deucher
2015-09-23 11:00 ` [patch 2/4] drm/amdgpu: integer overflow in amdgpu_info_ioctl() Dan Carpenter
2015-09-23 11:00 ` [patch 3/4] drm/amdgpu: info leak in amdgpu_gem_metadata_ioctl() Dan Carpenter
2015-09-23 11:00 ` [patch 4/4] drm/amdgpu: integer overflow in amdgpu_mode_dumb_create() Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).