[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 <jcrouse at codeaurora.org>

> Fixes: 62e3a3e342af ("drm/msm: fix leak in failed get_pages")
> Signed-off-by: Ben Hutchings <ben.hutchings at codethink.co.uk>
> ---
>  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 majordomo at 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