Re: [PATCH v3 16/21] lib: rspdm: Support SPDM negotiate_algorithms
From: Alistair
Date: Fri Sep 11 2026 - 00:53:59 EST
On Wed, 2026-09-09 at 01:17 +0100, Jonathan Cameron wrote:
> On Tue, 1 Sep 2026 11:03:42 +1000
> alistair23@xxxxxxxxx wrote:
>
> > From: Alistair Francis <alistair@xxxxxxxxxxxxx>
> >
> > Support the NEGOTIATE_ALGORITHMS SPDM command.
> >
> > Signed-off-by: Alistair Francis <alistair@xxxxxxxxxxxxx>
> 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)
> > + }
> > +}