Re: [PATCH 14/18] lib: rspdm: Support SPDM get_digests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Creating a mutable slice `&mut [u8]` from uninitialized memory is Undefined Behavior.
- [High] Missing bounds check on the returned length before incrementing vector capacity.
- [High] `spdm_authenticate` lacks the documented internal locking, allowing concurrent accesses to mutate state without synchronization.
- [Medium] Incorrect structure size used in the response length validation.
--

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

lib: rspdm: Support SPDM get_digests

This commit introduces support for the GET_DIGESTS SPDM command. It
adds the necessary constants, state tracking for certificate slots,
and the get_digests() function to perform the exchange.

> diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs
> index 72886a5dfd695..e42cfdd35524c 100644
> --- a/lib/rspdm/lib.rs
> +++ b/lib/rspdm/lib.rs
> @@ -120,6 +120,10 @@
>          return e.to_errno() as c_int;
>      }
>  
> +    if let Err(e) = state.get_digests() {
> +        return e.to_errno() as c_int;
> +    }
> +
>      0
>  }

Does spdm_authenticate properly handle concurrent invocations?

The exported C wrapper spdm_authenticate casts the opaque pointer to an
exclusive mutable reference &mut SpdmState without acquiring any lock.
The documentation explicitly promises to perform internal locking to serialize
multiple concurrent invocations.

Because SpdmState does not contain a lock and none is acquired, concurrent
calls from C drivers could create multiple aliasing &mut references to the
exact same memory, causing undefined behavior and data races when mutating
shared state like self.provisioned_slots.

Could we add the necessary locking around the state object?

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index 34676744e5094..bcb1cc955c4c6 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs

[ ... ]

> +    pub(crate) fn get_digests(&mut self) -> Result<(), Error> {
> +        let mut request = GetDigestsReq::default();
> +        request.version = self.version;
> +
> +        let req_sz = core::mem::size_of::<GetDigestsReq>();
> +        let rsp_sz = core::mem::size_of::<GetDigestsRsp>() + SPDM_SLOTS * self.hash_len;
> +
> +        // SAFETY: `request` is repr(C) and packed, so we can convert it to a slice
> +        let request_buf = unsafe { from_raw_parts_mut(&mut request as *mut _ as *mut u8, req_sz) };
> +
> +        let mut response_vec: KVec<u8> = KVec::with_capacity(rsp_sz, GFP_KERNEL)?;
> +        // SAFETY: `response_vec` is rsp_sz length, initialised, aligned
> +        // and won't be mutated
> +        let response_buf = unsafe { from_raw_parts_mut(response_vec.as_mut_ptr(), rsp_sz) };

Is this slice creation safe?

KVec::with_capacity() allocates memory but leaves it uninitialized. The code
then calls from_raw_parts_mut() to create a &mut [u8] slice spanning the entire
capacity.

In Rust, creating a slice reference to uninitialized memory is immediate
undefined behavior, as it violates the validity invariant of the u8 type. The
SAFETY comment asserts the memory is initialized, but it actually isn't.

Could we initialize the memory first (e.g., passing __GFP_ZERO to the
allocator or explicitly zeroing it), or handle it using MaybeUninit?

> +
> +        let len = self.spdm_exchange(request_buf, response_buf)?;
> +
> +        if len < (core::mem::size_of::<GetDigestsRsp>() as i32) {
> +            pr_err!("Truncated digests response\n");
> +            to_result(-(bindings::EIO as i32))?;
> +        }
> +
> +        // SAFETY: `len` is the length of data read, which will be smaller
> +        // then the capacity of the vector
> +        unsafe { response_vec.inc_len(len as usize) };

Are we missing a bounds check on the returned length before incrementing the
vector's length?

The code retrieves len from self.spdm_exchange() and immediately passes it to
response_vec.inc_len(). The safety comment assumes that len will inherently be
smaller than or equal to the capacity.

If an external device or transport layer returns a length greater than rsp_sz,
the inc_len call will set the vector's length beyond its physical allocation,
leading to out-of-bounds memory accesses in subsequent operations.

> +
> +        let response: &mut GetDigestsRsp = Untrusted::new_mut(&mut response_vec).validate_mut()?;
> +
> +        if len
> +            < (core::mem::size_of::<GetDigestsReq>()
> +                + response.param2.count_ones() as usize * self.hash_len) as i32
> +        {

Is the correct structure size used in this response length validation?

The code dynamically calculates the limit using size_of::<GetDigestsReq>(),
which is the request structure rather than the response structure
GetDigestsRsp.

While both structures currently happen to be exactly 4 bytes in size, using
the request structure to validate a response payload is logically flawed. If
the response structure size is updated in the future, this bounds check will
fail to properly validate the minimum length.

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