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

[Alistair <alistair@alistair23.me>]

On Wed, 2026-09-09 at 01:17 +0100, Jonathan Cameron wrote:
> 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.

It's not that easy, even with an underscore Rust complains about it
being unused


error[E0392]: lifetime parameter `'_a` is never used
  --> lib/rspdm/state.rs:46:29
   |
46 | pub(crate) struct SpdmState<'_a> {
   |                             ^^^ unused lifetime parameter
   |
   = help: consider removing `'_a`, referring to it in a field, or
using a marker such as `PhantomData`

error: aborting due to 1 previous error

I could use `PhantomData` to hide the error, but that doesn't feel
great when it's just a temp for a few patches.

1:
https://doc.rust-lang.org/nightly/core/marker/struct.PhantomData.html

> 
> >      // 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?  

Just a bug, fixed!

Alistair

> 
> > +    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)
> > +    }
> > +}