[PATCH] dma-buf/dma-resv: Stop leaking on krealloc() failure

[PATCH] dma-buf/dma-resv: Stop leaking on krealloc() failure

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently dma_resv_get_fences() will leak the previously
allocated array if the fence iteration got restarted and
the krealloc_array() fails.

Free the old array by hand, and make sure we still clear
the returned *fences so the caller won't end up accessing
freed memory. Some (but not all) of the callers of
dma_resv_get_fences() seem to still trawl through the
array even when dma_resv_get_fences() failed. And let's
zero out *num_fences as well for good measure.

Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linaro-mm-sig@lists.linaro.org
Fixes: d3c80698c9f5 ("dma-buf: use new iterator in dma_resv_get_fences v3")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/dma-buf/dma-resv.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index b6f71eb00866..38b4110378de 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -571,6 +571,7 @@ int dma_resv_get_fences(struct dma_resv *obj, enum dma_resv_usage usage,
 	dma_resv_for_each_fence_unlocked(&cursor, fence) {
 
 		if (dma_resv_iter_is_restarted(&cursor)) {
+			struct dma_fence **new_fences;
 			unsigned int count;
 
 			while (*num_fences)
@@ -579,13 +580,17 @@ int dma_resv_get_fences(struct dma_resv *obj, enum dma_resv_usage usage,
 			count = cursor.num_fences + 1;
 
 			/* Eventually re-allocate the array */
-			*fences = krealloc_array(*fences, count,
-						 sizeof(void *),
-						 GFP_KERNEL);
-			if (count && !*fences) {
+			new_fences = krealloc_array(*fences, count,
+						    sizeof(void *),
+						    GFP_KERNEL);
+			if (count && !new_fences) {
+				kfree(*fences);
+				*fences = NULL;
+				*num_fences = 0;
 				dma_resv_iter_end(&cursor);
 				return -ENOMEM;
 			}
+			*fences = new_fences;
 		}
 
 		(*fences)[(*num_fences)++] = dma_fence_get(fence);
-- 
2.39.3

Re: [PATCH] dma-buf/dma-resv: Stop leaking on krealloc() failure

[Christian König]

Am 13.07.23 um 21:47 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently dma_resv_get_fences() will leak the previously
> allocated array if the fence iteration got restarted and
> the krealloc_array() fails.
>
> Free the old array by hand, and make sure we still clear
> the returned *fences so the caller won't end up accessing
> freed memory. Some (but not all) of the callers of
> dma_resv_get_fences() seem to still trawl through the
> array even when dma_resv_get_fences() failed. And let's
> zero out *num_fences as well for good measure.
>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-sig@lists.linaro.org
> Fixes: d3c80698c9f5 ("dma-buf: use new iterator in dma_resv_get_fences v3")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Good catch, Reviewed-by: Christian König <christian.koenig@amd.com>

Should I add a CC: stable and push to drm-misc-fixes?

Thanks,
Christian.

> ---
>   drivers/dma-buf/dma-resv.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> index b6f71eb00866..38b4110378de 100644
> --- a/drivers/dma-buf/dma-resv.c
> +++ b/drivers/dma-buf/dma-resv.c
> @@ -571,6 +571,7 @@ int dma_resv_get_fences(struct dma_resv *obj, enum dma_resv_usage usage,
>   	dma_resv_for_each_fence_unlocked(&cursor, fence) {
>   
>   		if (dma_resv_iter_is_restarted(&cursor)) {
> +			struct dma_fence **new_fences;
>   			unsigned int count;
>   
>   			while (*num_fences)
> @@ -579,13 +580,17 @@ int dma_resv_get_fences(struct dma_resv *obj, enum dma_resv_usage usage,
>   			count = cursor.num_fences + 1;
>   
>   			/* Eventually re-allocate the array */
> -			*fences = krealloc_array(*fences, count,
> -						 sizeof(void *),
> -						 GFP_KERNEL);
> -			if (count && !*fences) {
> +			new_fences = krealloc_array(*fences, count,
> +						    sizeof(void *),
> +						    GFP_KERNEL);
> +			if (count && !new_fences) {
> +				kfree(*fences);
> +				*fences = NULL;
> +				*num_fences = 0;
>   				dma_resv_iter_end(&cursor);
>   				return -ENOMEM;
>   			}
> +			*fences = new_fences;
>   		}
>   
>   		(*fences)[(*num_fences)++] = dma_fence_get(fence);

Re: [PATCH] dma-buf/dma-resv: Stop leaking on krealloc() failure

[Ville Syrjälä]

On Fri, Jul 14, 2023 at 08:56:15AM +0200, Christian König wrote:
> Am 13.07.23 um 21:47 schrieb Ville Syrjala:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Currently dma_resv_get_fences() will leak the previously
> > allocated array if the fence iteration got restarted and
> > the krealloc_array() fails.
> >
> > Free the old array by hand, and make sure we still clear
> > the returned *fences so the caller won't end up accessing
> > freed memory. Some (but not all) of the callers of
> > dma_resv_get_fences() seem to still trawl through the
> > array even when dma_resv_get_fences() failed. And let's
> > zero out *num_fences as well for good measure.
> >
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: linux-media@vger.kernel.org
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linaro-mm-sig@lists.linaro.org
> > Fixes: d3c80698c9f5 ("dma-buf: use new iterator in dma_resv_get_fences v3")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Good catch, Reviewed-by: Christian König <christian.koenig@amd.com>
> 
> Should I add a CC: stable and push to drm-misc-fixes?

Sure, if you don't mind. Thanks.

> 
> Thanks,
> Christian.
> 
> > ---
> >   drivers/dma-buf/dma-resv.c | 13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> > index b6f71eb00866..38b4110378de 100644
> > --- a/drivers/dma-buf/dma-resv.c
> > +++ b/drivers/dma-buf/dma-resv.c
> > @@ -571,6 +571,7 @@ int dma_resv_get_fences(struct dma_resv *obj, enum dma_resv_usage usage,
> >   	dma_resv_for_each_fence_unlocked(&cursor, fence) {
> >   
> >   		if (dma_resv_iter_is_restarted(&cursor)) {
> > +			struct dma_fence **new_fences;
> >   			unsigned int count;
> >   
> >   			while (*num_fences)
> > @@ -579,13 +580,17 @@ int dma_resv_get_fences(struct dma_resv *obj, enum dma_resv_usage usage,
> >   			count = cursor.num_fences + 1;
> >   
> >   			/* Eventually re-allocate the array */
> > -			*fences = krealloc_array(*fences, count,
> > -						 sizeof(void *),
> > -						 GFP_KERNEL);
> > -			if (count && !*fences) {
> > +			new_fences = krealloc_array(*fences, count,
> > +						    sizeof(void *),
> > +						    GFP_KERNEL);
> > +			if (count && !new_fences) {
> > +				kfree(*fences);
> > +				*fences = NULL;
> > +				*num_fences = 0;
> >   				dma_resv_iter_end(&cursor);
> >   				return -ENOMEM;
> >   			}
> > +			*fences = new_fences;
> >   		}
> >   
> >   		(*fences)[(*num_fences)++] = dma_fence_get(fence);

-- 
Ville Syrjälä
Intel