Re: [PATCH 15/18] lib: rspdm: Support SPDM get_certificate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Unbounded lifetime in `Validate` implementation subverts borrow checker, causing strict aliasing UB.
- [High] Improper vector length accumulation via `inc_len` causes capacity overflow and memory corruption.
- [High] Creating an uninitialized memory slice causes Undefined Behavior.
- [High] Out-of-bounds memory reference creation before verifying `certs_buf` length.
- [High] Missing little-endian conversions for SPDM fields break the protocol on Big Endian architectures.
- [High] Raw `u16` arithmetic on untrusted network lengths can overflow and panic the kernel.
- [Medium] The validation of the certificate response size incorrectly checks against the request struct.
--

commit 2a3b232f8fe8f4917dbcbbbbc545cdee33becabb
Author: Alistair Francis <alistair@alistair23.me>

lib: rspdm: Support SPDM get_certificate

This commit adds support for the SPDM GET_CERTIFICATE command. It introduces
the get_certificate function and related state exchange logic to retrieve
and validate certificate chains.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index bcb1cc955c4c6..69b6f67a6ef52 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> @@ -661,4 +671,119 @@ pub(crate) fn get_digests(&mut self) -> Result<(), Error> {
>  
>          Ok(())
>      }
> +
> +    fn get_cert_exchange(
> +        &mut self,
> +        request_buf: &mut [u8],
> +        response_vec: &mut KVec<u8>,
> +        rsp_sz: usize,
> +    ) -> Result<&mut GetCertificateRsp, Error> {
> +        // 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 this create a slice over uninitialized memory? Since response_vec was
allocated with KVec::with_capacity(rsp_sz, GFP_KERNEL), it hasn't been
initialized to valid u8 values.

> +
> +        let rc = self.spdm_exchange(request_buf, response_buf)?;
> +
> +        if rc < (core::mem::size_of::<GetCertificateReq>() as i32) {

Is it correct to validate the response size against GetCertificateReq here?

Should this check against GetCertificateRsp instead, in case the structures
diverge in size in the future?

> +            pr_err!("Truncated certificate response\n");
> +            to_result(-(bindings::EIO as i32))?;
> +        }
> +
> +        // 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) };

Since get_certificate() reuses response_vec in a while loop across multiple
exchanges without clearing it, does calling inc_len() here continuously
accumulate the length?

If spdm_exchange always writes to the start of the buffer, could this
accumulation eventually exceed the vector's allocated capacity?

[ ... ]
> +    pub(crate) fn get_certificate(&mut self, slot: u8) -> Result<(), Error> {
> +        let mut request = GetCertificateReq::default();
> +        request.version = self.version;
> +        request.param1 = slot;
> +
> +        let req_sz = core::mem::size_of::<GetCertificateReq>();
> +        let rsp_sz = (core::mem::size_of::<GetCertificateRsp>() as u32 + u16::MAX as u32)
> +            .min(self.transport_sz) as usize;
> +
> +        request.offset = 0;
> +        request.length = (rsp_sz - core::mem::size_of::<GetCertificateRsp>()) as u16;

Are we missing a conversion to little-endian for request.length here?

[ ... ]
> +        let response = self.get_cert_exchange(request_buf, &mut response_vec, rsp_sz)?;
> +
> +        let total_cert_len =
> +            ((response.portion_length + response.remainder_length) & 0xFFFF) as usize;

Since portion_length and remainder_length are read directly from the
untrusted SPDM response, do they need to be converted from little-endian?

Could this addition overflow u16 and cause a panic if the responder provides
large lengths? Should they be cast to usize before adding?

[ ... ]
> +        while remainder_length > 0 {
> +            request.offset = offset.to_le() as u16;

Does offset.to_le() perform a byte swap on the 64-bit usize before
truncating it to u16? On big-endian systems, wouldn't this shift zeroes
into the lower 16 bits and always send an offset of 0?

Should this be (offset as u16).to_le() instead?

[ ... ]
> +            if response.portion_length == 0
> +                || (response.param1 & 0xF) != slot
> +                || offset as u16 + response.portion_length + response.remainder_length
> +                    != total_cert_len as u16

Could this arithmetic overflow u16 if a malicious responder supplies large
length values?

[ ... ]
> +        let header_length = core::mem::size_of::<SpdmCertChain>() + self.hash_len;
> +
> +        let ptr = certs_buf.as_mut_ptr();
> +        // SAFETY: `SpdmCertChain` is repr(C) and packed, so we can convert it from a slice
> +        let ptr = ptr.cast::<SpdmCertChain>();
> +        // SAFETY: `ptr` came from a reference and the cast above is valid.
> +        let certs: &mut SpdmCertChain = unsafe { &mut *ptr };
> +
> +        if total_cert_len < header_length
> +            || total_cert_len as u16 != certs.length
> +            || total_cert_len != certs_buf.len()
> +        {

Is it safe to create the mutable reference to SpdmCertChain before verifying
the length of certs_buf?

If the responder replies with a truncated payload, could this create a
reference to unallocated memory? Should the boundary checks happen before
creating the reference?

Also, does certs.length need to be converted from little-endian before
comparing it here?

> diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> index 1e5ee8a7582b1..8b44a056b3354 100644
> --- a/lib/rspdm/validator.rs
> +++ b/lib/rspdm/validator.rs
[ ... ]
> +impl Validate<&mut Unvalidated<KVec<u8>>> for &mut GetCertificateRsp {
> +    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::<GetCertificateRsp>() {
> +            return Err(EINVAL);
> +        }
> +
> +        let ptr = raw.as_mut_ptr();
> +        // CAST: `GetCertificateRsp` only contains integers and has `repr(C)`.
> +        let ptr = ptr.cast::<GetCertificateRsp>();
> +        // SAFETY: `ptr` came from a reference and the cast above is valid.
> +        let rsp: &mut GetCertificateRsp = unsafe { &mut *ptr };
> +
> +        Ok(rsp)
> +    }
> +}

Does returning Self here decouple the returned reference's lifetime from the
unvalidated parameter's lifetime?

In get_cert_exchange(), could this allow the returned response reference
to outlive the borrow of the underlying vector, potentially causing
overlapping mutable and immutable references to the same memory allocation?

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