Re: [PATCH] dma-buf: fix stack corruption in dma_fence_chain_release

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

That one test creates a 32k chain of fences I think.
Anyway my kernel crash was unrelated ;)

-Lionel

On 05/08/2019 16:02, Christian König wrote:
> Not even remotely :)I tested this with my own crafted code inside the 
> kernel.
>
> It's probably quite some hassle to actually trigger this problem from 
> userspace and I only found it because I created a very very long 
> sequence chain by accident.
>
> Christian.
>
> Am 05.08.19 um 14:03 schrieb Lionel Landwerlin:
>> By any change, did you run into this with a CTS test whose name ends 
>> with ".chain" ? :)
>>
>> -Lionel
>>
>> On 05/08/2019 10:36, Christian König wrote:
>>> We can't free up the chain using recursion or we run into a stack 
>>> overflow.
>>>
>>> Manually free up the dangling chain nodes to avoid recursion.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/dma-buf/dma-fence-chain.c | 24 +++++++++++++++++++++++-
>>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence-chain.c 
>>> b/drivers/dma-buf/dma-fence-chain.c
>>> index b5089f64be2a..44a741677d25 100644
>>> --- a/drivers/dma-buf/dma-fence-chain.c
>>> +++ b/drivers/dma-buf/dma-fence-chain.c
>>> @@ -178,8 +178,30 @@ static bool dma_fence_chain_signaled(struct 
>>> dma_fence *fence)
>>>   static void dma_fence_chain_release(struct dma_fence *fence)
>>>   {
>>>       struct dma_fence_chain *chain = to_dma_fence_chain(fence);
>>> +    struct dma_fence *prev;
>>> +
>>> +    /* Manually unlink the chain as much as possible to avoid 
>>> recursion
>>> +     * and potential stack overflow.
>>> +     */
>>> +    while ((prev = rcu_dereference_protected(chain->prev, true))) {
>>> +        struct dma_fence_chain *prev_chain;
>>> +
>>> +        if (kref_read(&prev->refcount) > 1)
>>> +               break;
>>> +
>>> +        prev_chain = to_dma_fence_chain(prev);
>>> +        if (!prev_chain)
>>> +            break;
>>> +
>>> +        /* No need for atomic operations since we hold the last
>>> +         * reference to prev_chain.
>>> +         */
>>> +        chain->prev = prev_chain->prev;
>>> +        RCU_INIT_POINTER(prev_chain->prev, NULL);
>>> +        dma_fence_put(prev);
>>> +    }
>>> +    dma_fence_put(prev);
>>>   -    dma_fence_put(rcu_dereference_protected(chain->prev, true));
>>>       dma_fence_put(chain->fence);
>>>       dma_fence_free(fence);
>>>   }
>>
>>
>
>

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