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);
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: [email protected]
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
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
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?
-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: [email protected]
> Signed-off-by: Gustavo A. R. Silva <[email protected]>
> ---
> 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
>
>
--
Kees Cook
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.
--
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: [email protected]
>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
>> ---
>> 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
>>
>>
>
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: [email protected]
>>> Signed-off-by: Gustavo A. R. Silva <[email protected]>
>>> ---
>>> 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
>>>
>>>
>>