Re: [PATCH v4 00/22] lib: Rust implementation of SPDM

[Alistair Francis]

On Thu, Oct 15, 2026 at 6:54 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
>
> On Mon, Sep 28, 2026 at 11:11:01AM +1000, alistair23@xxxxxxxxx wrote:
> > From: Alistair Francis <alistair.francis@xxxxxxx>
> >
> > Security Protocols and Data Models (SPDM) [1] is used for authentication,
> > attestation and key exchange. SPDM is generally used over a range of
> > transports, such as PCIe, MCTP/SMBus/I3C, ATA, SCSI, NVMe or TCP.
> >
> > From the kernels perspective SPDM is used to authenticate and attest devices.
> > In this threat model a device is considered untrusted until it can be verified
> > by the kernel and userspace using SPDM. As such SPDM data is untrusted data
> > that can be mallicious.
> >
> > The SPDM specification is also complex, with the 1.2.1 spec being almost 200
> > pages and the 1.3.0 spec being almost 250 pages long.
> >
> > As such we have the kernel parsing untrusted responses from a complex
> > specification, which sounds like a possible exploit vector. This is the type
> > of place where Rust excels!
> >
> > This series implements a SPDM requester in Rust.
>
> So I looked a bit at this, and here are some comments. I will not repeat
> what Alexandre has already said.
>
> First, I'd like to say that it's good to see that all wire-format
> parsing in lib/rspdm/validator.rs is written in completely safe Rust.
> That's exactly what we want to see for parsing in Rust. We want the code
> to have basically no unsafe code whatsoever so that we really get to
> rely on the bullet-proof guarantees about bugs being caught at compile
> time, or at least at runtime.
>
> Unfortunately it looks like the same cannot be said for the state
> machine in lib/rspdm/state.rs, which has way too much unsafe code in its
> interaction with the rest of the kernel. There needs to be more
> intermediate Rust abstractions here. This file should not have *any*
> unsafe code, except maybe 1-2 blocks for using the C transport and
> validate function pointers.

Yeah, it's the crypto functions. There are currently no bindings, so
it's just directly calling the C functions.

Without the crypto functions it's just a single unsafe call to
`transport_function()` in `state.rs` and a few in `lib.rs` to handl
the C <-> FFI boundy.

I was trying to avoid adding crypto and X.509 bindings in this series,
as that's a few more subsystems this ends up touching. We need to add
PQC as well, so crypto bindings are next on the todo list once this is
merged.

Is this going to stall on adding crypto bindings?

>
> I've also asked my LLM to take a look, please see below. I looked over
> the LLM report myself, and while it's more strongly worded than I would
> have said myself, its feedback seems more or less valid to me.

Ha, it is pretty strongly worded. I think I addressed everything below

>
> Alice
>
>
>
>
> #### 1. Bugs & Memory Safety / Soundness Issues
>
> - **UB in `shash_desc` allocation and deallocation in `SpdmState::update_response_algs` and `SpdmState::free_desc` (Patch 17)**:
>   - `update_response_algs` allocates a `KVec<u8>` via `KVec::from_elem(0u8, desc_len, GFP_KERNEL)`, calls `into_raw_parts()`, discards `_capacity`, and stores `desc_buf as *mut bindings::shash_desc`.
>   - In `free_desc`, it reconstructs `KVec::<u8>::from_raw_parts(self.desc as *mut u8, desc_len, desc_len)`, passing `desc_len` as the capacity even though `KVec::from_elem` does not guarantee `capacity == len`.
>   - Casting a `*mut u8` from `KVec<u8>` to `*mut bindings::shash_desc` only relies on `kmalloc` minimum alignment by accident, rather than properly allocating a dynamically-sized type or wrapping `crypto_shash` in a safe kernel abstraction.

Good point, fixed

