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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Integer overflow on the `offset` accumulator allows a malicious SPDM responder to force an infinite memory allocation loop or kernel panic.
- [High] The `Validate` implementation for `&GetCertificateRsp` and `get_cert_exchange` use elided lifetimes, creating a memory safety soundness hole.
- [High] The `request.length` field is cast to `usize` for buffer resizing after being converted to little-endian, severely truncating responses on big-endian architectures.

Pre-existing issues:
- [High] Stack-allocated request buffers are passed to a generic transport callback, violating kernel stack DMA rules.
--

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

lib: rspdm: Support SPDM get_certificate

This commit adds support for the GET_CERTIFICATE SPDM command. The kernel
sends requests to the responder and iterates over the returned certificates,
with validation planned for a future patch.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> @@ -686,4 +699,136 @@ pub(crate) fn get_digests(&mut self) -> Result<(), Error> {
[ ... ]
> +    fn get_cert_exchange(
> +        &mut self,
> +        request_buf: &mut [u8],
> +        response_vec: &mut KVec<u8>,
> +    ) -> Result<&GetCertificateRsp, Error> {

[Severity: High]
Does the elided lifetime here tie the returned reference to self rather than
the response_vec buffer?

Standard Rust method lifetime elision binds this detached lifetime to the
mutable self reference. This completely disables borrow checking for the
underlying response_vec, allowing safe callers to resize, mutate, or drop
the heap allocation while retaining a pointer to it, which could trigger a
use-after-free.

> +        let len = self.spdm_exchange(request_buf, response_vec.as_mut_slice())? as usize;
[ ... ]
> +    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).to_le();
> +
> +        // 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) };

[Severity: High]
This is a pre-existing issue, but does passing stack memory to a generic
transport callback violate kernel stack DMA rules?

The GetCertificateReq structure is allocated on the kernel stack and a
mutable slice pointing to this memory is passed to spdm_exchange(). Using
stack memory for hardware I/O is problematic because stacks may be virtually
mapped and lack physical contiguity or proper cacheline alignment.
Attempting to map this stack memory for DMA will cause mapping errors or
memory corruption.

> +
> +        let mut response_vec: KVec<u8> = KVec::from_elem(0u8, rsp_sz, GFP_KERNEL)?;
> +
> +        let response = self.get_cert_exchange(request_buf, &mut response_vec)?;
[ ... ]
> +        let mut offset: u16 = u16::from_le(response.portion_length);
> +        let mut remainder_length = u16::from_le(response.remainder_length) as usize;
> +
> +        while remainder_length > 0 {
> +            request.offset = offset.to_le();
> +            request.length =
> +                ((remainder_length.min(rsp_sz - core::mem::size_of::<GetCertificateRsp>())) as u16)
> +                    .to_le();
> +
> +            let request_buf =
> +                unsafe { from_raw_parts_mut(&mut request as *mut _ as *mut u8, req_sz) };
> +
> +            response_vec.resize(
> +                request.length as usize + core::mem::size_of::<GetCertificateRsp>(),

[Severity: High]
Will this resize operation truncate the response buffer on big-endian
architectures?

The request.length field is converted to little-endian using .to_le(), but
then immediately cast to usize to calculate the new size for resize().
On big-endian architectures, casting this byte-swapped value back to a
native usize yields a drastically incorrect size. This will truncate the
incoming response to the undersized buffer and reliably fail certificate
validation.

> +                0,
> +                GFP_KERNEL,
> +            )?;
> +
> +            let response = self.get_cert_exchange(request_buf, &mut response_vec)?;
[ ... ]
> +            certs_buf.extend_from_slice(
> +                &response_vec[8..(8 + u16::from_le(response.portion_length) as usize)],
> +                GFP_KERNEL,
> +            )?;
> +            offset += u16::from_le(response.portion_length);

[Severity: High]
Could this offset calculation overflow if a responder provides crafted
portion_length and remainder_length values?

The offset variable is explicitly typed as a u16. If an untrusted SPDM
responder manipulates the response lengths, offset += portion_length can
overflow. On systems with overflow checks enabled, this immediately triggers
a panic. Without overflow checks, offset wraps around, allowing the loop:

while remainder_length > 0 {
    ...
}

to run infinitely, continuously appending untrusted data to certs_buf until
all kernel memory is exhausted.

> +            remainder_length = u16::from_le(response.remainder_length) as usize;
> +        }

> diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> --- a/lib/rspdm/validator.rs
> +++ b/lib/rspdm/validator.rs
[ ... ]
> @@ -426,3 +427,58 @@ fn validate(unvalidated: &[u8]) -> Result<Self, Self::Err> {
[ ... ]
> +impl Validate<Untrusted<&[u8]>> for &GetCertificateRsp {

[Severity: High]
Does this Validate implementation fail to link the input slice's lifetime
to the output reference's lifetime?

Because explicit lifetimes are missing, the output reference is detached
from the input slice. Inside the validate() function, an unsafe block
dereferences a pointer into this unconstrained reference, breaking Rust's
safety guarantees.

> +    type Err = Error;
> +
> +    fn validate(unvalidated: &[u8]) -> Result<Self, Self::Err> {
> +        if unvalidated.len() < mem::size_of::<GetCertificateRsp>() {
> +            return Err(EINVAL);
> +        }
> +
> +        let ptr = unvalidated.as_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: &GetCertificateRsp = unsafe { &*ptr };
> +
> +        Ok(rsp)
> +    }
> +}

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