Re: [PATCH v2 16/21] lib: rspdm: Support SPDM negotiate_algorithms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect generic type inference in `KVec::from_raw_parts` and lost capacity leads to Rust Undefined Behavior during descriptor destruction.
- [High] The NEGOTIATE_ALGORITHMS handler rejects valid SPDM 1.0 responses due to a hardcoded 36-byte size validation.
--

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

lib: rspdm: Support SPDM negotiate_algorithms

Support the NEGOTIATE_ALGORITHMS SPDM command.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index 5ef14c8ed2375..e4eb009a977c3 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> @@ -72,9 +108,41 @@ pub(crate) struct SpdmState {
>      // Negotiated state
>      pub(crate) version: u8,
>      pub(crate) rsp_caps: u32,
> +    pub(crate) base_asym_alg: u32,
> +    pub(crate) base_hash_alg: u32,
> +    pub(crate) meas_hash_alg: u32,
> +
> +    /* Signature algorithm */
> +    base_asym_enc: &'a CStr,
> +    sig_len: usize,
> +
> +    /* Hash algorithm */
> +    base_hash_alg_name: &'a CStr,
> +    pub(crate) shash: *mut bindings::crypto_shash,
> +    pub(crate) desc: Option<&'a mut bindings::shash_desc>,
> +    pub(crate) hash_len: usize,
>  }
>  
> -impl SpdmState {
> +impl Drop for SpdmState<'_> {
> +    fn drop(&mut self) {
> +        if let Some(desc) = self.desc.take() {
> +            // SAFETY: `self.shash` is a valid handle
> +            let desc_len = core::mem::size_of::<bindings::shash_desc>()
> +                + unsafe { bindings::crypto_shash_descsize(self.shash) } as usize;
> +
> +            // SAFETY: `desc` was allocated and converted to a raw pointer with
> +            // into_raw_parts()
> +            let desc_vec = unsafe { KVec::from_raw_parts(desc, desc_len, desc_len) };

[Severity: High]
Does this code inadvertently cause Undefined Behavior by incorrectly inferring
the generic type of KVec?

Since `desc` is of type `&mut bindings::shash_desc`, `KVec::from_raw_parts`
infers the new vector as `KVec<bindings::shash_desc>`. Because `desc_len` is
calculated in bytes, this tells the allocator to free `desc_len` elements
of type `shash_desc` (which is 16 bytes) instead of `desc_len` bytes.

Also, it looks like the original allocation capacity was discarded when
`into_raw_parts()` was called in `update_response_algs()`, and here we are
passing `desc_len` as the capacity, which violates `from_raw_parts` invariants.

> +            drop(desc_vec);
> +        }
> +
> +        unsafe {
> +            bindings::crypto_free_shash(self.shash);
> +        }
> +    }
> +}
> +
[ ... ]
> +    fn update_response_algs(&mut self) -> Result<(), Error> {
[ ... ]
> +        // This is freed in when `SpdmState` is dropped, but this call
> +        // can happen multiple times.
> +        if self.shash != core::ptr::null_mut() {
> +            if let Some(desc) = self.desc.take() {
> +                // SAFETY: `self.shash` is a valid handle
> +                let desc_len = core::mem::size_of::<bindings::shash_desc>()
> +                    + unsafe { bindings::crypto_shash_descsize(self.shash) } as usize;
> +
> +                // SAFETY: `desc` was allocated and converted to a raw pointer with
> +                // into_raw_parts()
> +                let desc_vec = unsafe { KVec::from_raw_parts(desc, desc_len, desc_len) };

[Severity: High]
Will the same Undefined Behavior happen here when replacing an existing hash
algorithm context, since it also uses `desc` and `desc_len` to reconstruct the
`KVec`?

> +                drop(desc_vec);
> +            }
> +
> +            unsafe {
> +                bindings::crypto_free_shash(self.shash);
> +            }
> +        }
> +
> +        self.shash =
[ ... ]
> diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> index 7b5aca5d50f89..c53828376fcaa 100644
> --- a/lib/rspdm/validator.rs
> +++ b/lib/rspdm/validator.rs
[ ... ]
> +impl Validate<Untrusted<&[u8]>> for &NegotiateAlgsRsp {
> +    type Err = Error;
> +
> +    fn validate(unvalidated: &[u8]) -> Result<Self, Self::Err> {
> +        if unvalidated.len() < mem::size_of::<NegotiateAlgsRsp>() {
> +            return Err(EINVAL);
> +        }

[Severity: High]
Could this hardcoded length check reject valid SPDM 1.0 responses?

`SpdmState` allows negotiating SPDM version 1.0, and the SPDM 1.0.0
specification states that the NEGOTIATE_ALGORITHMS response base header is
exactly 32 bytes long.

Because `mem::size_of::<NegotiateAlgsRsp>()` is 36 bytes (including fields
introduced in SPDM 1.1), when communicating with an SPDM 1.0 responder
that returns 32 bytes, this will fail with EINVAL and break negotiation.

> +
> +        let ptr = unvalidated.as_ptr();
> +        // CAST: `NegotiateAlgsRsp` only contains integers and has `repr(C)`.
> +        let ptr = ptr.cast::<NegotiateAlgsRsp>();
> +        // SAFETY: `ptr` came from a reference and the cast above is valid.
> +        let rsp: &NegotiateAlgsRsp = unsafe { &*ptr };
> +
> +        Ok(rsp)
> +    }
> +}

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