lib: Rust implementation of SPDM

From: Benno Lossin <[email protected]>

Add functions to make casting slices only one unsafe block.

Signed-off-by Benno Lossin <[email protected]>

Message-ID: <[email protected]>

Signed-off-by Alistair Francis <[email protected]>

---
 rust/kernel/transmute.rs | 71 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

rust/kernel/validate.rs new file +16 -0

+/// Untrusted data of type `T`.
+///
+/// Data coming from userspace is considered untrusted and should be marked by this type.
+///
+/// The particular meaning of [`Untrusted<T>`] depends heavily on the type `T`. For example,
+/// `&Untrusted<[u8]>` is a reference to an untrusted slice. But the length is not considered
+/// untrusted, as it would otherwise violate normal Rust rules. For this reason, one can easily
+/// convert that reference to `&[Untrusted<u8>]`. Another such example is `Untrusted<KVec<T>>`, it
+/// derefs to `[Untrusted<T>]`. Raw bytes however do not behave in this way, `Untrusted<u8>` is
+/// totally opaque.
+///
+/// # Usage in API Design
+///
+/// The exact location where to put [`Untrusted`] depends on the kind of API. When asking for an
+/// untrusted input value, or buffer to write to, always move the [`Untrusted`] wrapper as far
+/// inwards as possible:

rust/kernel/validate.rs new file +29 -0

+/// very easily, but the converse is not possible.
+///
+/// For the same reason, when returning untrusted data by-value, one should move the [`Untrusted`]
+/// wrapper as far outward as possible:
+///
+/// ```ignore
+/// // use this
+/// pub fn read_all_from_userspace() -> Untrusted<KVec<u8>> { todo!() }
+///
+/// // and not this
+/// pub fn read_all_from_userspace() -> KVec<Untrusted<u8>> { todo!() }
+/// ```
+///
+/// Here too the reason is that `KVec<Untrusted<u8>>` is more restrictive compared to
+/// `Untrusted<KVec<u8>>`.
+#[repr(transparent)]
+pub struct Untrusted<T: ?Sized>(T);
+
+impl<T: ?Sized> Untrusted<T> {
+    /// Marks the given value as untrusted.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::validate::Untrusted;
+    ///
+    /// # mod bindings { pub(crate) unsafe fn read_foo_info() -> [u8; 4] { todo!() } };
+    /// fn read_foo_info() -> Untrusted<[u8; 4]> {
+    ///     // SAFETY: just an FFI call without preconditions.

rust/kernel/validate.rs new file +10 -0

+    }
+}
+
+impl<T, A: Allocator> Deref for Untrusted<Vec<T, A>> {
+    type Target = [Untrusted<T>];
+
+    #[inline]
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: `Untrusted<T>` transparently wraps `T`.
+        unsafe { cast_slice(self.0.as_slice()) }

From: Alistair Francis <[email protected]>

This is the initial commit of the Rust SPDM library.

lib/rspdm/consts.rs new file +0 -0

new file mode 100644
index 000000000000..9709f51849d7

lib/rspdm/consts.rs new file +4 -0

+
+// SPDM versions supported by this implementation
+pub(crate) const SPDM_VER_10: u8 = 0x10;
+

lib/rspdm/lib.rs new file +38 -0

new file mode 100644
index 000000000000..1010bd38df6d
@@ -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;
+

lib/rspdm/state.rs new file +38 -0

new file mode 100644
index 000000000000..c894cf30bb87
@@ -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.

lib/rspdm/state.rs new file +17 -0

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

lib/rspdm/state.rs new file +11 -0

+        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);

lib/rspdm/state.rs new file +16 -0

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

Quoted 3 lines · On Mon Sep 28, 2026 at 10:11 AM JST, alistair23 wrote:

From: Alistair Francis <[email protected]>

This is the initial commit of the Rust SPDM library.

This commit message deserves to be a bit more verbose. :)

<...>

Quoted 6 lines

