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

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

On Thu Oct 15, 2026 at 2:23 PM JST, Alistair Francis wrote:
> On Thu, Oct 15, 2026 at 6:54 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> 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.
>
> 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?

So without going all the way to fully-fledged crypto bindings, you will
spare yourself and your reviewers quite a bit of pain if you at least
implement a safe and lightweight crypto wrapper that is local to SPDM
and could eventually grow into official crypto bindings as a future
effort. As it is the code is very hard to review: for instance
`crypto_free_shash` is always called after `free_desc`, and looks like
it could/should be part of it, but that's hard to assess as the crypto
code is mixed with the rest and called here and there. I just cannot
tell whether all these unsafe blocks are safe of not, which is a problem
since the goal of writing this in Rust is to prevent exploits.

A self-contained crypto module exposing a safe interface would remove
that burden, reduce the number of `unsafe` blocks, and make the series
easier to review globally while not necessarily resulting in more code.
You just need to store the relevant pointers into a new data structure,
enforce the required invariants at construction time, and codify the
actions that SPDM is already doing in the form of methods.