* [PATCH] drm/vc4: Fix resource leak in 'vc4_get_hang_state_ioctl()' in error handling path
@ 2017-05-10 3:53 ` Christophe JAILLET
0 siblings, 0 replies; 5+ messages in thread
From: Christophe JAILLET @ 2017-05-10 3:53 UTC (permalink / raw)
To: eric, airlied
Cc: dri-devel, linux-kernel, kernel-janitors, Christophe JAILLET
If one 'drm_gem_handle_create()' fails, we leak somes handles and some
memory.
In order to fix it:
- move the 'free(bo_state)' at the end of the function in the error
handling path. This has the side effect to also try to free it if the
first 'kcalloc' fails. This is harmless.
- delete already allocated handles
- remove the now useless 'err' label
The way the code is now written will also delete the handles if the
'copy_to_user()' call fails.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch also add the 'vc4_free_hang_state()' call in the error
handling path. It sounds logical to me, but I'm not sure of it.
---
drivers/gpu/drm/vc4/vc4_gem.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index e9c381c42139..891c7a22cf81 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -111,8 +111,8 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
&handle);
if (ret) {
- state->bo_count = i - 1;
- goto err;
+ state->bo_count = i;
+ goto err_free;
}
bo_state[i].handle = handle;
bo_state[i].paddr = vc4_bo->base.paddr;
@@ -124,13 +124,16 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
state->bo_count * sizeof(*bo_state)))
ret = -EFAULT;
- kfree(bo_state);
-
err_free:
-
vc4_free_hang_state(dev, kernel_state);
-err:
+ if (ret) {
+ for (i = 0; i < state->bo_count; i++)
+ drm_gem_handle_delete(file_priv, bo_state[i].handle);
+ }
+
+ kfree(bo_state);
+
return ret;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] drm/vc4: Fix resource leak in 'vc4_get_hang_state_ioctl()' in error handling path
@ 2017-05-10 3:53 ` Christophe JAILLET
0 siblings, 0 replies; 5+ messages in thread
From: Christophe JAILLET @ 2017-05-10 3:53 UTC (permalink / raw)
To: eric, airlied
Cc: dri-devel, linux-kernel, kernel-janitors, Christophe JAILLET
If one 'drm_gem_handle_create()' fails, we leak somes handles and some
memory.
In order to fix it:
- move the 'free(bo_state)' at the end of the function in the error
handling path. This has the side effect to also try to free it if the
first 'kcalloc' fails. This is harmless.
- delete already allocated handles
- remove the now useless 'err' label
The way the code is now written will also delete the handles if the
'copy_to_user()' call fails.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch also add the 'vc4_free_hang_state()' call in the error
handling path. It sounds logical to me, but I'm not sure of it.
---
drivers/gpu/drm/vc4/vc4_gem.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index e9c381c42139..891c7a22cf81 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -111,8 +111,8 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
&handle);
if (ret) {
- state->bo_count = i - 1;
- goto err;
+ state->bo_count = i;
+ goto err_free;
}
bo_state[i].handle = handle;
bo_state[i].paddr = vc4_bo->base.paddr;
@@ -124,13 +124,16 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
state->bo_count * sizeof(*bo_state)))
ret = -EFAULT;
- kfree(bo_state);
-
err_free:
-
vc4_free_hang_state(dev, kernel_state);
-err:
+ if (ret) {
+ for (i = 0; i < state->bo_count; i++)
+ drm_gem_handle_delete(file_priv, bo_state[i].handle);
+ }
+
+ kfree(bo_state);
+
return ret;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/vc4: Fix resource leak in 'vc4_get_hang_state_ioctl()' in error handling path
2017-05-10 3:53 ` Christophe JAILLET
(?)
@ 2017-05-10 18:15 ` Eric Anholt
-1 siblings, 0 replies; 5+ messages in thread
From: Eric Anholt @ 2017-05-10 18:15 UTC (permalink / raw)
To: Christophe JAILLET, airlied
Cc: dri-devel, linux-kernel, kernel-janitors, Christophe JAILLET
[-- Attachment #1: Type: text/plain, Size: 1962 bytes --]
Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
> If one 'drm_gem_handle_create()' fails, we leak somes handles and some
> memory.
>
> In order to fix it:
> - move the 'free(bo_state)' at the end of the function in the error
> handling path. This has the side effect to also try to free it if the
> first 'kcalloc' fails. This is harmless.
> - delete already allocated handles
> - remove the now useless 'err' label
>
> The way the code is now written will also delete the handles if the
> 'copy_to_user()' call fails.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch also add the 'vc4_free_hang_state()' call in the error
> handling path. It sounds logical to me, but I'm not sure of it.
> ---
> drivers/gpu/drm/vc4/vc4_gem.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> index e9c381c42139..891c7a22cf81 100644
> --- a/drivers/gpu/drm/vc4/vc4_gem.c
> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> @@ -111,8 +111,8 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
> &handle);
>
> if (ret) {
> - state->bo_count = i - 1;
> - goto err;
> + state->bo_count = i;
> + goto err_free;
> }
> bo_state[i].handle = handle;
> bo_state[i].paddr = vc4_bo->base.paddr;
> @@ -124,13 +124,16 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
> state->bo_count * sizeof(*bo_state)))
> ret = -EFAULT;
>
> - kfree(bo_state);
> -
> err_free:
> -
> vc4_free_hang_state(dev, kernel_state);
>
> -err:
> + if (ret) {
> + for (i = 0; i < state->bo_count; i++)
> + drm_gem_handle_delete(file_priv, bo_state[i].handle);
> + }
vc4_free_hang_state() has freed the state pointer you're using here.
Also, I think you'd have a NULL deref from the goto err_free in the
!bo_state error path.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/vc4: Fix resource leak in 'vc4_get_hang_state_ioctl()' in error handling path
@ 2017-05-10 18:15 ` Eric Anholt
0 siblings, 0 replies; 5+ messages in thread
From: Eric Anholt @ 2017-05-10 18:15 UTC (permalink / raw)
To: airlied; +Cc: Christophe JAILLET, kernel-janitors, linux-kernel, dri-devel
[-- Attachment #1: Type: text/plain, Size: 1962 bytes --]
Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
> If one 'drm_gem_handle_create()' fails, we leak somes handles and some
> memory.
>
> In order to fix it:
> - move the 'free(bo_state)' at the end of the function in the error
> handling path. This has the side effect to also try to free it if the
> first 'kcalloc' fails. This is harmless.
> - delete already allocated handles
> - remove the now useless 'err' label
>
> The way the code is now written will also delete the handles if the
> 'copy_to_user()' call fails.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch also add the 'vc4_free_hang_state()' call in the error
> handling path. It sounds logical to me, but I'm not sure of it.
> ---
> drivers/gpu/drm/vc4/vc4_gem.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> index e9c381c42139..891c7a22cf81 100644
> --- a/drivers/gpu/drm/vc4/vc4_gem.c
> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> @@ -111,8 +111,8 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
> &handle);
>
> if (ret) {
> - state->bo_count = i - 1;
> - goto err;
> + state->bo_count = i;
> + goto err_free;
> }
> bo_state[i].handle = handle;
> bo_state[i].paddr = vc4_bo->base.paddr;
> @@ -124,13 +124,16 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
> state->bo_count * sizeof(*bo_state)))
> ret = -EFAULT;
>
> - kfree(bo_state);
> -
> err_free:
> -
> vc4_free_hang_state(dev, kernel_state);
>
> -err:
> + if (ret) {
> + for (i = 0; i < state->bo_count; i++)
> + drm_gem_handle_delete(file_priv, bo_state[i].handle);
> + }
vc4_free_hang_state() has freed the state pointer you're using here.
Also, I think you'd have a NULL deref from the goto err_free in the
!bo_state error path.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/vc4: Fix resource leak in 'vc4_get_hang_state_ioctl()' in error handling path
@ 2017-05-10 18:15 ` Eric Anholt
0 siblings, 0 replies; 5+ messages in thread
From: Eric Anholt @ 2017-05-10 18:15 UTC (permalink / raw)
To: airlied; +Cc: Christophe JAILLET, kernel-janitors, linux-kernel, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1962 bytes --]
Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
> If one 'drm_gem_handle_create()' fails, we leak somes handles and some
> memory.
>
> In order to fix it:
> - move the 'free(bo_state)' at the end of the function in the error
> handling path. This has the side effect to also try to free it if the
> first 'kcalloc' fails. This is harmless.
> - delete already allocated handles
> - remove the now useless 'err' label
>
> The way the code is now written will also delete the handles if the
> 'copy_to_user()' call fails.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch also add the 'vc4_free_hang_state()' call in the error
> handling path. It sounds logical to me, but I'm not sure of it.
> ---
> drivers/gpu/drm/vc4/vc4_gem.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> index e9c381c42139..891c7a22cf81 100644
> --- a/drivers/gpu/drm/vc4/vc4_gem.c
> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> @@ -111,8 +111,8 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
> &handle);
>
> if (ret) {
> - state->bo_count = i - 1;
> - goto err;
> + state->bo_count = i;
> + goto err_free;
> }
> bo_state[i].handle = handle;
> bo_state[i].paddr = vc4_bo->base.paddr;
> @@ -124,13 +124,16 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
> state->bo_count * sizeof(*bo_state)))
> ret = -EFAULT;
>
> - kfree(bo_state);
> -
> err_free:
> -
> vc4_free_hang_state(dev, kernel_state);
>
> -err:
> + if (ret) {
> + for (i = 0; i < state->bo_count; i++)
> + drm_gem_handle_delete(file_priv, bo_state[i].handle);
> + }
vc4_free_hang_state() has freed the state pointer you're using here.
Also, I think you'd have a NULL deref from the goto err_free in the
!bo_state error path.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-05-10 18:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-10 3:53 [PATCH] drm/vc4: Fix resource leak in 'vc4_get_hang_state_ioctl()' in error handling path Christophe JAILLET
2017-05-10 3:53 ` Christophe JAILLET
2017-05-10 18:15 ` Eric Anholt
2017-05-10 18:15 ` Eric Anholt
2017-05-10 18:15 ` Eric Anholt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.