| Message ID | 20230713194745.1751-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf/dma-resv: Stop leaking on krealloc() failure | expand |
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);
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);
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);