linux-kernel - Re: [PATCH][next] dma-buf: Fix NULL pointer dereference in dma_fence_enable_sw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <067ee7e8-6dc3-4e84-84fe-bc00e1193848@amd.com>
Date:   Thu, 12 Oct 2023 08:48:54 +0200
From:   Christian König <christian.koenig@....com>
To:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Kees Cook <keescook@...omium.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc:     Sumit Semwal <sumit.semwal@...aro.org>,
        Gustavo Padovan <gustavo@...ovan.org>,
        Arvind Yadav <Arvind.Yadav@....com>,
        linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linaro-mm-sig@...ts.linaro.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] dma-buf: Fix NULL pointer dereference in
 dma_fence_enable_sw_signaling()



Am 11.10.23 um 18:13 schrieb Gustavo A. R. Silva:
>
>
> On 10/11/23 10:03, Kees Cook wrote:
>> On Wed, Oct 11, 2023 at 08:03:43AM -0600, Gustavo A. R. Silva wrote:
>>> Currently, a NULL pointer dereference will happen in function
>>> `dma_fence_enable_sw_signaling()` (at line 615), in case `chain`
>>> is not allocated in `mock_chain()` and this function returns
>>> `NULL` (at line 86). See below:
>>>
>>> drivers/dma-buf/st-dma-fence-chain.c:
>>>   86         chain = mock_chain(NULL, f, 1);
>>>   87         if (!chain)
>>>   88                 err = -ENOMEM;
>>>   89
>>>   90         dma_fence_enable_sw_signaling(chain);
>>
>> Instead of the larger patch, should line 88 here just do a "return
>> -ENOMEM" instead?
>
> Nope. I would have to add a `goto` to skip 
> `dma_fence_enable_sw_signaling(chain)`.
>
> I originally thought of that, but as other _signaling() functions have
> sanity-checks inside, I decided to go with that solution.
>
> This bug has been there since Sep 2022. So, adding a sanity check 
> inside that
> function should prevent any other issue of this same kind to enter the 
> codebase
> and stay there for years.

I'm trying to remove those sanity checks for years since they are hiding 
problems instead of getting them fixed.

Calling dma_fence_enable_sw_signaling with a NULL pointer is a coding 
error and not a recoverable runtime error.

The test case should be fixed instead.

Regards,
Christian.

>
> -- 
> Gustavo
>
>>
>> -Kees
>>
>>>
>>> drivers/dma-buf/dma-fence.c:
>>>   611 void dma_fence_enable_sw_signaling(struct dma_fence *fence)
>>>   612 {
>>>   613         unsigned long flags;
>>>   614
>>>   615         spin_lock_irqsave(fence->lock, flags);
>>>                    ^^^^^^^^^^^
>>>                     |
>>>               NULL pointer reference
>>>               if fence == NULL
>>>
>>>   616         __dma_fence_enable_signaling(fence);
>>>   617         spin_unlock_irqrestore(fence->lock, flags);
>>>   618 }
>>>
>>> Fix this by adding a NULL check before dereferencing `fence` in
>>> `dma_fence_enable_sw_signaling()`. This will prevent any other NULL
>>> pointer dereference when the `fence` passed as an argument is `NULL`.
>>>
>>> Addresses-Coverity: ("Dereference after null check")
>>> Fixes: d62c43a953ce ("dma-buf: Enable signaling on fence for 
>>> selftests")
>>> Cc: stable@...r.kernel.org
>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
>>> ---
>>>   drivers/dma-buf/dma-fence.c | 9 ++++++++-
>>>   include/linux/dma-fence.h   | 2 +-
>>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>> index 8aa8f8cb7071..4d2f13560d0f 100644
>>> --- a/drivers/dma-buf/dma-fence.c
>>> +++ b/drivers/dma-buf/dma-fence.c
>>> @@ -607,14 +607,21 @@ static bool 
>>> __dma_fence_enable_signaling(struct dma_fence *fence)
>>>    * This will request for sw signaling to be enabled, to make the 
>>> fence
>>>    * complete as soon as possible. This calls 
>>> &dma_fence_ops.enable_signaling
>>>    * internally.
>>> + *
>>> + * Returns 0 on success and a negative error value when @fence is 
>>> NULL.
>>>    */
>>> -void dma_fence_enable_sw_signaling(struct dma_fence *fence)
>>> +int dma_fence_enable_sw_signaling(struct dma_fence *fence)
>>>   {
>>>       unsigned long flags;
>>>   +    if (!fence)
>>> +        return -EINVAL;
>>> +
>>>       spin_lock_irqsave(fence->lock, flags);
>>>       __dma_fence_enable_signaling(fence);
>>>       spin_unlock_irqrestore(fence->lock, flags);
>>> +
>>> +    return 0;
>>>   }
>>>   EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
>>>   diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>>> index ebe78bd3d121..1e4025e925e6 100644
>>> --- a/include/linux/dma-fence.h
>>> +++ b/include/linux/dma-fence.h
>>> @@ -399,7 +399,7 @@ int dma_fence_add_callback(struct dma_fence *fence,
>>>                  dma_fence_func_t func);
>>>   bool dma_fence_remove_callback(struct dma_fence *fence,
>>>                      struct dma_fence_cb *cb);
>>> -void dma_fence_enable_sw_signaling(struct dma_fence *fence);
>>> +int dma_fence_enable_sw_signaling(struct dma_fence *fence);
>>>     /**
>>>    * dma_fence_is_signaled_locked - Return an indication if the fence
>>> -- 
>>> 2.34.1
>>>
>>>
>>