lib/rspdm/consts.rs new file +0 -0

new file mode 100644
index 000000000000..9709f51849d7

lib/rspdm/consts.rs line 10

Quoted 10 lines

lib/rspdm/consts.rs new file +10 -0

@@ -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::*;.

Quoted 4 lines

lib/rspdm/consts.rs new file +4 -0

+
+// SPDM versions supported by this implementation
+pub(crate) const SPDM_VER_10: u8 = 0x10;
+

lib/rspdm/consts.rs line 24

Quoted 10 lines

lib/rspdm/consts.rs new file +10 -0

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

<...>

Quoted 44 lines

lib/rspdm/lib.rs new file +38 -0

new file mode 100644
index 000000000000..1010bd38df6d
@@ -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;
+

lib/rspdm/lib.rs line 48

Quoted 10 lines

lib/rspdm/lib.rs new file +10 -0

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

<...>

Quoted 44 lines

lib/rspdm/state.rs new file +38 -0

new file mode 100644
index 000000000000..c894cf30bb87
@@ -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.

lib/rspdm/state.rs line 48

Quoted 10 lines

lib/rspdm/state.rs new file +10 -0

+///
+/// `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

lib/rspdm/state.rs line 51

Quoted 3 lines

lib/rspdm/state.rs new file +3 -0

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

lib/rspdm/state.rs line 53

Quoted 2 lines

lib/rspdm/state.rs new file +2 -0

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

Quoted 18 lines

lib/rspdm/state.rs new file +17 -0

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

lib/rspdm/state.rs line 80

Quoted 10 lines

lib/rspdm/state.rs new file +10 -0

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

Quoted 11 lines

lib/rspdm/state.rs new file +11 -0

+        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);

lib/rspdm/state.rs line 101

Quoted 10 lines

lib/rspdm/state.rs new file +10 -0

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

<...>

lib/rspdm/state.rs line 194

Quoted 8 lines

lib/rspdm/state.rs new file +8 -0

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

lib/rspdm/state.rs line 121

Quoted 13 lines

lib/rspdm/state.rs new file +12 -0

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

Quoted 17 lines

lib/rspdm/state.rs new file +16 -0

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

lib/rspdm/state.rs line 147

Quoted 10 lines

lib/rspdm/state.rs new file +10 -0

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

Quoted 23 lines

Quoted 6 lines

lib/rspdm/state.rs new file +6 -0

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

lib/rspdm/state.rs line 48

Quoted 10 lines

lib/rspdm/state.rs new file +10 -0

+///
+/// `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

Quoted 11 lines

lib/rspdm/state.rs line 51

Quoted 3 lines

lib/rspdm/state.rs new file +3 -0

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

Quoted 11 lines

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.

Quoted 2 lines

lib/rspdm/state.rs new file +2 -0

+    pub(crate) transport: bindings::spdm_transport,
+    pub(crate) transport_priv: *mut c_void,

Quoted 3 lines

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.

Quoted 15 lines

lib/rspdm/validator.rs line 66

Quoted 13 lines

lib/rspdm/validator.rs new file +12 -0

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

On Thu Oct 15, 2026 at 12:06 PM JST, Alistair Francis wrote:
<...>

Quoted 35 lines

Quoted 34 lines

Quoted 6 lines

lib/rspdm/state.rs new file +6 -0

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

lib/rspdm/state.rs line 48

Quoted 10 lines

lib/rspdm/state.rs new file +10 -0

+///
+/// `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

lib/rspdm/state.rs line 51

Quoted 3 lines

lib/rspdm/state.rs new file +3 -0

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

Quoted 1 line

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

Another related point of feedback is that this code seems to be designed to interact with C drivers exclusively - i.e. I don't see a publicly exported Rust API.

That's probably what has guided the current design of storing pointers, as that's what the C functions expect; and the C side is also responsible for taking care of lifetime and bound device status, so in that context using a &Device<Bound> will be of little help as it will need to be forged by trusting that the calling C code knows what it is doing.

