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

["Alexandre Courbot" <acourbot@nvidia.com>]

On Mon Sep 28, 2026 at 10:11 AM JST, alistair23 wrote:
> From: Alistair Francis <alistair@alistair23.me>
>
> 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.

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.

> +    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?

> +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.

> +    }
> +}
> +
> +#[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.