[PATCH] drm/msm: Fix possible null dereference on failure of get_pages()

[Ben Hutchings]

Commit 62e3a3e342af changed get_pages() to initialise
msm_gem_object::pages before trying to initialise msm_gem_object::sgt,
so that put_pages() would properly clean up pages in the failure
case.

However, this means that put_pages() now needs to check that
msm_gem_object::sgt is not null before trying to clean it up, and
this check was only applied to part of the cleanup code.  Move
it all into the conditional block.  (Strictly speaking we don't
need to make the kfree() conditional, but since we can't avoid
checking for null ourselves we may as well do so.)

Fixes: 62e3a3e342af ("drm/msm: fix leak in failed get_pages")
Signed-off-by: Ben Hutchings 
---
 drivers/gpu/drm/msm/msm_gem.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 07376de9ff4c..37ec3411297b 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -132,17 +132,19 @@ static void put_pages(struct drm_gem_object *obj)
struct msm_gem_object *msm_obj = to_msm_bo(obj);
 
if (msm_obj->pages) {
-   /* For non-cached buffers, ensure the new pages are clean
-* because display controller, GPU, etc. are not coherent:
-*/
-   if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
-   dma_unmap_sg(obj->dev->dev, msm_obj->sgt->sgl,
-   msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
+   if (msm_obj->sgt) {
+   /* For non-cached buffers, ensure the new
+* pages are clean because display controller,
+* GPU, etc. are not coherent:
+*/
+   if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
+   dma_unmap_sg(obj->dev->dev, msm_obj->sgt->sgl,
+msm_obj->sgt->nents,
+DMA_BIDIRECTIONAL);
 
-   if (msm_obj->sgt)
sg_free_table(msm_obj->sgt);
-
-   kfree(msm_obj->sgt);
+   kfree(msm_obj->sgt);
+   }
 
if (use_pages(obj))
drm_gem_put_pages(obj, msm_obj->pages, true, false);
-- 
2.16.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/msm: Fix possible null dereference on failure of get_pages()

[Jordan Crouse]

On Tue, Apr 03, 2018 at 11:38:45PM +0100, Ben Hutchings wrote:
> Commit 62e3a3e342af changed get_pages() to initialise
> msm_gem_object::pages before trying to initialise msm_gem_object::sgt,
> so that put_pages() would properly clean up pages in the failure
> case.
> 
> However, this means that put_pages() now needs to check that
> msm_gem_object::sgt is not null before trying to clean it up, and
> this check was only applied to part of the cleanup code.  Move
> it all into the conditional block.  (Strictly speaking we don't
> need to make the kfree() conditional, but since we can't avoid
> checking for null ourselves we may as well do so.)

Seems legit to me. Thanks for the catch.

Reviewed-by: Jordan Crouse 

> Fixes: 62e3a3e342af ("drm/msm: fix leak in failed get_pages")
> Signed-off-by: Ben Hutchings 
> ---
>  drivers/gpu/drm/msm/msm_gem.c | 20 +++-
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 07376de9ff4c..37ec3411297b 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -132,17 +132,19 @@ static void put_pages(struct drm_gem_object *obj)
>   struct msm_gem_object *msm_obj = to_msm_bo(obj);
>  
>   if (msm_obj->pages) {
> - /* For non-cached buffers, ensure the new pages are clean
> -  * because display controller, GPU, etc. are not coherent:
> -  */
> - if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
> - dma_unmap_sg(obj->dev->dev, msm_obj->sgt->sgl,
> - msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
> + if (msm_obj->sgt) {
> + /* For non-cached buffers, ensure the new
> +  * pages are clean because display controller,
> +  * GPU, etc. are not coherent:
> +  */
> + if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
> + dma_unmap_sg(obj->dev->dev, msm_obj->sgt->sgl,
> +  msm_obj->sgt->nents,
> +  DMA_BIDIRECTIONAL);
>  
> - if (msm_obj->sgt)
>   sg_free_table(msm_obj->sgt);
> -
> - kfree(msm_obj->sgt);
> + kfree(msm_obj->sgt);
> + }
>  
>   if (use_pages(obj))
>   drm_gem_put_pages(obj, msm_obj->pages, true, false);
> -- 
> 2.16.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel