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

[Jonathan Cameron <jic23@kernel.org>]

On Tue,  1 Sep 2026 11:03:42 +1000
alistair23@gmail.com wrote:

> From: Alistair Francis <alistair@alistair23.me>
> 
> Support the NEGOTIATE_ALGORITHMS SPDM command.
> 
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
A question on types inline. Otherwise with the thing Aksh
called out this looks fine to me.

> diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs
> index 76325babdff2..d418d15e4c70 100644
> --- a/lib/rspdm/lib.rs
> +++ b/lib/rspdm/lib.rs

> @@ -117,11 +121,12 @@ pub extern "C" fn spdm_destroy(state_ptr: *mut spdm_state) {
>      if state_ptr.is_null() {
>          return;
>      }
> +

Move that to the early commit.

>      // SAFETY: `state_ptr` was returned from `spdm_create()`, which leaked a
>      // `Pin<KBox<Mutex<SpdmState>>>` via `KBox::into_raw`.  The caller
>      // guarantees the state is no longer in use.  Reconstructing the pinned
>      // box and dropping it runs `Drop` for the `Mutex` and `SpdmState` and
>      // frees the allocation.
> -    let b = unsafe { KBox::from_raw(state_ptr as *mut Mutex<SpdmState>) };
> +    let b = unsafe { KBox::from_raw(state_ptr as *mut Mutex<SpdmState<'_>>) };
>      drop(unsafe { Pin::new_unchecked(b) });
>  }
> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index 5ef14c8ed237..b78086c75370 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
...

>  
> @@ -373,4 +452,164 @@ pub(crate) fn get_capabilities(&mut self) -> Result<(), Error> {
>  
>          Ok(())
>      }
> +
> +    fn update_response_algs(&mut self) -> Result<(), Error> {
> +        match self.base_asym_alg {
> +            SPDM_ASYM_RSASSA_2048 => {
> +                self.sig_len = 256;
> +                self.base_asym_enc = CStr::from_bytes_with_nul(b"pkcs1\0")?;
> +            }
> +            SPDM_ASYM_RSASSA_3072 => {
> +                self.sig_len = 384;
> +                self.base_asym_enc = CStr::from_bytes_with_nul(b"pkcs1\0")?;
> +            }
> +            SPDM_ASYM_RSASSA_4096 => {
> +                self.sig_len = 512;
> +                self.base_asym_enc = CStr::from_bytes_with_nul(b"pkcs1\0")?;
> +            }
> +            SPDM_ASYM_ECDSA_ECC_NIST_P256 => {
> +                self.sig_len = 64;
> +                self.base_asym_enc = CStr::from_bytes_with_nul(b"p1363\0")?;
> +            }
> +            SPDM_ASYM_ECDSA_ECC_NIST_P384 => {
> +                self.sig_len = 96;
> +                self.base_asym_enc = CStr::from_bytes_with_nul(b"p1363\0")?;
> +            }
> +            SPDM_ASYM_ECDSA_ECC_NIST_P521 => {
> +                self.sig_len = 132;
> +                self.base_asym_enc = CStr::from_bytes_with_nul(b"p1363\0")?;
> +            }
> +            _ => {
> +                pr_err!("Unknown asym algorithm\n");
> +                return Err(EINVAL);
> +            }
> +        }
> +
> +        match self.base_hash_alg {
> +            SPDM_HASH_SHA_256 => {
> +                self.base_hash_alg_name = CStr::from_bytes_with_nul(b"sha256\0")?;
> +            }
> +            SPDM_HASH_SHA_384 => {
> +                self.base_hash_alg_name = CStr::from_bytes_with_nul(b"sha384\0")?;
> +            }
> +            SPDM_HASH_SHA_512 => {
> +                self.base_hash_alg_name = CStr::from_bytes_with_nul(b"sha512\0")?;
> +            }
> +            _ => {
> +                pr_err!("Unknown hash algorithm\n");
> +                return Err(EINVAL);
> +            }
> +        }
> +
> +        // This is freed in when `SpdmState` is dropped, but this call

freed when

> +        // 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;
> +
...

> diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> index 42c0b28cdcaa..4f7a82d4b210 100644
> --- a/lib/rspdm/validator.rs
> +++ b/lib/rspdm/validator.rs
> @@ -9,7 +9,8 @@
>  
>  use crate::bindings::{
>      __IncompleteArrayField,
> -    __le16, //
> +    __le16,
> +    __le32, //
>  };

> +
> +#[repr(C, packed)]
> +pub(crate) struct NegotiateAlgsRsp {
> +    pub(crate) version: u8,
> +    pub(crate) code: u8,
> +    pub(crate) param1: u8,
> +    pub(crate) param2: u8,
> +
> +    pub(crate) length: u16,

Why do we treat this one as native endian but the ext_asym below
as explicitly little endian?  

> +    pub(crate) measurement_specification_sel: u8,
> +    pub(crate) other_params_sel: u8,
> +
> +    pub(crate) measurement_hash_algo: u32,
> +    pub(crate) base_asym_sel: u32,
> +    pub(crate) base_hash_sel: u32,
> +
> +    reserved1: [u8; 11],
> +
> +    pub(crate) mel_specification_sel: u8,
> +    pub(crate) ext_asym_sel_count: u8,
> +    pub(crate) ext_hash_sel_count: u8,
> +    reserved2: [u8; 2],
> +
> +    pub(crate) ext_asym: __IncompleteArrayField<__le32>,
> +    pub(crate) ext_hash: __IncompleteArrayField<__le32>,
> +    pub(crate) resp_alg_struct: __IncompleteArrayField<RegAlg>,
> +}
> +
> +impl<'a> Validate<Untrusted<&'a [u8]>> for &'a NegotiateAlgsRsp {
> +    type Err = Error;
> +
> +    fn validate(unvalidated: &[u8]) -> Result<Self, Self::Err> {
> +        if unvalidated.len() < mem::size_of::<NegotiateAlgsRsp>() {
> +            return Err(EINVAL);
> +        }
> +
> +        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)
> +    }
> +}