[Linaro-mm-sig] Re: Question about 'dma_resv_get_fences'

Hi,

Am 30.09.24 um 21:38 schrieb Zichen Xie:

Dear Linux Developers for DMA BUFFER SHARING FRAMEWORK,

We are curious about the function 'dma_resv_get_fences' here: https://elixir.bootlin.com/linux/v6.11/source/drivers/dma-buf/dma-resv.c#L56..., and the logic below:

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)
dma_fence_put((*fences)[--(*num_fences)]);

count = cursor.num_fences + 1;

/* Eventually re-allocate the array */
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);
}

The existing check 'if (count && !new_fences)' may fail if count==0, and 'krealloc_array' with count==0 is an undefined behavior. The realloc may fail and return a NULL pointer, leading to a NULL Pointer Dereference in '(*fences)[(*num_fences)++] = dma_fence_get(fence);'

You already answered the question yourself "count = cursor.num_fences + 1;". So count can never be 0.

What could theoretically be possible is that num_fences overflows, but this value isn't userspace controllable and we would run into memory allocation failures long before that happened.

But we could potentially remove this whole handling since if there are no fences in the dma_resv object we don't enter the loop in the first place.

Regards, Christian.

Please correct us if we miss some key prerequisites for this function! Thank you very much!