Re: [RFC PATCH v5 1/6] rust: DmaFence: remove static lifetime

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rust: dma_fence: lack of scoped API for FenceCallbackRegistration causes UAF on leak
- [Medium] rust: missing Safety doc for unsafe fn FenceCallbackRegistration::new()
--

commit b4625b7afb0f7f8aedcbc49f28795f7e749ceb88
Author: Philipp Stanner <phasta@kernel.org>

rust: DmaFence: remove static lifetime

This commit removes the 'static lifetime bound from the FenceCallback trait
and makes FenceCallbackRegistration::new an unsafe function.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index 18a43e1bb442f..eef2b9e93f356 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs
[ ... ]
> @@ -326,7 +326,7 @@ impl<T: FenceCallback> FenceCallbackRegistration<T> {
>      /// On success the callback is pinned in place and will fire when the fence
>      /// signals. On `AlreadySignaled` the callback is returned to the caller so
>      /// that owned resources can be reclaimed.
> -    pub fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a
> +    pub unsafe fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a
>      where
>          T: 'a,
>      {

[Severity: High]
Could returning a safe impl PinInit type here create a soundness hole?

By removing the + 'static bound and making this an unsafe fn, it appears
callers are implicitly required to prevent the returned object from being
leaked to avoid a Use-After-Free.

If safe code leaks the returned object (e.g., via core::mem::forget), Drop
is never called to unregister the callback from the C backend. The C backend
can later access freed borrowed data when the fence signals.

This seems to cause a concrete Use-After-Free in a later patch in this series
(commit "rust: drm: Add JobQueue") where JobQueue::add_dependency wraps this
in a safe API:

rust/kernel/drm/job_queue.rs:Job::add_dependency() {
    ...
        // SAFETY: The registration is not forgotten. It drops with JobQueue.
        let registration = unsafe { FenceCallbackRegistration::new(fence, waker) };
    ...
}

If safe code leaks the Job via core::mem::forget(job) and then drops the
JobQueue, the callback remains registered.

Standard Rust idioms typically require a scoped API (where the callback is
unregistered at the end of a closure) or a strictly 'static bound to safely
support callbacks, since relying on leak-prevention is unsound in safe code.

[Severity: Medium]
Additionally, since this is now an unsafe fn, should it include a # Safety
documentation section explaining this strict leak-prevention requirement to
the caller?

[ ... ]
> @@ -693,7 +693,8 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
>  ///
>  /// let cb_data = CallbackData { };
>  /// let waiting_fence = ARef::from(fence.as_fence());
> -/// let cb_reg = FenceCallbackRegistration::new(&waiting_fence, cb_data);
> +/// // SAFETY: `cb_data`'s content is not forgotten.
> +/// let cb_reg = unsafe { FenceCallbackRegistration::new(&waiting_fence, cb_data) };
>  /// let cb_reg = KBox::pin_init(cb_reg, GFP_KERNEL)?;
>  ///
>  /// // TODO signalling guards

[Severity: Medium]
Is this SAFETY comment misleading?

The comment implies that the requirement is about not forgetting cb_data's
content, but the actual safety requirement is that the registration itself
must not outlive the lifetime of the borrowed data.

The registration must not be leaked so it can be properly dropped and
unregistered.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009191124.1022902-2-phasta@kernel.org?part=1