Hello, I'll put the code of debugfs wrappers in question and then elaborate.

From RFL: linux/rust/kernel/debugfs.rs at master · torvalds/linux · GitHub

The creation of Entry:

pub(crate) fn file<T>(
    name: &CStr,
    parent: &'a Entry<'_>,
    data: &'a T,
    file_ops: &FileOps<T>,
) -> Self {
    // SAFETY: The invariants of this function's arguments ensure the safety of this call.
    // * `name` is a valid C string by the invariants of `&CStr`.
    // * `parent.as_ptr()` is a pointer to a valid `dentry` because we have `&'a Entry`.
    // * `data` is a valid pointer to `T` for lifetime `'a`.
    // * The returned `Entry` has lifetime `'a`, so it cannot outlive `parent` or `data`.
    // * The caller guarantees that `vtable` is compatible with `data`.
    // * The guarantees on `FileOps` assert the vtable will be compatible with the data we have
    //   provided.
    let entry = unsafe {
        bindings::debugfs_create_file_full(
            name.as_char_ptr(),
            file_ops.mode(),
            parent.as_ptr(),
            core::ptr::from_ref(data) as *mut c_void,
            core::ptr::null(),
            &**file_ops,
        )
    };

    Entry {
        entry,
        _parent: None,
        _phantom: PhantomData,
    }
}

One of file operations:

/// Prints private data stashed in a seq_file to that seq file.
///
/// # Safety
///
/// `seq` must point to a live `seq_file` whose private data is a valid pointer to a `T` which may
/// not have any unique references alias it during the call.
unsafe extern "C" fn writer_act<T: Writer + Sync>(
    seq: *mut bindings::seq_file,
    _: *mut c_void,
) -> c_int {
    // SAFETY: By caller precondition, this pointer is valid pointer to a `T`, and
    // there are not and will not be any unique references until we are done.
    let data = unsafe { &*((*seq).private.cast::<T>()) };
    // SAFETY: By caller precondition, `seq_file` points to a live `seq_file`, so we can lift
    // it.
    let seq_file = unsafe { SeqFile::from_raw(seq) };
    seq_print!(seq_file, "{}", WriterAdapter(data));
    0
}

The Drop handler of Entry:

impl Drop for Entry<'_> {
    fn drop(&mut self) {
        // SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries.
        // `as_ptr` guarantees that the pointer is of this form.
        unsafe { bindings::debugfs_remove(self.as_ptr()) }
    }
}

So, the question. I suspect a possible violation of &T contract, leading to UAF.

Timeline:

• Execution context A has Entry
• Another execution context B runs in parallel to A
• B has pointers registered by debugfs_create_file_full.
• B invokes writer_act, &T is created.
• Assume B stops the execution in between assembly instruction.
• A calls drop on Entry, bindings::debugfs_remove(self.as_ptr()) is called on the A's stack frame, pointers are successfuly removed.
• Entry is dropped, lifetime ended, thus it is safe for data: &'a T to no longer we valid, memory location data was pointing to gets deallocated.
• B continues execution. Because it was just stopped in between assembly instructions, it is not visible to it that it was stopped and it doesn't recheck if the data pointer was or was not removed by debugfs_remove. So the moment it continued execution we already have UB where &T exists to bogus memory, and subsequent reads are UAFs.

In my mind, the drop must block until B somehow acknowledges that it is not running any callback and data isn't used. Is it the assumption I miss? Or am I missing something else?

4 posts - 3 participants

Read full topic