Re: [PATCH v4 10/22] lib: rspdm: Initial commit of Rust SPDM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rust: ForeignOwnable type mismatch in spdm_destroy()
- [Low] rspdm: needless mutable request_buf in spdm_exchange()
--

commit 3c4db293fc6c18056667fde06984c7043ab7fc64
Author: Alistair Francis <alistair@alistair23.me>

lib: rspdm: Initial commit of Rust SPDM

This commit adds the initial Rust implementation of the Security Protocol and
Data Model (SPDM) library. It introduces session allocation, authentication
stubs, and session destruction functions exposed via a C API.

> diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs
> new file mode 100644
> --- /dev/null
> +++ b/lib/rspdm/lib.rs
[ ... ]
> +pub extern "C" fn spdm_create(
> +    dev: *mut bindings::device,
> +    transport: bindings::spdm_transport,
> +    transport_priv: *mut c_void,
> +    transport_sz: u32,
> +    validate: bindings::spdm_validate,
> +) -> *mut spdm_state {
> +    // Wrap the `SpdmState` in a `Mutex` so that concurrent FFI callers (for
> +    // example, two threads racing on `spdm_authenticate()` for the same
> +    // device) serialize on the lock and never form aliased `&mut SpdmState`
> +    // references.
> +    let state = SpdmState::new(dev, transport, transport_priv, transport_sz, validate);
> +    match KBox::pin_init(new_mutex!(state), flags::GFP_KERNEL) {
> +        Ok(b) => b.into_foreign() as *mut spdm_state,
> +        Err(_) => ptr::null_mut(),
> +    }
> +}
[ ... ]
> +pub extern "C" fn spdm_destroy(state_ptr: *mut spdm_state) {
> +    if state_ptr.is_null() {
> +        return;
> +    }
> +
> +    // SAFETY: `state_ptr` was returned from `spdm_create()` which used `into_foreign()`
> +    // to create the pointer.
> +    let mutex: KBox<Mutex<SpdmState>> = unsafe { KBox::from_foreign(state_ptr as *mut c_void) };

[Severity: High]
Does this break the ForeignOwnable safety contract? 

The spdm_create() function converts a Pin<KBox<Mutex<SpdmState>>> into a raw
pointer via into_foreign(), but here in spdm_destroy(), the pointer is
reconstructed as an unpinned KBox<Mutex<SpdmState>>. 

Reconstructing a different type from a raw pointer breaks the strict type
symmetry required by the ForeignOwnable trait and violates Pin invariants.
Should this be reconstructed as a pinned box to match its original allocation?

> +
> +    drop(mutex);
> +}

[ ... ]
> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> new file mode 100644
> --- /dev/null
> +++ b/lib/rspdm/state.rs
[ ... ]
> +    pub(crate) fn spdm_exchange(
> +        &self,
> +        request_buf: &mut [u8],

[Severity: Low]
Is it necessary to require a mutable slice for the request buffer here?

The function only appears to use its read-only pointer to pass to the
transport layer:

lib/rspdm/state.rs:spdm_exchange() {
    ...
    let length = unsafe {
        transport_function(
            ...
            request_buf.as_ptr() as *const c_void,
            ...
    ...
}

This enforces mutable access to a buffer that is never modified, creating
unnecessary constraints on callers by forcing them to hold or provide
mutable references to read-only buffers. Could this be changed to &[u8]?

> +        response_buf: &mut [u8],
> +    ) -> Result<i32, Error> {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928011123.450800-1-alistair.francis@wdc.com?part=10