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(-)
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
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
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
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
linaro-mm-sig@lists.linaro.org
-
Christian König -
Gustavo A. R. Silva -
Gustavo A. R. Silva -
Kees Cook