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

[Alice Ryhl <aliceryhl@google.com>]

On Mon, Sep 28, 2026 at 11:11:01AM +1000, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> 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.

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.

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.

- **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()`.

- **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`.

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

- **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]`.

- **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`).

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



============================




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

---

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

---

### 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**
  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.