lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <067ee7e8-6dc3-4e84-84fe-bc00e1193848@amd.com> Date: Thu, 12 Oct 2023 08:48:54 +0200 From: Christian König <christian.koenig@....com> To: "Gustavo A. R. Silva" <gustavo@...eddedor.com>, Kees Cook <keescook@...omium.org>, "Gustavo A. R. Silva" <gustavoars@...nel.org> Cc: Sumit Semwal <sumit.semwal@...aro.org>, Gustavo Padovan <gustavo@...ovan.org>, Arvind Yadav <Arvind.Yadav@....com>, linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org, linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org Subject: Re: [PATCH][next] dma-buf: Fix NULL pointer dereference in dma_fence_enable_sw_signaling() 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@...r.kernel.org >>> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.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 >>> >>> >>
Powered by blists - more mailing lists