Re: [PATCH v5 09/23] lib: rspdm: Initial commit of Rust SPDM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] rspdm: un-sliced request_buf in spdm_exchange() fails SpdmHeader validation
- [Low] lib: rspdm: missing unsafe qualifier on spdm_destroy() and spdm_authenticate()
- [Low] lib: rspdm: non-vertical import and core::ffi usage in lib.rs
--

commit 8332eb50f9f3f308b55adfa6b54fbbcfc9566854
Author: Alistair Francis <alistair.francis@wdc.com>

lib: rspdm: Initial commit of Rust SPDM

This commit introduces the initial framework for the Rust Security Protocol
and Data Model (SPDM) library, including C FFI entry points, session state
management, and message header validation.

> diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs
> new file mode 100644
> index 0000000000000..27449ac1ca25d
> --- /dev/null
> +++ b/lib/rspdm/lib.rs
> @@ -0,0 +1,263 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2026 Western Digital
> +
> +//! Top level library for SPDM
> +//!
> +//! Rust implementation of the DMTF Security Protocol and Data Model (SPDM)
> +//! <https://www.dmtf.org/dsp/DSP0274>
> +//!
> +//! Top level library, including C compatible public functions to be called
> +//! from other subsytems.
> +
> +use crate::bindings::{
> +    spdm_state,
> +    EPROTONOSUPPORT, //
> +};
> +use core::ffi::{
> +    c_int,
> +    c_void, //
> +};
> +use core::ptr;
> +use kernel::prelude::*;
> +use kernel::{
> +    alloc::flags,
> +    bindings,
> +    device::{
> +        Bound,
> +        Device, //
> +    },
> +    error::to_result,
> +    new_mutex,
> +    sync::Mutex,
> +    types::ForeignOwnable, //
> +};
> +
> +use crate::state::{SpdmState, Transport};

[Severity: Low]
This isn't a bug, but per Documentation/rust/coding-guidelines.rst, should
c_int and c_void come from kernel::prelude::* rather than core::ffi, and
should the crate::state import use the vertical import style?

[ ... ]

> +/// Authenticates a device through a sequence of SPDM exchanges.
> +///
> +/// See `include/linux/spdm.h` for the full C API documentation.
> +#[export]
> +pub extern "C" fn spdm_authenticate(_state_ptr: *mut spdm_state) -> c_int {
> +    -(EPROTONOSUPPORT as i32)
> +}
> +
> +/// Destroys an SPDM session.
> +///
> +/// See `include/linux/spdm.h` for the full C API documentation.
> +#[export]
> +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 = unsafe {
> +        <Pin<KBox<Mutex<SpdmState<'_, CTransport>>>> as ForeignOwnable>::from_foreign(
> +            state_ptr.cast::<c_void>(),
> +        )
> +    };
> +
> +    drop(mutex);
> +}

[Severity: Low]
Should spdm_destroy() and spdm_authenticate() be marked unsafe extern "C" fn
with # Safety documentation sections, matching spdm_create()?

In spdm_destroy(), when state_ptr is non-null, it is passed to
ForeignOwnable::from_foreign(), which requires the caller to guarantee that
the pointer was returned by a previous call to into_foreign() and is not
reclaimed more than once:

rust/kernel/types.rs:ForeignOwnable::from_foreign() {
    /// # Safety
    ///
    /// The provided pointer must have been returned by a previous call to [`into_foreign`], and it
    /// must not be passed to `from_foreign` more than once.
    unsafe fn from_foreign(ptr: *mut c_void) -> Self;
}

Similarly, spdm_authenticate() takes a raw *mut spdm_state pointer from C
that later patches in this series pass to ForeignOwnable::borrow(). Without
an unsafe qualifier on these functions, their internal SAFETY comments rely
on caller preconditions that are not part of a safety contract.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> new file mode 100644
> index 0000000000000..f864d869eff0c
> --- /dev/null
> +++ b/lib/rspdm/state.rs
> @@ -0,0 +1,145 @@

[ ... ]

> +    /// Start a SPDM exchange
> +    ///
> +    /// The data in `request_buf` is sent to the device and the response
> +    /// is returned as untrusted data to be validated by the caller.
> +    #[allow(dead_code)]
> +    pub(crate) fn spdm_exchange(
> +        &self,
> +        request_buf: &[u8],
> +        mut response_buf: KVec<u8>,
> +    ) -> Result<Untrusted<KVec<u8>>, Error> {
> +        let header_size = core::mem::size_of::<SpdmHeader>();
> +        let request: SpdmHeader = Untrusted::new(request_buf).validate(self)?;

[Severity: High]
Will SpdmState::spdm_exchange() fail with EPROTO for any SPDM request
message that has a payload after the 4-byte SpdmHeader?

Here, the full request_buf slice is passed to SpdmHeader::validate() without
slicing it to ..header_size (unlike response_buf.get(..header_size) below).
In lib/rspdm/validator.rs, SpdmHeader::validate() calls
SpdmHeader::read_from_bytes(unvalidated):

lib/rspdm/validator.rs:<SpdmHeader as Validate>::validate() {
    fn validate(unvalidated: &[u8], _context: &'c SpdmState<'c, T>) -> Result<Self, Self::Err> {
        SpdmHeader::read_from_bytes(unvalidated).or(Err(EPROTO))
    }
}

Because zerocopy's FromBytes::read_from_bytes() enforces exact length
equality via Ref::sized_from():

rust/zerocopy/src/ref.rs:Ref::sized_from() {
    if bytes.len() != mem::size_of::<T>() {
        return Err(SizeError::new(bytes).into());
    }
}

any request_buf longer than 4 bytes is rejected with Err(EPROTO) before
self.transport.exchange() is ever called. In the subsequent patches in this
series (such as get_capabilities, negotiate_algorithms, get_certificate, and
challenge), requests with payloads beyond the 4-byte header are passed to
spdm_exchange(), causing spdm_authenticate() to fail unconditionally once it
moves past the 4-byte GET_VERSION request.

Should request_buf be sliced to ..header_size before validating the request
header, or should SpdmHeader::validate() read from the prefix of the slice?

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