>
> - **Mismatched `ForeignOwnable` types in `lib/rspdm/lib.rs` (Patch 10)**:
>   - `spdm_create` creates a `Pin<KBox<Mutex<SpdmState<'_>>>>` via `KBox::pin_init` and calls `.into_foreign()`.
>   - `spdm_authenticate` borrows it as `<Pin<KBox<Mutex<SpdmState<'_>>>> as ForeignOwnable>::borrow(...)`.
>   - `spdm_destroy`, however, calls `KBox::<Mutex<SpdmState<'_>>>::from_foreign(...)` (without `Pin`). It should use `Pin::<KBox<Mutex<SpdmState<'_>>>>::from_foreign(...)` to match `into_foreign()`.

Shasiko caught this one

>
> - **Unchecked transport return length in `spdm_exchange` and callers**:
>   - In `spdm_exchange`, `Untrusted::new(&response_buf[..]).validate(&*self)` validates the entire pre-allocated `response_buf` instead of `&response_buf[..length as usize]`.
>   - Only `get_version` checks `if rc > response_vec.len()` before `response_vec.truncate(rc)`. None of `get_capabilities`, `negotiate_algs`, `get_digests`, `get_cert_exchange`, or `challenge` check if `rc > response_vec.len()`; if a transport returns a size larger than the buffer, `truncate(rc)` is a no-op. Better yet, `spdm_exchange` itself should enforce `(length as usize) <= response_buf.len()` and return `usize`.

The next version handles this better, but I don't think this is an
actual issue. If `transport_function` returns a wrong length we will
just check the entire `response_buf`. It's not a soundness or safety
issue, just a logic bug in `transport_function`. That's just up to
`transport_function` to tell us how much data we received.

>
> #### 2. Design & Abstraction Issues
>
> - **`Untrusted<T>` / `Validate` Usage Defeats Its Own Purpose (Patches 3–4, 10, 15–22)**:
>   - The motivation for `Untrusted<T>` is that I/O boundaries (such as `spdm_exchange`) accept or return `Untrusted` data so the caller cannot touch the bytes without going through `Validate`.
>   - Instead, `spdm_exchange` takes a plain `response_buf: &mut [u8]`, and every caller manually wraps the slice and validates it in a single expression:
>     ```rust
>     let response: GetVersionRsp = Untrusted::new(response_vec.as_slice()).validate(&*self)?;
>     ```
>     Wrapping a plain `&[u8]` in `Untrusted::new(...)` only to immediately call `.validate(...)` on the same line provides no type-level guarantee that untrusted buffers aren't read elsewhere (and in fact, `response_vec` is indexed directly right after `validate()` to extend `self.transcript`).
>   - Furthermore, adding `type Context` with a lifetime parameter `'c` to `Validate<'c, Input>` makes the general `Validate` trait much heavier when a normal parsing method `FromBytes` / `try_from_bytes(buf: &[u8], state: &SpdmState)` would be simpler and more idiomatic.

Yeah, I caught this already

>
> - **Abuse of `CString` and `CString::into_vec` (Patches 21 & 22)**:
>   - Patch 21 adds `CString::into_vec` solely so `verify_signature` in Patch 22 can format the 64-byte ASCII `spdm_prefix` using `CString::try_from_fmt(...)`.
>   - Because `CString`'s internal `KVec<u8>` includes the trailing NUL terminator (`\0`), `prefix.into_vec()` actually returns **65 bytes** (64 ASCII bytes + 1 NUL byte), which then forces line 1013 to compensate for the extra NUL byte by subtracting `1` from `zero_pad_len`:
>     ```rust
>     let zero_pad_len = SPDM_COMBINED_PREFIX_SZ - SPDM_PREFIX_SZ - SPDM_CONTEXT.len() - 1;
>     ```
>   - Not only is `CString::into_vec` a footgun, `verify_signature` doesn't need `CString` or heap allocation at all: `m` is already allocated right above on line 981, or it can format directly into a fixed-size stack buffer `[0u8; SPDM_COMBINED_PREFIX_SZ]`.

Fixed

