diff mbox series

[next] dma-buf: Fix NULL pointer dereference in dma_fence_enable_sw_signaling()

Message ID ZSarP0/+hG8/87//@work (mailing list archive)
State Changes Requested
Headers show
Series [next] dma-buf: Fix NULL pointer dereference in dma_fence_enable_sw_signaling() | expand

Commit Message

Gustavo A. R. Silva Oct. 11, 2023, 2:03 p.m. UTC
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: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/dma-buf/dma-fence.c | 9 ++++++++-
 include/linux/dma-fence.h   | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

Comments

Kees Cook Oct. 11, 2023, 4:03 p.m. UTC | #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: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.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
> 
>
Gustavo A. R. Silva Oct. 11, 2023, 4:13 p.m. UTC | #2
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: stable@vger.kernel.org
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.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
>>
>>
>
Christian König Oct. 12, 2023, 6:48 a.m. UTC | #3
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@vger.kernel.org
>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.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
>>>
>>>
>>
diff mbox series

Patch

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