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

[Alistair Francis]

On Thu, Oct 15, 2026 at 1:17 AM Alexandre Courbot <acourbot@xxxxxxxxxx> wrote:
>
> On Mon Sep 28, 2026 at 10:11 AM JST, alistair23 wrote:
> > From: Alistair Francis <alistair@xxxxxxxxxxxxx>
> >
> > This is the initial commit of the Rust SPDM library.
>
> This commit message deserves to be a bit more verbose. :)
>
> <...>
> > diff --git a/lib/rspdm/consts.rs b/lib/rspdm/consts.rs
> > new file mode 100644
> > index 000000000000..9709f51849d7
> > --- /dev/null
> > +++ b/lib/rspdm/consts.rs
> > @@ -0,0 +1,92 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +// Copyright (C) 2026 Western Digital
> > +
> > +//! Constants used by the library
> > +//!
> > +//! Rust implementation of the DMTF Security Protocol and Data Model (SPDM)
> > +//! <https://www.dmtf.org/dsp/DSP0274>
> > +
> > +use kernel::error::{code::EINVAL, Error};
>
> This can just be `use kernel::prelude::*;`.
>
> > +
> > +// SPDM versions supported by this implementation
> > +pub(crate) const SPDM_VER_10: u8 = 0x10;
> > +
> > +pub(crate) const SPDM_MIN_VER: u8 = SPDM_VER_10;
> > +
> > +#[allow(dead_code)]
> > +pub(crate) const SPDM_REQ: u8 = 0x80;
> > +#[allow(dead_code)]
> > +pub(crate) const SPDM_ERROR: u8 = 0x7f;
> > +
> > +#[derive(Clone, Copy)]
> > +#[repr(u8)]
> > +pub(crate) enum SpdmErrorCode {
>
> This is in `consts.rs` but is not a const... Actually everything in this
> module looks like it should be in `lib.rs`, and by doing so you could
> also remove the `pub(crate)` visibility.
>
> <...>
> > diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs
> > new file mode 100644
> > index 000000000000..1010bd38df6d
> > --- /dev/null
> > +++ b/lib/rspdm/lib.rs
> > @@ -0,0 +1,96 @@
> > +// 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,
> > +    new_mutex,
> > +    sync::Mutex,
> > +    types::ForeignOwnable, //
> > +};
> > +
> > +use crate::state::SpdmState;
> > +
> > +const __LOG_PREFIX: &[u8] = b"spdm\0";
> > +
> > +mod consts;
> > +mod state;
> > +mod validator;
> > +
> > +/// spdm_create() - Allocate SPDM session
> > +///
> > +/// `dev`: Responder device
> > +/// `transport`: Transport function to perform one message exchange
> > +/// `transport_priv`: Transport private data
> > +/// `transport_sz`: Maximum message size the transport is capable of (in bytes)
> > +/// `validate`: Function to validate additional leaf certificate requirements
> > +///  (optional, may be %NULL)
> > +///
> > +/// Return a pointer to the allocated SPDM session state or NULL on error.
>
> The commenting style is a bit all over the place in this series,
> sometimes using the Rust style, sometimes the C kernel style, sometimes
> apparently something else.
>
> For Rust items, please follow the Rustdoc style. These functions though
> are the C FFI interface though, so they are useful to the C callers and
> as such I think their documentation belongs to the interface file (i.e.
> `include/linux/spdm.h`).
>
> <...>
> > diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> > new file mode 100644
> > index 000000000000..c894cf30bb87
> > --- /dev/null
> > +++ b/lib/rspdm/state.rs
> > @@ -0,0 +1,241 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +// Copyright (C) 2026 Western Digital
> > +
> > +//! The `SpdmState` struct and implementation.
> > +//!
> > +//! Rust implementation of the DMTF Security Protocol and Data Model (SPDM)
> > +//! <https://www.dmtf.org/dsp/DSP0274>
> > +
> > +use core::ffi::c_void;
> > +use kernel::prelude::*;
> > +use kernel::{
> > +    bindings,
> > +    error::{
> > +        code::EINVAL,
> > +        to_result,
> > +        Error, //
> > +    },
> > +    validate::Untrusted,
> > +};
> > +
> > +use crate::consts::{
> > +    SpdmErrorCode,
> > +    SPDM_ERROR,
> > +    SPDM_MIN_VER,
> > +    SPDM_REQ, //
> > +};
> > +use crate::validator::{
> > +    SpdmErrorRsp,
> > +    SpdmHeader, //
> > +};
> > +
> > +/// The current SPDM session state for a device.
> > +///
> > +/// Concurrent access is serialised by wrapping the whole struct in a
> > +/// `Mutex<SpdmState>` at the FFI boundary, so `spdm_authenticate()` callers
> > +/// run one at a time and the locked `&mut SpdmState` is the only way to
> > +/// reach the inner fields.
> > +///
> > +/// `dev`: Responder device.  Used for error reporting and passed to @transport.
> > +/// `transport`: Transport function to perform one message exchange.
> > +/// `transport_priv`: Transport private data.
> > +/// `transport_sz`: Maximum message size the transport is capable of (in bytes).
> > +///  Used as DataTransferSize in GET_CAPABILITIES exchange.
> > +/// `validate`: Function to validate additional leaf certificate requirements.
> > +///
> > +/// `version`: Maximum common supported version of requester and responder.
> > +///  Negotiated during GET_VERSION exchange.
>
> Field documentation should be their own doccomments before each
> individual field (see [1] for an example and general good practices when
> writing Rust doccomments).
>
> [1] https://doc.rust-lang.org/rust-by-example/meta/doc.html
>
> > +#[expect(dead_code)]
> > +pub(crate) struct SpdmState {
> > +    pub(crate) dev: *mut bindings::device,
>
> This makes me very nervous. I see this is passed to C callbacks. We need
> to maintain the guarantee that the device still exists, so an
> `ARef<Device>`, from which you obtain a pointer to pass to the C
> callbacks, should be the minimum here. Does the device also need to be
> bound? Looking at `drivers/pci/cma.c` it seems to be. Then this needs to
> be a `&Device<Bound>`, with a lifetime to `SpdmState` so callers only
> use it while the device is bound.

Good point, I have converted this to use a `&Device<Bound>`

>
> Just storing a C pointer and passing it whenever a function needs it
> cancels the purpose of using Rust and make the safety requirements of
> the callback you pass it to impossible to guarantee, as nothing enforces
> that the device still exists or is in a valid state when they are
> invoked.
>
> > +    pub(crate) transport: bindings::spdm_transport,
> > +    pub(crate) transport_priv: *mut c_void,
>
> Here I also suspect we want to define some kind of ownership model for
> the private data instead of hoping that the pointer is still valid when
> we use it.

Any ideas here? It's just a pointer that we pass back to C when
calling the `transport_function()`, I'm not clear on what we could do
to ensure it's still valid.

>
> > +    pub(crate) transport_sz: u32,
> > +    pub(crate) validate: bindings::spdm_validate,
> > +
> > +    // Negotiated state
> > +    pub(crate) version: u8,
> > +}
> > +
> > +impl SpdmState {
> > +    pub(crate) fn new(
> > +        dev: *mut bindings::device,
> > +        transport: bindings::spdm_transport,
> > +        transport_priv: *mut c_void,
> > +        transport_sz: u32,
> > +        validate: bindings::spdm_validate,
> > +    ) -> Self {
> > +        SpdmState {
> > +            dev,
> > +            transport,
> > +            transport_priv,
> > +            transport_sz,
> > +            validate,
> > +            version: SPDM_MIN_VER,
> > +        }
> > +    }
> > +
> > +    #[allow(dead_code)]
> > +    fn spdm_err(&self, rsp: &SpdmErrorRsp) -> Result<(), Error> {
>
> `&self` is never used - this should probably be a method of
> `SpdmErrorRsp`.
>
> > +        match rsp.error_code {
> > +            SpdmErrorCode::InvalidRequest => {
> > +                pr_err!("Invalid request\n");
> > +                Err(EINVAL)
> > +            }
> > +            SpdmErrorCode::InvalidSession => {
> > +                if rsp.version == 0x11 {
> > +                    pr_err!("Invalid session {:#x}\n", rsp.error_data);
> > +                    Err(EINVAL)
> > +                } else {
> > +                    pr_err!("Undefined error {:#x}\n", rsp.error_code);
> > +                    Err(EINVAL)
> > +                }
> > +            }
> > +            SpdmErrorCode::Busy => {
> > +                pr_err!("Busy\n");
> > +                Err(EBUSY)
> > +            }
> > +            SpdmErrorCode::UnexpectedRequest => {
> > +                pr_err!("Unexpected request\n");
> > +                Err(EINVAL)
>
> These blocks are getting repetitive. Instead you can implement `Debug`
> on `SpdmErrorCode` and `From<SpdmErrorCode> for Error`, and get rid of
> this method entirely as callers will be able to print the error directly
> and have it automatically converted to the kernel error type.
>
> <...>
> > +    /// Start a SPDM exchange
> > +    ///
> > +    /// The data in `request_buf` is sent to the device and the response is
> > +    /// stored in `response_buf`.
> > +    #[allow(dead_code)]
> > +    pub(crate) fn spdm_exchange(
> > +        &self,
> > +        request_buf: &mut [u8],
>
> This shouldn't need to be mutable?

Nope, it doesn't need to be. That's just leftover from earlier versions.

>
> > +impl<'a, 'c> Validate<'c, Untrusted<&'a [u8]>> for SpdmHeader {
> > +    type Err = Error;
> > +
> > +    type Context = &'c SpdmState;
> > +
> > +    fn validate(unvalidated: &[u8], _context: &'c SpdmState) -> Result<Self, Self::Err> {
> > +        Ok(SpdmHeader {
> > +            version: *unvalidated.get(0).ok_or(EIO)?,
> > +            code: *unvalidated.get(1).ok_or(EIO)?,
> > +            param1: *unvalidated.get(2).ok_or(EIO)?,
> > +            param2: *unvalidated.get(3).ok_or(EIO)?,
> > +        })
>
> Here you will definitely want to leverage zerocopy's `FromBytes` trait.

I was hoping not too actually.

The SPDM responses are a bit complex. For example NegotiateAlgsRsp has
multiple dynamically sized arrays, and the fields change based on the
version.

Using zerocopy means we need packed structs and even then I'm not
clear on how we can handle the KVec<u32> or even the
KVec<RespAlgStruct> nicley.

The current approach has the obvious downside of copying the data, but
as the data is pretty small in the scheme of things and not part of a
hot path, I think the tradeoff of copying the data with much cleaner
and easier to understand code is worth it.

Maybe in the future zerocopy could be a performance improvement, but I
think right now it's a large complexity overhead in this case.

Note that obviously SpdmHeader is not complex, but I think they should
all follow the same pattern.

Alistair

>
> > +    }
> > +}
> > +
> > +#[expect(dead_code)]
> > +pub(crate) struct SpdmErrorRsp {
> > +    pub(crate) version: u8,
> > +    /// This will always be SPDM_ERROR (0x7F)
> > +    pub(crate) code: u8,
> > +    pub(crate) error_code: SpdmErrorCode,
> > +    pub(crate) error_data: u8,
> > +}
> > +
> > +impl<'a, 'c> Validate<'c, Untrusted<&'a [u8]>> for SpdmErrorRsp {
> > +    type Err = Error;
> > +
> > +    type Context = &'c SpdmState;
> > +
> > +    fn validate(unvalidated: &[u8], _context: &'c SpdmState) -> Result<Self, Self::Err> {
> > +        Ok(SpdmErrorRsp {
> > +            version: *unvalidated.get(0).ok_or(EIO)?,
> > +            code: *unvalidated.get(1).ok_or(EIO)?,
> > +            // `try_from` rejects unknown `SpdmErrorCode` discriminants.
> > +            error_code: SpdmErrorCode::try_from(*unvalidated.get(2).ok_or(EIO)?)?,
> > +            error_data: *unvalidated.get(3).ok_or(EIO)?,
> > +        })
> > +    }
>
> Same here.