>
> - **Direct `unsafe` C FFI Calls in Driver/Library Code (`lib/rspdm/state.rs`)**:
>   - `lib/rspdm/state.rs` directly manipulates raw C pointers (`*mut bindings::device`, `*mut bindings::crypto_shash`, `*mut bindings::shash_desc`, `*mut bindings::x509_certificate`, `*mut bindings::public_key`) and makes raw `unsafe` calls to `crypto_alloc_shash`, `crypto_shash_digest`, `x509_cert_parse`, `public_key_verify_signature`, and `get_random_bytes`.
>   - Per Rust-for-Linux conventions, C subsystem bindings (`crypto_shash`, `x509_certificate`, `public_key`, `get_random_bytes`) should be wrapped in safe abstractions under `rust/kernel/`, and `dev` should use `device::Device` rather than a raw `*mut bindings::device` with no lifetime or reference-count guarantee.
>   - Note also in `validate_cert_chain`: `let cert = unsafe { *cert_ptr };` copies the entire C `struct x509_certificate` by value onto the stack just to read fields (`cert.unsupported_sig`, `cert.blacklisted`, `cert.sig`).

Fixed the device::Device part, crypto still doesn't have bindings (see above)

>
> - **Useless lifetime `'a` on `SpdmState<'a>` (`state.rs:136`)**:
>   - `SpdmState<'a>` has a lifetime parameter `'a` only used for `base_asym_enc: &'a CStr` and `base_hash_alg_name: &'a CStr`, which only ever point to `'static` string literals (`c_str!("pkcs1")`, `c_str!("sha256")`, etc.). Using `&'static CStr` (and `c"..."` instead of `CStr::from_bytes_with_nul(b"...\0")?`, which also eliminates the need for Patch 9!) removes the `'a` lifetime parameter from `SpdmState` entirely.

Outdated with Device lifetimes, although good point about patch 9

>
>
>
> ============================
>
>
>
>
> While `lib/rspdm/validator.rs` is a step up from earlier versions in that it contains **zero `unsafe` blocks** and uses `.get(...)` instead of `[]` indexing, the implementation is very unidiomatic, does a lot of wasted heap allocations, and still has bugs (including a kernel panic / underflow vector).
>
> Here is a breakdown of the issues in `validator.rs`:
>
> ---
>
> ### 1. Kernel Panic / Integer Underflow & Range Bug in `SpdmCertChain::validate`
> Even without `unsafe`, safe Rust in the kernel must not panic on untrusted input when `CONFIG_RUST_OVERFLOW_CHECKS` is enabled. Look at lines 849–872:
>
> ```rust
> let length = u16::from_le_bytes(
>     unvalidated
>         .get(0..2)
>         .ok_or(EIO)?
>         .try_into()
>         .map_err(|_| EINVAL)?,
> );
>
> let root_hash_end = SPDM_CERT_CHAIN_HDR_SZ + context.hash_len;
> ...
> let cert_chain_end = length as usize - root_hash_end;
> let mut certificates = KVec::new();
> certificates.extend_from_slice(
>     unvalidated.get(root_hash_end..cert_chain_end).ok_or(EIO)?,
>     GFP_KERNEL,
> )?;
> ```
> - **Arithmetic underflow**: `length` comes directly from the first 2 bytes of the untrusted certificate chain before `state.rs` compares it to `total_cert_len`. If a responder sends `length < 4 + hash_len`, `length as usize - root_hash_end` underflows (panicking with overflow checks enabled, or wrapping to `usize::MAX - ...`).
> - **Wrong slice end**: `length` in the SPDM Certificate Chain format (SPDM 1.0 Table 15) is the *total* length of the chain including `root_hash_end` (`4 + H`). Subtracting `root_hash_end` gives the *number of certificate bytes*, not the end index. Using it as the end index of `root_hash_end..cert_chain_end` slices `[4+H .. length-(4+H)]`, dropping the last `4 + H` bytes of the certificate chain (or returning `Err(EIO)` if the certificates are smaller than `4 + H` bytes).

Shasiko caught this too

