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@xxxxxxxxxxxxxxx
Signed-off-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
---
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
