From: Alistair Francis <alistair@alistair23.me>
Support the GET_VERSION SPDM command.
Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
lib/rspdm/consts.rs | 17 +++++++++++
lib/rspdm/lib.rs | 6 +++-
lib/rspdm/state.rs | 58 ++++++++++++++++++++++++++++++++++--
lib/rspdm/validator.rs | 67 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 144 insertions(+), 4 deletions(-)
diff --git a/lib/rspdm/consts.rs b/lib/rspdm/consts.rs
index 40ce60eba2f3..38f48f0863e2 100644
--- a/lib/rspdm/consts.rs
+++ b/lib/rspdm/consts.rs
@@ -7,6 +7,20 @@
//! Rust implementation of the DMTF Security Protocol and Data Model (SPDM)
//! <https://www.dmtf.org/dsp/DSP0274>
+use crate::validator::SpdmHeader;
+use core::mem;
+
+/* SPDM versions supported by this implementation */
+pub(crate) const SPDM_VER_10: u8 = 0x10;
+#[allow(dead_code)]
+pub(crate) const SPDM_VER_11: u8 = 0x11;
+#[allow(dead_code)]
+pub(crate) const SPDM_VER_12: u8 = 0x12;
+pub(crate) const SPDM_VER_13: u8 = 0x13;
+
+pub(crate) const SPDM_MIN_VER: u8 = SPDM_VER_10;
+pub(crate) const SPDM_MAX_VER: u8 = SPDM_VER_13;
+
pub(crate) const SPDM_REQ: u8 = 0x80;
pub(crate) const SPDM_ERROR: u8 = 0x7f;
@@ -115,3 +129,6 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
Ok(())
}
}
+
+pub(crate) const SPDM_GET_VERSION: u8 = 0x84;
+pub(crate) const SPDM_GET_VERSION_LEN: usize = mem::size_of::<SpdmHeader>() + 255;
diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs
index 2bb716140e0a..08abcbb21247 100644
--- a/lib/rspdm/lib.rs
+++ b/lib/rspdm/lib.rs
@@ -108,7 +108,11 @@
/// Return 0 on success or a negative errno. In particular, -EPROTONOSUPPORT
/// indicates authentication is not supported by the device.
#[no_mangle]
-pub unsafe extern "C" fn spdm_authenticate(_state: &'static mut SpdmState) -> c_int {
+pub unsafe extern "C" fn spdm_authenticate(state: &'static mut SpdmState) -> c_int {
+ if let Err(e) = state.get_version() {
+ return e.to_errno() as c_int;
+ }
+
0
}
diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
index 68861f30e3fa..3ad53ec05044 100644
--- a/lib/rspdm/state.rs
+++ b/lib/rspdm/state.rs
@@ -8,6 +8,7 @@
//! <https://www.dmtf.org/dsp/DSP0274>
use core::ffi::c_void;
+use core::slice::from_raw_parts_mut;
use kernel::prelude::*;
use kernel::{
bindings,
@@ -15,8 +16,10 @@
validate::Untrusted,
};
-use crate::consts::{SpdmErrorCode, SPDM_ERROR, SPDM_REQ};
-use crate::validator::{SpdmErrorRsp, SpdmHeader};
+use crate::consts::{
+ SpdmErrorCode, SPDM_ERROR, SPDM_GET_VERSION_LEN, SPDM_MAX_VER, SPDM_MIN_VER, SPDM_REQ,
+};
+use crate::validator::{GetVersionReq, GetVersionRsp, SpdmErrorRsp, SpdmHeader};
/// The current SPDM session state for a device. Based on the
/// C `struct spdm_state`.
@@ -61,7 +64,7 @@ pub(crate) fn new(
transport_sz,
keyring,
validate,
- version: 0x10,
+ version: SPDM_MIN_VER,
}
}
@@ -217,4 +220,53 @@ pub(crate) fn spdm_exchange(
Ok(length)
}
+
+ /// Negoiate a supported SPDM version and store the information
+ /// in the `SpdmState`.
+ pub(crate) fn get_version(&mut self) -> Result<(), Error> {
+ let mut request = GetVersionReq::default();
+ request.version = self.version;
+
+ // 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,
+ core::mem::size_of::<GetVersionReq>(),
+ )
+ };
+
+ let mut response_vec: KVec<u8> = KVec::with_capacity(SPDM_GET_VERSION_LEN, GFP_KERNEL)?;
+ // SAFETY: `request` is repr(C) and packed, so we can convert it to a slice
+ let response_buf =
+ unsafe { from_raw_parts_mut(response_vec.as_mut_ptr(), SPDM_GET_VERSION_LEN) };
+
+ let rc = self.spdm_exchange(request_buf, response_buf)?;
+
+ // 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) };
+
+ let response: &mut GetVersionRsp = Untrusted::new_mut(&mut response_vec).validate_mut()?;
+
+ let mut foundver = false;
+ for i in 0..response.version_number_entry_count {
+ // Creating a reference on a packed struct will result in
+ // undefined behaviour, so we operate on the raw data directly
+ let unaligned = core::ptr::addr_of_mut!(response.version_number_entries) as *mut u16;
+ let addr = unaligned.wrapping_add(i as usize);
+ let version = (unsafe { core::ptr::read_unaligned::<u16>(addr) } >> 8) as u8;
+
+ if version >= self.version && version <= SPDM_MAX_VER {
+ self.version = version;
+ foundver = true;
+ }
+ }
+
+ if !foundver {
+ pr_err!("No common supported version\n");
+ to_result(-(bindings::EPROTO as i32))?;
+ }
+
+ Ok(())
+ }
}
diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
index a0a3a2f46952..f69be6aa6280 100644
--- a/lib/rspdm/validator.rs
+++ b/lib/rspdm/validator.rs
@@ -7,6 +7,7 @@
//! Rust implementation of the DMTF Security Protocol and Data Model (SPDM)
//! <https://www.dmtf.org/dsp/DSP0274>
+use crate::bindings::{__IncompleteArrayField, __le16};
use crate::consts::SpdmErrorCode;
use core::mem;
use kernel::prelude::*;
@@ -15,6 +16,8 @@
validate::{Unvalidated, Validate},
};
+use crate::consts::SPDM_GET_VERSION;
+
#[repr(C, packed)]
pub(crate) struct SpdmHeader {
pub(crate) version: u8,
@@ -64,3 +67,67 @@ pub(crate) struct SpdmErrorRsp {
pub(crate) error_code: SpdmErrorCode,
pub(crate) error_data: u8,
}
+
+#[repr(C, packed)]
+pub(crate) struct GetVersionReq {
+ pub(crate) version: u8,
+ pub(crate) code: u8,
+ pub(crate) param1: u8,
+ pub(crate) param2: u8,
+}
+
+impl Default for GetVersionReq {
+ fn default() -> Self {
+ GetVersionReq {
+ version: 0,
+ code: SPDM_GET_VERSION,
+ param1: 0,
+ param2: 0,
+ }
+ }
+}
+
+#[repr(C, packed)]
+pub(crate) struct GetVersionRsp {
+ pub(crate) version: u8,
+ pub(crate) code: u8,
+ param1: u8,
+ param2: u8,
+ reserved: u8,
+ pub(crate) version_number_entry_count: u8,
+ pub(crate) version_number_entries: __IncompleteArrayField<__le16>,
+}
+
+impl Validate<&mut Unvalidated<KVec<u8>>> for &mut GetVersionRsp {
+ type Err = Error;
+
+ fn validate(unvalidated: &mut Unvalidated<KVec<u8>>) -> Result<Self, Self::Err> {
+ let raw = unvalidated.raw_mut();
+ if raw.len() < mem::size_of::<GetVersionRsp>() {
+ return Err(EINVAL);
+ }
+
+ let version_number_entries = *(raw.get(5).ok_or(ENOMEM))? as usize;
+ let total_expected_size = version_number_entries * 2 + 6;
+ if raw.len() < total_expected_size {
+ return Err(EINVAL);
+ }
+
+ let ptr = raw.as_mut_ptr();
+ // CAST: `GetVersionRsp` only contains integers and has `repr(C)`.
+ let ptr = ptr.cast::<GetVersionRsp>();
+ // SAFETY: `ptr` came from a reference and the cast above is valid.
+ let rsp: &mut GetVersionRsp = unsafe { &mut *ptr };
+
+ // Creating a reference on a packed struct will result in
+ // undefined behaviour, so we operate on the raw data directly
+ let unaligned = core::ptr::addr_of_mut!(rsp.version_number_entries) as *mut u16;
+ for version_offset in 0..version_number_entries {
+ let addr = unaligned.wrapping_add(version_offset);
+ let version = unsafe { core::ptr::read_unaligned::<u16>(addr) };
+ unsafe { core::ptr::write_unaligned::<u16>(addr, version.to_le()) }
+ }
+
+ Ok(rsp)
+ }
+}
--
2.52.0On Wed, 11 Feb 2026 13:29:19 +1000
alistair23@gmail.com wrote:
> From: Alistair Francis <alistair@alistair23.me>
>
> Support the GET_VERSION SPDM command.
>
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
Hi Alistair,
Various minor comments inline. Biggest one is what I think
is some confusing le16 handling that to me is in the wrong place.
Jonathan
> ---
> lib/rspdm/consts.rs | 17 +++++++++++
> lib/rspdm/lib.rs | 6 +++-
> lib/rspdm/state.rs | 58 ++++++++++++++++++++++++++++++++++--
> lib/rspdm/validator.rs | 67 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 144 insertions(+), 4 deletions(-)
>
> diff --git a/lib/rspdm/consts.rs b/lib/rspdm/consts.rs
> index 40ce60eba2f3..38f48f0863e2 100644
> --- a/lib/rspdm/consts.rs
> +++ b/lib/rspdm/consts.rs
> @@ -115,3 +129,6 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> Ok(())
> }
> }
> +
> +pub(crate) const SPDM_GET_VERSION: u8 = 0x84;
> +pub(crate) const SPDM_GET_VERSION_LEN: usize = mem::size_of::<SpdmHeader>() + 255;
I'd either add a comment on where the 255 comes from. Also do we have a U8_MAX definition
we can use? Or some way to get it from code?
> diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs
> index 2bb716140e0a..08abcbb21247 100644
> --- a/lib/rspdm/lib.rs
> +++ b/lib/rspdm/lib.rs
> @@ -108,7 +108,11 @@
> /// Return 0 on success or a negative errno. In particular, -EPROTONOSUPPORT
> /// indicates authentication is not supported by the device.
> #[no_mangle]
> -pub unsafe extern "C" fn spdm_authenticate(_state: &'static mut SpdmState) -> c_int {
> +pub unsafe extern "C" fn spdm_authenticate(state: &'static mut SpdmState) -> c_int {
Is there more to this rename than it appears? Can it get pushed back to earlier patch?
> + if let Err(e) = state.get_version() {
> + return e.to_errno() as c_int;
> + }
> +
> 0
> }
>
> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index 68861f30e3fa..3ad53ec05044 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
> @@ -8,6 +8,7 @@
> //! <https://www.dmtf.org/dsp/DSP0274>
>
> use core::ffi::c_void;
> +use core::slice::from_raw_parts_mut;
> use kernel::prelude::*;
> use kernel::{
> bindings,
> @@ -15,8 +16,10 @@
> validate::Untrusted,
> };
>
> -use crate::consts::{SpdmErrorCode, SPDM_ERROR, SPDM_REQ};
> -use crate::validator::{SpdmErrorRsp, SpdmHeader};
> +use crate::consts::{
> + SpdmErrorCode, SPDM_ERROR, SPDM_GET_VERSION_LEN, SPDM_MAX_VER, SPDM_MIN_VER, SPDM_REQ,
Is the Rust linter fussy about these being on one line? Feels like we'd
get less churn over time if we formatted this as one per line.
> +};
> +use crate::validator::{GetVersionReq, GetVersionRsp, SpdmErrorRsp, SpdmHeader};
>
> /// The current SPDM session state for a device. Based on the
> /// C `struct spdm_state`.
> @@ -61,7 +64,7 @@ pub(crate) fn new(
> transport_sz,
> keyring,
> validate,
> - version: 0x10,
> + version: SPDM_MIN_VER,
Pull that def back to earlier patch to keep the churn confined.
> }
> }
>
> @@ -217,4 +220,53 @@ pub(crate) fn spdm_exchange(
>
> Ok(length)
> }
> +
> + /// Negoiate a supported SPDM version and store the information
> + /// in the `SpdmState`.
> + pub(crate) fn get_version(&mut self) -> Result<(), Error> {
> + let mut request = GetVersionReq::default();
> + request.version = self.version;
> +
> + // 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,
> + core::mem::size_of::<GetVersionReq>(),
> + )
> + };
> +
> + let mut response_vec: KVec<u8> = KVec::with_capacity(SPDM_GET_VERSION_LEN, GFP_KERNEL)?;
> + // SAFETY: `request` is repr(C) and packed, so we can convert it to a slice
> + let response_buf =
> + unsafe { from_raw_parts_mut(response_vec.as_mut_ptr(), SPDM_GET_VERSION_LEN) };
> +
> + let rc = self.spdm_exchange(request_buf, response_buf)?;
> +
> + // SAFETY: `rc` is the length of data read, which will be smaller
> + // then the capacity of the vector
than?
> + unsafe { response_vec.inc_len(rc as usize) };
> +
> + let response: &mut GetVersionRsp = Untrusted::new_mut(&mut response_vec).validate_mut()?;
> +
> + let mut foundver = false;
> + for i in 0..response.version_number_entry_count {
> + // Creating a reference on a packed struct will result in
> + // undefined behaviour, so we operate on the raw data directly
> + let unaligned = core::ptr::addr_of_mut!(response.version_number_entries) as *mut u16;
Maybe pull that out of the loop like you do below?
> + let addr = unaligned.wrapping_add(i as usize);
> + let version = (unsafe { core::ptr::read_unaligned::<u16>(addr) } >> 8) as u8;
Given the endian conversion below, this is correct, but I think leaving it as the __le16 until
here is better. We could also just pull out the correct byte and ignore the other one.
It's a spec quirk that they decided to have it defined as a __le16 rather than u8[2] as
the fields are confined one byte or the other.
I wonder if we should also reject any alpha versions? I.e. check the bottom 4 bits
are 0.
> +
> + if version >= self.version && version <= SPDM_MAX_VER {
> + self.version = version;
> + foundver = true;
> + }
> + }
> +
> + if !foundver {
> + pr_err!("No common supported version\n");
> + to_result(-(bindings::EPROTO as i32))?;
> + }
> +
> + Ok(())
> + }
> }
> diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> index a0a3a2f46952..f69be6aa6280 100644
> --- a/lib/rspdm/validator.rs
> +++ b/lib/rspdm/validator.rs
> @@ -7,6 +7,7 @@
> //! Rust implementation of the DMTF Security Protocol and Data Model (SPDM)
> //! <https://www.dmtf.org/dsp/DSP0274>
>
> +use crate::bindings::{__IncompleteArrayField, __le16};
> use crate::consts::SpdmErrorCode;
> use core::mem;
> use kernel::prelude::*;
> @@ -15,6 +16,8 @@
> validate::{Unvalidated, Validate},
> };
>
> +use crate::consts::SPDM_GET_VERSION;
> +
> #[repr(C, packed)]
> pub(crate) struct SpdmHeader {
> pub(crate) version: u8,
> @@ -64,3 +67,67 @@ pub(crate) struct SpdmErrorRsp {
> pub(crate) error_code: SpdmErrorCode,
> pub(crate) error_data: u8,
> }
> +
> +#[repr(C, packed)]
Why packed?
> +pub(crate) struct GetVersionReq {
> + pub(crate) version: u8,
> + pub(crate) code: u8,
> + pub(crate) param1: u8,
> + pub(crate) param2: u8,
> +}
> +
> +impl Default for GetVersionReq {
> + fn default() -> Self {
> + GetVersionReq {
> + version: 0,
Given this only takes one value we could default it to 0x10
(even though it's written above to keep the state machine in sync).
> + code: SPDM_GET_VERSION,
> + param1: 0,
> + param2: 0,
> + }
> + }
> +}
> +
> +#[repr(C, packed)]
> +pub(crate) struct GetVersionRsp {
> + pub(crate) version: u8,
> + pub(crate) code: u8,
> + param1: u8,
> + param2: u8,
> + reserved: u8,
> + pub(crate) version_number_entry_count: u8,
> + pub(crate) version_number_entries: __IncompleteArrayField<__le16>,
> +}
> +
> +impl Validate<&mut Unvalidated<KVec<u8>>> for &mut GetVersionRsp {
> + type Err = Error;
> +
> + fn validate(unvalidated: &mut Unvalidated<KVec<u8>>) -> Result<Self, Self::Err> {
> + let raw = unvalidated.raw_mut();
> + if raw.len() < mem::size_of::<GetVersionRsp>() {
> + return Err(EINVAL);
> + }
I guess it probably belongs at the header validation point rather that here,
but can we also check the version matches what was requested. Possibly an
additional check here that it is 0x10 (as this one is a special case where
only that value is correct).
> +
> + let version_number_entries = *(raw.get(5).ok_or(ENOMEM))? as usize;
> + let total_expected_size = version_number_entries * 2 + 6;
Instead of 6 can we use mem::size_of::<GetVersionRsp>()?
Ideally something similar for the 2 as well even if it's a bit verbose.
(if I was lazy in the C code you get to make it better here :)
> + if raw.len() < total_expected_size {
> + return Err(EINVAL);
> + }
> +
> + let ptr = raw.as_mut_ptr();
> + // CAST: `GetVersionRsp` only contains integers and has `repr(C)`.
> + let ptr = ptr.cast::<GetVersionRsp>();
> + // SAFETY: `ptr` came from a reference and the cast above is valid.
> + let rsp: &mut GetVersionRsp = unsafe { &mut *ptr };
> +
> + // Creating a reference on a packed struct will result in
Silly question, but why is it packed? We might need to do this for some
structures in SPDM but do we have transports that mess up the alignment enough
that where the messages themselves don't need to be packed we have to mark
the structures so anyway? Probably good to document this somewhere.
> + // undefined behaviour, so we operate on the raw data directly
> + let unaligned = core::ptr::addr_of_mut!(rsp.version_number_entries) as *mut u16;
> + for version_offset in 0..version_number_entries {
> + let addr = unaligned.wrapping_add(version_offset);
> + let version = unsafe { core::ptr::read_unaligned::<u16>(addr) };
> + unsafe { core::ptr::write_unaligned::<u16>(addr, version.to_le()) }
I'd like to see a comment on why this seems to be doing an endian swap if we
are on big endian. Looking back at the c code, it has an endian swap but
only as part of the search for a version to use (finding the largest both
device and and kernel support).
Maybe I'm reading it wrong but isn't this putting cpu endian data into a structure
element that is of type __le16?
> + }
> +
> + Ok(rsp)
> + }
> +}
On Tue, Mar 3, 2026 at 9:36 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Wed, 11 Feb 2026 13:29:19 +1000
> alistair23@gmail.com wrote:
>
> > From: Alistair Francis <alistair@alistair23.me>
> >
> > Support the GET_VERSION SPDM command.
> >
> > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> Hi Alistair,
>
> Various minor comments inline. Biggest one is what I think
> is some confusing le16 handling that to me is in the wrong place.
>
> Jonathan
>
> > ---
> > lib/rspdm/consts.rs | 17 +++++++++++
> > lib/rspdm/lib.rs | 6 +++-
> > lib/rspdm/state.rs | 58 ++++++++++++++++++++++++++++++++++--
> > lib/rspdm/validator.rs | 67 ++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 144 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/rspdm/consts.rs b/lib/rspdm/consts.rs
> > index 40ce60eba2f3..38f48f0863e2 100644
> > --- a/lib/rspdm/consts.rs
> > +++ b/lib/rspdm/consts.rs
>
> > @@ -115,3 +129,6 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> > Ok(())
> > }
> > }
> > +
> > +pub(crate) const SPDM_GET_VERSION: u8 = 0x84;
> > +pub(crate) const SPDM_GET_VERSION_LEN: usize = mem::size_of::<SpdmHeader>() + 255;
>
> I'd either add a comment on where the 255 comes from. Also do we have a U8_MAX definition
> we can use? Or some way to get it from code?
Fixed, can just use `u8::MAX`
>
> > diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs
> > index 2bb716140e0a..08abcbb21247 100644
> > --- a/lib/rspdm/lib.rs
> > +++ b/lib/rspdm/lib.rs
> > @@ -108,7 +108,11 @@
> > /// Return 0 on success or a negative errno. In particular, -EPROTONOSUPPORT
> > /// indicates authentication is not supported by the device.
> > #[no_mangle]
> > -pub unsafe extern "C" fn spdm_authenticate(_state: &'static mut SpdmState) -> c_int {
> > +pub unsafe extern "C" fn spdm_authenticate(state: &'static mut SpdmState) -> c_int {
>
> Is there more to this rename than it appears? Can it get pushed back to earlier patch?
Rust will complain about `state` being unused, by adding a `_` to the
front we don't get the warning in the previous patch. So in this patch
I rename it back to `state` as it's now used.
This is a public function that matches the C implementation, hence the
arguments being fixed in the previous patch.
>
> > + if let Err(e) = state.get_version() {
> > + return e.to_errno() as c_int;
> > + }
> > +
> > 0
> > }
> >
> > diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> > index 68861f30e3fa..3ad53ec05044 100644
> > --- a/lib/rspdm/state.rs
> > +++ b/lib/rspdm/state.rs
> > @@ -8,6 +8,7 @@
> > //! <https://www.dmtf.org/dsp/DSP0274>
> >
> > use core::ffi::c_void;
> > +use core::slice::from_raw_parts_mut;
> > use kernel::prelude::*;
> > use kernel::{
> > bindings,
> > @@ -15,8 +16,10 @@
> > validate::Untrusted,
> > };
> >
> > -use crate::consts::{SpdmErrorCode, SPDM_ERROR, SPDM_REQ};
> > -use crate::validator::{SpdmErrorRsp, SpdmHeader};
> > +use crate::consts::{
> > + SpdmErrorCode, SPDM_ERROR, SPDM_GET_VERSION_LEN, SPDM_MAX_VER, SPDM_MIN_VER, SPDM_REQ,
>
> Is the Rust linter fussy about these being on one line? Feels like we'd
> get less churn over time if we formatted this as one per line.
This is just how the linter formats it when I run ` LLVM=1 make
rustfmt`. I don't really want to be manually formatting things
>
> > +};
> > +use crate::validator::{GetVersionReq, GetVersionRsp, SpdmErrorRsp, SpdmHeader};
> >
> > /// The current SPDM session state for a device. Based on the
> > /// C `struct spdm_state`.
> > @@ -61,7 +64,7 @@ pub(crate) fn new(
> > transport_sz,
> > keyring,
> > validate,
> > - version: 0x10,
> > + version: SPDM_MIN_VER,
>
> Pull that def back to earlier patch to keep the churn confined.
Fixed
>
> > }
> > }
> >
> > @@ -217,4 +220,53 @@ pub(crate) fn spdm_exchange(
> >
> > Ok(length)
> > }
> > +
> > + /// Negoiate a supported SPDM version and store the information
> > + /// in the `SpdmState`.
> > + pub(crate) fn get_version(&mut self) -> Result<(), Error> {
> > + let mut request = GetVersionReq::default();
> > + request.version = self.version;
> > +
> > + // 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,
> > + core::mem::size_of::<GetVersionReq>(),
> > + )
> > + };
> > +
> > + let mut response_vec: KVec<u8> = KVec::with_capacity(SPDM_GET_VERSION_LEN, GFP_KERNEL)?;
> > + // SAFETY: `request` is repr(C) and packed, so we can convert it to a slice
> > + let response_buf =
> > + unsafe { from_raw_parts_mut(response_vec.as_mut_ptr(), SPDM_GET_VERSION_LEN) };
> > +
> > + let rc = self.spdm_exchange(request_buf, response_buf)?;
> > +
> > + // SAFETY: `rc` is the length of data read, which will be smaller
> > + // then the capacity of the vector
>
> than?
Fixed
>
> > + unsafe { response_vec.inc_len(rc as usize) };
> > +
> > + let response: &mut GetVersionRsp = Untrusted::new_mut(&mut response_vec).validate_mut()?;
> > +
> > + let mut foundver = false;
> > + for i in 0..response.version_number_entry_count {
> > + // Creating a reference on a packed struct will result in
> > + // undefined behaviour, so we operate on the raw data directly
> > + let unaligned = core::ptr::addr_of_mut!(response.version_number_entries) as *mut u16;
>
> Maybe pull that out of the loop like you do below?
Fixed
>
> > + let addr = unaligned.wrapping_add(i as usize);
> > + let version = (unsafe { core::ptr::read_unaligned::<u16>(addr) } >> 8) as u8;
> Given the endian conversion below, this is correct, but I think leaving it as the __le16 until
> here is better. We could also just pull out the correct byte and ignore the other one.
Hmm... I'm not sure I follow.
The SPDM spec describes this as a u16, so I'm reading the entire value
and just taking the upper bits for the Major/Minor version
So I'm not sure what you think I should do differently?
> It's a spec quirk that they decided to have it defined as a __le16 rather than u8[2] as
> the fields are confined one byte or the other.
>
> I wonder if we should also reject any alpha versions? I.e. check the bottom 4 bits
> are 0.
Hmmm... That seems a bit unnecessary. Maybe a warning though?
>
> > +
> > + if version >= self.version && version <= SPDM_MAX_VER {
> > + self.version = version;
> > + foundver = true;
> > + }
> > + }
> > +
> > + if !foundver {
> > + pr_err!("No common supported version\n");
> > + to_result(-(bindings::EPROTO as i32))?;
> > + }
> > +
> > + Ok(())
> > + }
> > }
> > diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> > index a0a3a2f46952..f69be6aa6280 100644
> > --- a/lib/rspdm/validator.rs
> > +++ b/lib/rspdm/validator.rs
> > @@ -7,6 +7,7 @@
> > //! Rust implementation of the DMTF Security Protocol and Data Model (SPDM)
> > //! <https://www.dmtf.org/dsp/DSP0274>
> >
> > +use crate::bindings::{__IncompleteArrayField, __le16};
> > use crate::consts::SpdmErrorCode;
> > use core::mem;
> > use kernel::prelude::*;
> > @@ -15,6 +16,8 @@
> > validate::{Unvalidated, Validate},
> > };
> >
> > +use crate::consts::SPDM_GET_VERSION;
> > +
> > #[repr(C, packed)]
> > pub(crate) struct SpdmHeader {
> > pub(crate) version: u8,
> > @@ -64,3 +67,67 @@ pub(crate) struct SpdmErrorRsp {
> > pub(crate) error_code: SpdmErrorCode,
> > pub(crate) error_data: u8,
> > }
> > +
> > +#[repr(C, packed)]
>
> Why packed?
We cast it from a struct to a slice (array), so we need it to be
packed to avoid gaps in the slice (array)
>
> > +pub(crate) struct GetVersionReq {
> > + pub(crate) version: u8,
> > + pub(crate) code: u8,
> > + pub(crate) param1: u8,
> > + pub(crate) param2: u8,
> > +}
> > +
> > +impl Default for GetVersionReq {
> > + fn default() -> Self {
> > + GetVersionReq {
> > + version: 0,
>
> Given this only takes one value we could default it to 0x10
> (even though it's written above to keep the state machine in sync).
We could, I do find that less clear though. Explicit writing it as
part of `get_version()` to me is really clear what is happening
>
> > + code: SPDM_GET_VERSION,
> > + param1: 0,
> > + param2: 0,
> > + }
> > + }
> > +}
> > +
> > +#[repr(C, packed)]
> > +pub(crate) struct GetVersionRsp {
> > + pub(crate) version: u8,
> > + pub(crate) code: u8,
> > + param1: u8,
> > + param2: u8,
> > + reserved: u8,
> > + pub(crate) version_number_entry_count: u8,
> > + pub(crate) version_number_entries: __IncompleteArrayField<__le16>,
> > +}
> > +
> > +impl Validate<&mut Unvalidated<KVec<u8>>> for &mut GetVersionRsp {
> > + type Err = Error;
> > +
> > + fn validate(unvalidated: &mut Unvalidated<KVec<u8>>) -> Result<Self, Self::Err> {
> > + let raw = unvalidated.raw_mut();
> > + if raw.len() < mem::size_of::<GetVersionRsp>() {
> > + return Err(EINVAL);
> > + }
>
> I guess it probably belongs at the header validation point rather that here,
> but can we also check the version matches what was requested. Possibly an
> additional check here that it is 0x10 (as this one is a special case where
> only that value is correct).
Good point, I have added a check here
>
> > +
> > + let version_number_entries = *(raw.get(5).ok_or(ENOMEM))? as usize;
> > + let total_expected_size = version_number_entries * 2 + 6;
>
> Instead of 6 can we use mem::size_of::<GetVersionRsp>()?
Yep! Good idea
> Ideally something similar for the 2 as well even if it's a bit verbose.
> (if I was lazy in the C code you get to make it better here :)
let total_expected_size = version_number_entries *
mem::size_of::<__le16>() + mem::size_of::<GetVersionRsp>();
>
>
> > + if raw.len() < total_expected_size {
> > + return Err(EINVAL);
> > + }
> > +
> > + let ptr = raw.as_mut_ptr();
> > + // CAST: `GetVersionRsp` only contains integers and has `repr(C)`.
> > + let ptr = ptr.cast::<GetVersionRsp>();
> > + // SAFETY: `ptr` came from a reference and the cast above is valid.
> > + let rsp: &mut GetVersionRsp = unsafe { &mut *ptr };
> > +
> > + // Creating a reference on a packed struct will result in
>
> Silly question, but why is it packed? We might need to do this for some
> structures in SPDM but do we have transports that mess up the alignment enough
> that where the messages themselves don't need to be packed we have to mark
> the structures so anyway? Probably good to document this somewhere.
Like above, the Rust structs contain the information which we cast to
the slice (u8 array). So we want it to be packed to avoid gaps in the
buffer.
>
> > + // undefined behaviour, so we operate on the raw data directly
> > + let unaligned = core::ptr::addr_of_mut!(rsp.version_number_entries) as *mut u16;
> > + for version_offset in 0..version_number_entries {
> > + let addr = unaligned.wrapping_add(version_offset);
> > + let version = unsafe { core::ptr::read_unaligned::<u16>(addr) };
> > + unsafe { core::ptr::write_unaligned::<u16>(addr, version.to_le()) }
>
> I'd like to see a comment on why this seems to be doing an endian swap if we
> are on big endian. Looking back at the c code, it has an endian swap but
> only as part of the search for a version to use (finding the largest both
> device and and kernel support).
>
> Maybe I'm reading it wrong but isn't this putting cpu endian data into a structure
> element that is of type __le16?
On second thought I don't think this is required at all. It's
converting the LE data from the SPDM response to LE. Which is just
unnecessary no-op (LE) or a bug (BE) depending on the CPU endianness.
Alistair
>
> > + }
> > +
> > + Ok(rsp)
> > + }
> > +}
>
>
> >
> > > + let addr = unaligned.wrapping_add(i as usize);
> > > + let version = (unsafe { core::ptr::read_unaligned::<u16>(addr) } >> 8) as u8;
> > Given the endian conversion below, this is correct, but I think leaving it as the __le16 until
> > here is better. We could also just pull out the correct byte and ignore the other one.
>
> Hmm... I'm not sure I follow.
>
> The SPDM spec describes this as a u16, so I'm reading the entire value
> and just taking the upper bits for the Major/Minor version
Miss read by me I think... Or tied up with endian conversion I think you
say you are getting rid of below.
>
> So I'm not sure what you think I should do differently?
>
> > It's a spec quirk that they decided to have it defined as a __le16 rather than u8[2] as
> > the fields are confined one byte or the other.
> >
> > I wonder if we should also reject any alpha versions? I.e. check the bottom 4 bits
> > are 0.
>
> Hmmm... That seems a bit unnecessary. Maybe a warning though?
Warning works for me.
>
> >
> > > +
> > > + if version >= self.version && version <= SPDM_MAX_VER {
> > > + self.version = version;
> > > + foundver = true;
> > > + }
> > > + }
> > > +
> > > + if !foundver {
> > > + pr_err!("No common supported version\n");
> > > + to_result(-(bindings::EPROTO as i32))?;
> > > + }
> > > +
> > > + Ok(())
> > > + }
> > > }
> > > diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> > > index a0a3a2f46952..f69be6aa6280 100644
> > > --- a/lib/rspdm/validator.rs
> > > +++ b/lib/rspdm/validator.rs
> > > @@ -7,6 +7,7 @@
> > > //! Rust implementation of the DMTF Security Protocol and Data Model (SPDM)
> > > //! <https://www.dmtf.org/dsp/DSP0274>
> > >
> > > +use crate::bindings::{__IncompleteArrayField, __le16};
> > > use crate::consts::SpdmErrorCode;
> > > use core::mem;
> > > use kernel::prelude::*;
> > > @@ -15,6 +16,8 @@
> > > validate::{Unvalidated, Validate},
> > > };
> > >
> > > +use crate::consts::SPDM_GET_VERSION;
> > > +
> > > #[repr(C, packed)]
> > > pub(crate) struct SpdmHeader {
> > > pub(crate) version: u8,
> > > @@ -64,3 +67,67 @@ pub(crate) struct SpdmErrorRsp {
> > > pub(crate) error_code: SpdmErrorCode,
> > > pub(crate) error_data: u8,
> > > }
> > > +
> > > +#[repr(C, packed)]
> >
> > Why packed?
>
> We cast it from a struct to a slice (array), so we need it to be
> packed to avoid gaps in the slice (array)
Ah ok. I'm learning slowly...
>
> >
> > > +pub(crate) struct GetVersionReq {
> > > + pub(crate) version: u8,
> > > + pub(crate) code: u8,
> > > + pub(crate) param1: u8,
> > > + pub(crate) param2: u8,
> > > +}
> > > +
> > > + // undefined behaviour, so we operate on the raw data directly
> > > + let unaligned = core::ptr::addr_of_mut!(rsp.version_number_entries) as *mut u16;
> > > + for version_offset in 0..version_number_entries {
> > > + let addr = unaligned.wrapping_add(version_offset);
> > > + let version = unsafe { core::ptr::read_unaligned::<u16>(addr) };
> > > + unsafe { core::ptr::write_unaligned::<u16>(addr, version.to_le()) }
> >
> > I'd like to see a comment on why this seems to be doing an endian swap if we
> > are on big endian. Looking back at the c code, it has an endian swap but
> > only as part of the search for a version to use (finding the largest both
> > device and and kernel support).
> >
> > Maybe I'm reading it wrong but isn't this putting cpu endian data into a structure
> > element that is of type __le16?
>
> On second thought I don't think this is required at all. It's
> converting the LE data from the SPDM response to LE. Which is just
> unnecessary no-op (LE) or a bug (BE) depending on the CPU endianness.
I think that makes sense but I'll take a close look at v4.
J
>
> Alistair
>
> >
> > > + }
> > > +
> > > + Ok(rsp)
> > > + }
> > > +}
> >
On Fri, Mar 13, 2026 at 6:36 AM Alistair Francis <alistair23@gmail.com> wrote: > > This is a public function that matches the C implementation, hence the > arguments being fixed in the previous patch. Since you have it in a C header and in `bindings_helper.h`, then you should be able to use `#[export]`, which will do a typecheck that the signatures match (it also does the `no_mangle`, so you can replace it): https://rust.docs.kernel.org/macros/attr.export.html > This is just how the linter formats it when I run ` LLVM=1 make > rustfmt`. I don't really want to be manually formatting things Please use the "kernel vertical style" for imports (it is done automatically by `rustfmt` too by adding the `//`): https://docs.kernel.org/rust/coding-guidelines.html#imports We have `size_of`, `align_of` and so on in the prelude, so you can use those directly: https://rust.docs.kernel.org/kernel/prelude/index.html I hope that helps! Cheers, Miguel
On Fri, Mar 13, 2026 at 6:53 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > We have `size_of`, `align_of` and so on in the prelude, so you can use > those directly: > > https://rust.docs.kernel.org/kernel/prelude/index.html Sorry, I cut the quotes too much -- this was supposed to reply to the `total_expected_size` part of the conversation. Cheers, Miguel
This is cool! some minor comments inline.
>
> /// The current SPDM session state for a device. Based on the
> /// C `struct spdm_state`.
> @@ -61,7 +64,7 @@ pub(crate) fn new(
> transport_sz,
> keyring,
> validate,
> - version: 0x10,
> + version: SPDM_MIN_VER,
> }
> }
>
> @@ -217,4 +220,53 @@ pub(crate) fn spdm_exchange(
>
> Ok(length)
> }
> +
> + /// Negoiate a supported SPDM version and store the information
typo: s/Negoiate/Negotiate
> + /// in the `SpdmState`.
> + pub(crate) fn get_version(&mut self) -> Result<(), Error> {
> + let mut request = GetVersionReq::default();
> + request.version = self.version;
> +
> + // 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,
> + core::mem::size_of::<GetVersionReq>(),
> + )
> + };
> +
> + let mut response_vec: KVec<u8> =
> KVec::with_capacity(SPDM_GET_VERSION_LEN, GFP_KERNEL)?;
> + // SAFETY: `request` is repr(C) and packed, so we can
> convert it to a slice
The SAFETY comment here refers to `request` when setting up the
response buffer, seems unneeded?
Wilfred
© 2016 - 2026 Red Hat, Inc.