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 <[email protected]> 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/[email protected]?part=1
