Messages in this thread | | | Date | Thu, 12 Oct 2023 08:48:54 +0200 | Subject | Re: [PATCH][next] dma-buf: Fix NULL pointer dereference in dma_fence_enable_sw_signaling() | From | Christian König <> |
| |
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 >>> >>> >>
| |