Re: [PATCH 11/18] lib: rspdm: Support SPDM get_version

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Missing capacity bounds check before increasing vector length leads to out-of-bounds heap memory read.
- [High] Creating a `&mut [u8]` slice from uninitialized heap memory triggers Undefined Behavior.
- [High] Endianness bug when parsing little-endian SPDM version numbers.
- [High] Out-of-bounds pointer provenance (UB) when accessing the `version_number_entries` flexible array member.
- [High] Data race and mutable aliasing due to missing internal locking in `spdm_authenticate`.
- [Medium] `SPDM_GET_VERSION_LEN` is too small to accommodate a maximum-sized valid GET_VERSION response.
--

commit 0a244ec3f0839ba962c85556e66ffad42451e041
Author: Alistair Francis <alistair@alistair23.me>

lib: rspdm: Support SPDM get_version

This commit adds support for the SPDM GET_VERSION command to negotiate
a supported SPDM version and store it in the SpdmState.

> diff --git a/lib/rspdm/consts.rs b/lib/rspdm/consts.rs
> index 2feddde678858..5482a0f6cee09 100644
> --- a/lib/rspdm/consts.rs
> +++ b/lib/rspdm/consts.rs
[ ... ]
> @@ -54,3 +65,6 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
>          Ok(())
>      }
>  }
> +
> +pub(crate) const SPDM_GET_VERSION: u8 = 0x84;
> +pub(crate) const SPDM_GET_VERSION_LEN: usize = mem::size_of::<SpdmHeader>() + u8::MAX as usize;

Is this length large enough to accommodate a maximum-sized valid GET_VERSION
response?

A valid GET_VERSION response payload contains up to 255 16-bit version
entries. The maximum possible valid response size would be 6 bytes (header
and count) plus 510 bytes (entries), totaling 516 bytes.

Allocating only 259 bytes means valid responses with more than 126 entries
might be truncated and rejected during length validation.


> diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs
> index 758d43fba5cba..1cc6c33516fb2 100644
> --- a/lib/rspdm/lib.rs
> +++ b/lib/rspdm/lib.rs
[ ... ]
> @@ -105,7 +105,13 @@
>  /// Return 0 on success or a negative errno.  In particular, -EPROTONOSUPPORT
>  /// indicates authentication is not supported by the device.
>  #[export]
> -pub unsafe extern "C" fn spdm_authenticate(_state_ptr: *mut spdm_state) -> c_int {
> +pub unsafe extern "C" fn spdm_authenticate(state_ptr: *mut spdm_state) -> c_int {
> +    let state: &mut SpdmState = unsafe { &mut (*(state_ptr as *mut SpdmState)) };

Can concurrent FFI calls to this function on the same device state create
mutable aliased Rust references?

If multiple callers invoke this without external synchronization, creating
a mutable reference directly from the pointer could trigger data races
and undefined behavior. Does this need internal locking to serialize
concurrent invocations?


> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index 18e81f24c7249..26b90942b298d 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> @@ -232,4 +237,60 @@ pub(crate) fn spdm_exchange(
[ ... ]
> +    pub(crate) fn get_version(&mut self) -> Result<(), Error> {
> +        let mut request = GetVersionReq::default();
[ ... ]
> +        let mut response_vec: KVec<u8> = KVec::with_capacity(SPDM_GET_VERSION_LEN, GFP_KERNEL)?;
> +        // SAFETY: `response_vec` is SPDM_GET_VERSION_LEN length, initialised, aligned
> +        // and won't be mutated
> +        let response_buf =
> +            unsafe { from_raw_parts_mut(response_vec.as_mut_ptr(), SPDM_GET_VERSION_LEN) };

Does creating a mutable slice from this vector trigger undefined behavior?

KVec::with_capacity() allocates memory but leaves the contents uninitialized.
Forming a reference or slice over uninitialized memory for types that expect
valid initialization (like u8) is immediate undefined behavior in Rust.

Should the buffer be explicitly initialized first, for example by using
KVec::from_elem()?


> +        let rc = self.spdm_exchange(request_buf, response_buf)?;
> +
> +        // SAFETY: `rc` is the length of data read, which will be smaller
> +        // than the capacity of the vector
> +        unsafe { response_vec.inc_len(rc as usize) };

Is it guaranteed that rc will always be smaller than the capacity of the
vector?

The length returned by spdm_exchange() originates from the hardware
transport function, which might report a payload length larger than the
requested buffer.

If a device returns a length larger than SPDM_GET_VERSION_LEN, this would
push the vector's length beyond its actual allocation, potentially leading
to an out-of-bounds heap memory read later on. Should there be a bounds
check before increasing the vector length?


> +        let response: &mut GetVersionRsp = Untrusted::new_mut(&mut response_vec).validate_mut()?;
> +
> +        let mut foundver = false;
> +        let unaligned = core::ptr::addr_of_mut!(response.version_number_entries) as *mut u16;

Does deriving this pointer from a strongly typed reference violate Rust's
strict aliasing rules?

The validate method returns a strongly typed &mut GetVersionRsp, which
restricts the memory provenance strictly to the struct's size (6 bytes,
since the trailing __IncompleteArrayField is a zero-sized type).

Using a raw pointer derived from this reference to read memory beyond
that 6-byte boundary is undefined behavior under Stacked Borrows.


> +        for i in 0..response.version_number_entry_count {
> +            // Creating a reference on a packed struct will result in
> +            // undefined behaviour, so we operate on the raw data directly
> +            let addr = unaligned.wrapping_add(i as usize);
> +            let alpha_version = (unsafe { core::ptr::read_unaligned::<u16>(addr) } & 0xF) as u8;
> +            let version = (unsafe { core::ptr::read_unaligned::<u16>(addr) } >> 8) as u8;

Will this correctly parse the version on big-endian architectures?

The protocol version numbers are specified as little-endian 16-bit integers
(__le16), but core::ptr::read_unaligned() reads the values natively.

On big-endian machines, reading a little-endian 0x1000 (SPDM version 1.0)
natively will incorrectly interpret the bytes backwards, yielding 0x0010.
This would extract a version of 0 and break the SPDM negotiation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508031710.514574-1-alistair.francis@wdc.com?part=11