>
> ---
>
> ### 2. Allocating and Copying into `KVec`s Only to Immediately Drop Them (`#[expect(dead_code)]` Everywhere)
> Almost every response struct in `validator.rs` (`GetVersionRsp`, `RespAlgStruct`, `SupportedAlgorithms`, `GetCapabilitiesRsp`, `NegotiateAlgsRsp`, `GetDigestsRsp`, `SpdmCertChain`, `ChallengeRsp`) is annotated with `#[expect(dead_code)]`.
>
> Why? Because `validate()` heap-allocates `KVec`s and copies payload data out of the response buffer into fields that **are never read anywhere in the crate**:
> - **`GetDigestsRsp::validate`**: Allocates up to 8 `KVec<u8>`s (`digests: [KVec<u8>; SPDM_SLOTS]`) and copies every digest into the heap. Then `SpdmState::get_digests` only reads `response.header.param1` and `response.header.param2` and immediately drops `response` (freeing all 8 `KVec`s without ever looking at the digests).
> - **`SupportedAlgorithms::from_bytes`**: Allocates `ext_asym: KVec<u32>`, `ext_hash: KVec<u32>`, and `alg_struct: KVec<RespAlgStruct>` (each containing 2 more `KVec`s). `SpdmState::get_capabilities` never accesses `response.supported_algorithms` at all.
> - **`NegotiateAlgsRsp::validate`**: Loops over `ext_asym_sel_count`, `ext_hash_sel_count`, and `header.param1` (`RespAlgStruct`), allocating `KVec`s for all of them. Then, as soon as `validate()` returns to `SpdmState::negotiate_algs`, `state.rs` checks:
>   ```rust
>   || response.ext_asym_sel_count != 0
>   || response.ext_hash_sel_count != 0
>   || response.header.param1 > request.header.param1 // request.header.param1 is always 0!
>   ```
>   So any non-empty `ext_asym`, `ext_hash`, or `resp_alg_struct` is **always rejected** in `state.rs`—yet `validator.rs` has 45 lines of code to heap-allocate and populate those vectors from untrusted counts *before* `state.rs` rejects them!
> - **`SpdmCertChain::validate`**: Heap-allocates `root_hash: KVec<u8>` and `certificates: KVec<u8>`. `SpdmState::get_certificate` only checks `certs.length`, drops `certs`, and stores the original unsplit `certs_buf` in `self.certs[slot]`.
> - **`ChallengeRsp::validate`**: Heap-allocates `cert_chain_hash: KVec<u8>` and `opaque_data: KVec<u8>`, neither of which is ever read or verified.
> - **Even fixed-size requests heap-allocate**: `SpdmHeader::to_bytes()` (4 bytes), `GetVersionReq::to_bytes()` (4 bytes), `GetDigestsReq::to_bytes()` (4 bytes), `GetCertificateReq::to_bytes()` (8 bytes), `NegotiateAlgsReq::to_bytes()` (32 bytes), and `ChallengeReq::to_bytes()` (36 or 44 bytes) all allocate a `KVec<u8>` (without `with_capacity`, causing multiple reallocations as `.push()` / `.extend_from_slice()` are called) instead of returning a fixed-size byte array `[u8; N]` or implementing `AsBytes`.
>
> Why copy slices out of `unvalidated: &'a [u8]` into newly allocated `KVec<u8>`s at all, when the parsed response struct can either borrow `&'a [u8]` sub-slices directly from the response buffer or validate in-place?

I replied to Alexandre about this as well.

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

The current approach has the obvious downside of copying the data, but
the buffers are small in the scheme of things. Copying it allows us to
use nice Rust structs (not packed), ensure the data is in the correct
endianes and check and validate the entire message. It also means that
the Rsp structs contain all of the information. As your LLM points
out, it currently isn't all read, but this is a basic starting point.

In terms of the allocations, it is only a few bytes, so I don't think
it matters that much.

In terms of unused elements, there are only a few unused, most of the
elements aren't dead code

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

