Re: [PATCH 12/18] lib: rspdm: Support SPDM get_capabilities

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Incorrect request and response buffer sizes for SPDM 1.0 and 1.1 GET_CAPABILITIES exchanges.
- [High] Missing little-endian to native-endian conversion for `response.data_transfer_size`.
- [High] `GetCapabilitiesRsp::validate` unconditionally rejects valid SPDM 1.0 responses.
- [High] Creating a mutable slice (`&mut [u8]`) from uninitialized memory causes Undefined Behavior.
- [Medium] Vector length bounds check does not prevent out-of-bounds capacity overflow if transport returns a larger length.
--

commit 1fee665b22607bae77947f31f9d54552e86f77d2
Author: Alistair Francis <alistair@alistair23.me>

lib: rspdm: Support SPDM get_capabilities

This commit adds support for the GET_CAPABILITIES SPDM command. It negotiates
responder capabilities and updates the SPDM state.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index 26b90942b298d..e7119ffa9a691 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs

[ ... ]

> @@ -293,4 +304,67 @@ pub(crate) fn get_version(&mut self) -> Result<(), Error> {
>  
>          Ok(())
>      }
> +
> +    /// Obtain the supported capabilities from an SPDM session and store the
> +    /// information in the `SpdmState`.
> +    pub(crate) fn get_capabilities(&mut self) -> Result<(), Error> {
> +        let mut request = GetCapabilitiesReq::default();
> +        request.version = self.version;
> +
> +        let (req_sz, rsp_sz) = match self.version {
> +            SPDM_VER_10 => (4, 8),
> +            SPDM_VER_11 => (8, 8),

Are these buffer sizes correct for SPDM 1.0 and 1.1?

According to the SPDM specification, both SPDM 1.0 and 1.1 responses include
a mandatory 4-byte flags field, which would make their true size 12 bytes.
Furthermore, the 1.1 request is also 12 bytes.

Additionally, since GetCapabilitiesRsp::validate() enforces a minimum length
of 12 bytes for SPDM 1.1, wouldn't an 8-byte response unconditionally fail
validation with EINVAL?

> +            _ => {
> +                request.data_transfer_size = self.transport_sz.to_le();
> +                request.max_spdm_msg_size = request.data_transfer_size;
> +
> +                (
> +                    core::mem::size_of::<GetCapabilitiesReq>(),
> +                    core::mem::size_of::<GetCapabilitiesRsp>(),
> +                )
> +            }
> +        };
> +
> +        // 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, req_sz) };
> +
> +        let mut response_vec: KVec<u8> = KVec::with_capacity(rsp_sz, GFP_KERNEL)?;
> +        // SAFETY: `response_vec` is rsp_sz length, initialised, aligned
> +        // and won't be mutated
> +        let response_buf = unsafe { from_raw_parts_mut(response_vec.as_mut_ptr(), rsp_sz) };

Does creating a mutable slice from this vector trigger undefined behavior?

The safety comment mentions the memory is initialized, but
KVec::with_capacity() allocates uninitialized capacity. Creating a &mut [u8]
reference to uninitialized memory is undefined behavior in Rust.

Should this use something like KVec::resize() or MaybeUninit to ensure the
memory is initialized before slicing?

> +
> +        let rc = self.spdm_exchange(request_buf, response_buf)?;
> +
> +        if rc < (rsp_sz as i32) {
> +            pr_err!("Truncated capabilities response\n");
> +            to_result(-(bindings::EIO as i32))?;
> +        }
> +
> +        // SAFETY: `rc` is the length of data read, which will be smaller
> +        // or the same as the capacity of the vector
> +        unsafe { response_vec.inc_len(rc as usize) };

The code checks if rc is truncated, but what if a buggy transport driver
returns an rc strictly larger than rsp_sz?

If that happens, response_vec.inc_len() would force the length beyond the
allocated capacity, leading to out-of-bounds memory accesses. Should there
be a check to ensure rc <= rsp_sz?

> +
> +        let response: &mut GetCapabilitiesRsp =
> +            Untrusted::new_mut(&mut response_vec).validate_mut()?;
> +
> +        self.rsp_caps = u32::from_le(response.flags);
> +        if (self.rsp_caps & SPDM_RSP_MIN_CAPS) != SPDM_RSP_MIN_CAPS {
> +            pr_err!(
> +                "{:#x} capabilities are supported, which don't meet required {:#x}",
> +                self.rsp_caps,
> +                SPDM_RSP_MIN_CAPS
> +            );
> +            to_result(-(bindings::EPROTONOSUPPORT as i32))?;
> +        }
> +
> +        if self.version >= SPDM_VER_12 {
> +            if response.data_transfer_size < SPDM_MIN_DATA_TRANSFER_SIZE {

Is an endianness conversion missing here?

The SPDM protocol transmits multi-byte fields in little-endian format, but
response.data_transfer_size is read directly here.

On big-endian architectures, could this be read as an artificially massive
integer and bypass the minimum size bounds check? Should this use
u32::from_le() like the flags field above it?

> +                pr_err!("Malformed capabilities response\n");
> +                to_result(-(bindings::EPROTO as i32))?;
> +            }
> +            self.transport_sz = self.transport_sz.min(response.data_transfer_size);
> +        }
> +
> +        Ok(())
> +    }
>  }

> diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> index 8f45bafd4d691..7dc55882c8809 100644
> --- a/lib/rspdm/validator.rs
> +++ b/lib/rspdm/validator.rs

[ ... ]

> @@ -141,3 +145,94 @@ fn validate(unvalidated: &mut Unvalidated<KVec<u8>>) -> Result<Self, Self::Err>
>          Ok(rsp)
>      }
>  }

[ ... ]

> +impl Validate<&mut Unvalidated<KVec<u8>>> for &mut GetCapabilitiesRsp {
> +    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::<GetCapabilitiesRsp>() {
> +            let version = *(raw.get(0).ok_or(ENOMEM))?;
> +            let version_1_1_len = mem::size_of::<GetCapabilitiesRsp>()
> +                - mem::size_of::<__IncompleteArrayField<__le16>>()
> +                - mem::size_of::<u32>()
> +                - mem::size_of::<u32>();
> +
> +            if version == SPDM_VER_11 && raw.len() == version_1_1_len {
> +                // Version 1.1 of the spec doesn't include all of the fields
> +                // So let's extend the KVec with 0s so we can cast the
> +                // vector to GetCapabilitiesRsp
> +
> +                for _i in version_1_1_len..mem::size_of::<GetCapabilitiesRsp>() {
> +                    raw.push(0, GFP_KERNEL)?;
> +                }
> +            } else {
> +                return Err(EINVAL);
> +            }

Does this unintentionally reject valid SPDM 1.0 responses?

If the SPDM session negotiated SPDM 1.0, the response will carry the
SPDM_VER_10 version byte and have a shorter response length. Because this
strictly checks against SPDM_VER_11, won't SPDM 1.0 responses fall through
to the else branch and unconditionally return EINVAL?

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

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