I think it would be helpful on the contrary to design things around a Rust API, on top of which the C FFI is built. That way you would have to work with properly bound references, and have a proof that the model is viable before exposing it to C. Also I guess Rust drivers will want to use this at some point, so better design with that in mind from the get-go.

Also related, what is the reason for having this code in lib? Why not make this a regular kernel module?

Quoted 12 lines

Quoted 12 lines

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.

Quoted 2 lines

lib/rspdm/state.rs new file +2 -0

+    pub(crate) transport: bindings::spdm_transport,
+    pub(crate) transport_priv: *mut c_void,

Quoted 7 lines

Quoted 3 lines

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.

I suspect something close to what fwctl does? Basically SpdmState would store a reference to a generic type, which outlives the SPDM driver bound time. That's also why having the Rust API first is helpful, as it will provide the lifetime that needs to be outlived.

Looking at the example in cma.c, the private data appears to be host.doe_mb, which exists before spdm_create is called and is still there after spdm_destroy, so that seems to be the correct model, i.e. there should be no need to transfer full ownership of the private data to the SPDM driver - a reference should be sufficient.

Actually thinking some more about it seems like the fwctl Rust bindings could be a good model to follow. They are simple and adopt a similar pattern of a sub-component of a larger driver that never outlives its parent.

<...>

Quoted 34 lines

Quoted 15 lines

lib/rspdm/validator.rs line 66

Quoted 13 lines

lib/rspdm/validator.rs new file +12 -0

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

Quoted 2 lines

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

For more complex structures sure, but in the case of SpdmHeader zerocopy looks like it would work nicely? It also supports performing a copy (see read_from_bytes and read_from_prefix).

That was kind of the goal with SpdmState. The issue is all of the drivers calling this (PCIe, NVMe, SATA and SCSI) are C drivers and won't be Rust for the foreseeable future. So there aren't any Rust users.

I have improved things a lot with v5, let me know if you still want a Rust API. It just feels a bit silly to add one that will probably never be used.

Quoted 9 lines · Alistair Francis

That was kind of the goal with SpdmState. The issue is all of the drivers calling this (PCIe, NVMe, SATA and SCSI) are C drivers and won't be Rust for the foreseeable future. So there aren't any Rust users.

I have improved things a lot with v5, let me know if you still want a Rust API. It just feels a bit silly to add one that will probably never be used.

I mean, it is true that we don't want dead code, but in general we would also want more Rust callers in the future -- so I guess it depends on that "probably" you mention, i.e. how likely it is. So could you please elaborate? i.e. why is it that you don't expect future Rust callers? Would the NVMe Rust driver want to use this eventually if merged? (Cc'ing Andreas)

Quoted 11 lines · Miguel Ojeda

Quoted 9 lines · Alistair Francis

That was kind of the goal with SpdmState. The issue is all of the drivers calling this (PCIe, NVMe, SATA and SCSI) are C drivers and won't be Rust for the foreseeable future. So there aren't any Rust users.

I have improved things a lot with v5, let me know if you still want a Rust API. It just feels a bit silly to add one that will probably never be used.

Quoted 6 lines

I mean, it is true that we don't want dead code, but in general we would also want more Rust callers in the future -- so I guess it depends on that "probably" you mention, i.e. how likely it is. So could you please elaborate? i.e. why is it that you don't expect future Rust callers? Would the NVMe Rust driver want to use this eventually if merged? (Cc'ing Andreas)

Yeah the "probably never be used" part sounds a bit strong to me, unless there is a reason I missed for why no Rust driver would ever want to use SPDM.

Regardless of that, I think it still makes sense to have a layered API design where the (necessarily unsafe) C API builds on top of a safe Rust API that enforces the invariants and other rules you want to uphold. That makes the invariants that C callers need to uphold manually clear, and avoids having C-isms in the Rust code (like we had with the crypto APIs calls in v4). Not to mention my favorite: it makes things easier to review. :)