Re: [PATCH 13/18] lib: rspdm: Support SPDM negotiate_algorithms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Use-After-Free and Double Free of `shash_desc` allocation.
- [High] Invalid pointer dereference and kernel panic when `crypto_alloc_shash` fails.
- [High] Logic error in measurement capability check bypasses algorithm validation.
- [High] Missing upper-bound check on `rc` allows vector length to exceed capacity.
- [High] Endianness bug in `request.length` field generation.
- [High] Creating `&mut [u8]` slices over uninitialized memory is Undefined Behavior.
- [High] Memory leak of the `SpdmState` struct in `spdm_destroy()`.
--

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

lib: rspdm: Support SPDM negotiate_algorithms

This commit adds support for the NEGOTIATE_ALGORITHMS SPDM command.

> diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs
> index 9628f258854c6..72886a5dfd695 100644
> --- a/lib/rspdm/lib.rs
> +++ b/lib/rspdm/lib.rs
[ ... ]
> @@ -123,4 +127,16 @@
>  ///
>  /// @spdm_state: SPDM session state
>  #[export]
> -pub unsafe extern "C" fn spdm_destroy(_state_ptr: *mut spdm_state) {}
> +pub unsafe extern "C" fn spdm_destroy(state_ptr: *mut spdm_state) {
> +    let state: &mut SpdmState = unsafe { &mut (*(state_ptr as *mut SpdmState)) };
> +
> +    if let Some(desc) = &mut state.desc {
> +        unsafe {
> +            bindings::kfree(*desc as *mut _ as *mut c_void);
> +        }
> +    }
> +

Does this leak the SpdmState allocation itself? The internal fields are cleaned
up, but state_ptr itself does not appear to be freed here.

> +    unsafe {
> +        bindings::crypto_free_shash(state.shash);
> +    }
> +}

If crypto_alloc_shash() fails earlier, it can return an ERR_PTR. If that
happens, state.shash holds an error pointer. Since crypto_free_shash()
only checks for NULL, will this cause a kernel panic when trying to free
an ERR_PTR here?

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index e7119ffa9a691..34676744e5094 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> @@ -367,4 +427,155 @@ pub(crate) fn get_capabilities(&mut self) -> Result<(), Error> {
[ ... ]
> +        let mut desc_vec: KVec<u8> = KVec::with_capacity(desc_len, GFP_KERNEL)?;
> +        // SAFETY: `desc_vec` is `desc_len` long
> +        let desc_buf = unsafe { from_raw_parts_mut(desc_vec.as_mut_ptr(), desc_len) };

Is this creating a reference to uninitialized memory? KVec::with_capacity()
leaves the underlying memory uninitialized, and forming a slice over
uninitialized memory is undefined behavior in Rust.

> +
> +        let desc = unsafe {
> +            core::mem::transmute::<*mut c_void, &mut bindings::shash_desc>(
> +                desc_buf.as_mut_ptr() as *mut c_void
> +            )
> +        };
> +        desc.tfm = self.shash;
> +
> +        self.desc = Some(desc);

Can this cause a use-after-free and a later double-free? desc_vec is a local
variable that will be dropped at the end of this function, freeing its
backing buffer. self.desc then holds a dangling reference. Furthermore,
when spdm_destroy() cleans up the session, it calls bindings::kfree() on
this dangling pointer, which seems like it would corrupt the kernel
allocator.

[ ... ]
> +    pub(crate) fn negotiate_algs(&mut self) -> Result<(), Error> {
> +        let mut request = NegotiateAlgsReq::default();
> +        request.version = self.version;
> +
> +        if self.version >= SPDM_VER_12 && (self.rsp_caps & SPDM_KEY_EX_CAP) == SPDM_KEY_EX_CAP {
> +            request.other_params_support = SPDM_OPAQUE_DATA_FMT_GENERAL;
> +        }
> +
> +        let req_sz = core::mem::size_of::<NegotiateAlgsReq>();
> +        let rsp_sz = core::mem::size_of::<NegotiateAlgsRsp>();
> +
> +        request.length = req_sz as u16;

The SPDM protocol requires multi-byte fields to be transmitted in
little-endian format. Will this send a corrupted, big-endian length value
on big-endian architectures? The base_asym_algo field uses to_le(), so
perhaps this should as well.

> +
> +        // 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 also creating a reference to uninitialized memory? The safety
comment notes it is initialized, but KVec::with_capacity() does not initialize
the allocated memory.

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

Does this code verify that rc is not larger than rsp_sz? If
spdm_exchange() returns a value greater than rsp_sz (for example, if a
device sends extra payload data), would this set the vector length beyond
its allocated capacity and lead to out-of-bounds memory accesses?

[ ... ]
> +        if self.rsp_caps & SPDM_MEAS_CAP_MASK == SPDM_MEAS_CAP_MASK
> +            && (self.meas_hash_alg.count_ones() != 1
> +                || response.measurement_specification_sel != SPDM_MEAS_SPEC_DMTF)
> +        {
> +            pr_err!("Malformed algorithms response\n");
> +            to_result(-(bindings::EPROTO as i32))?;
> +        }

Does this condition correctly check for measurement support?
SPDM_MEAS_CAP_MASK is defined as bits 3 and 4 (11b). According to the SPDM
specification, 11b is a reserved value, while valid measurement support is
01b or 10b. By checking for strict equality with the mask, it looks like
this evaluates to false for legitimate devices and bypasses the algorithm
validation.

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