>
> ---
>
> ### 3. Magic Byte Offsets Instead of a Cursor/Reader or `FromBytes`
> Because the author moved away from `#[repr(C, packed)]` structs in v4, they replaced struct definitions with hardcoded byte offsets. Look at `SupportedAlgorithms::from_bytes` and `NegotiateAlgsRsp::validate`:
>
> ```rust
> let alg_struct_count = *buf.get(0).ok_or(EIO)?;
> let length = read_le16(2)?;
> let measurement_specification = *buf.get(4).ok_or(EIO)?;
> let other_params_support = *buf.get(5).ok_or(EIO)?;
> let base_asym_algo = read_le32(6)?;
> let base_hash_algo = read_le32(10)?;
> let ext_asym_count = *buf.get(26).ok_or(EIO)?;
> let ext_hash_count = *buf.get(27).ok_or(EIO)?;
> let mel_specification = *buf.get(29).ok_or(EIO)?;
>
> let mut offset = 30;
> ```
> - **Hard to audit against the spec**: Jumping from offset `10` to `26` (skipping 12 reserved bytes) or `27` to `29` with magic numbers makes it very easy to miscount offsets, and `read_le16` / `read_le32` closures are copy-pasted across multiple functions.
> - **What would be much cleaner**:
>   1. For fixed-size layouts, use `FromBytes::from_bytes_copy_prefix` (or `zerocopy`) on `#[repr(C)]` structs with explicit reserved fields (`_reserved: [u8; 12]`) and little-endian integer wrappers, **or**

I don't think this would be cleaner, it actually sounds much less
clean. Also, there aren't that many fixed size structs

>   2. Use a small sequential reader struct wrapping `&'a [u8]` (e.g., `r.read_u8()?`, `r.read_le16()?`, `r.read_le32()?`, `r.skip(12)?`, `r.read_slice(len)?`). That eliminates manual offset arithmetic, cannot underflow/overflow `offset`, and reads top-to-bottom like the spec table.
>
> ---
>
> ### 4. Inconsistent Split of Responsibilities Between `validator.rs` and `state.rs`
> The boundary between what is validated in `Validate::validate` (`validator.rs`) vs. what is checked in `SpdmState` (`state.rs`) is arbitrary:
> - **Duplicated checks**: `GetCapabilitiesRsp::validate` checks `if data_transfer_size < SPDM_MIN_DATA_TRANSFER_SIZE` (`validator.rs:456`), and then `SpdmState::get_capabilities` checks `if response.data_transfer_size < 42` (`state.rs:508`) again.
> - **Misplaced checks**: `GetDigestsRsp::validate` checks `header.param2 == 0` inside `validator.rs`, whereas `NegotiateAlgsRsp` and `GetCertificateRsp` leave basic structural checks (`ext_asym_sel_count != 0`, `response.header.param1 & 0xF != slot`, `total_cert_len != cert_chain_length`) to `state.rs`.
> - **Inconsistent error codes**: Within `validator.rs`, out-of-bounds buffer slices return `EIO`, wrong response codes return `EINVAL`, and wrong versions or lengths return `EPROTO`. In `spdm_exchange`, if a truncated packet is `< 4` bytes (header size), `spdm_exchange` returns `Ok(length)` and lets the caller's `validate()` fail with `EIO`.
>
> ---
>
> ### Summary
> The move to safe code in `validator.rs` achieved the goal of removing `unsafe` pointer casts, but it was done in the most mechanical way possible: replacing struct field accesses with hardcoded `.get(offset..offset + N)` calls and copying every variable-length region into throwaway `KVec`s.
>
> Replacing the manual offset indexing with either `FromBytes` / `zerocopy` (for fixed headers) + a tiny slice cursor (borrowing `&'a [u8]` slices instead of allocating `KVec`s) would probably cut `validator.rs` in half, remove ~15 unnecessary heap allocations per authentication, and fix the `SpdmCertChain` underflow/range bug.

That won't handle endian conversion though. A slice cursor is an
interesting idea though, I might add that as a followup

Alistair