Re: [PATCH v4 19/22] lib: rspdm: Support SPDM get_certificate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rspdm: SPDM cert chain truncation and underflow in SpdmCertChain::validate
- [Medium] rspdm: integer underflow in get_certificate when transport_sz is small
--

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

lib: rspdm: Support SPDM get_certificate

This commit introduces support for the GET_CERTIFICATE SPDM command. It
sends requests to the responder and iterates over the returned certificates
for later validation.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index 502d99eb13a66..780b9d459abf9 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> @@ -654,4 +659,107 @@ pub(crate) fn get_digests(&mut self) -> Result<(), Error> {
[ ... ]
> +    pub(crate) fn get_certificate(&mut self, slot: u8) -> Result<(), Error> {
> +        let mut request = GetCertificateReq::default();
> +        request.header.version = self.version;
> +        request.header.param1 = slot;
> +
> +        let rsp_sz =
> +            (GET_CERTIFICATE_RSP_HDR_SZ as u32 + u16::MAX as u32).min(self.transport_sz) as usize;
> +
> +        request.offset = 0;
> +        request.length = (rsp_sz - GET_CERTIFICATE_RSP_HDR_SZ) as u16;

[Severity: Medium]
What happens if the caller provides a `transport_sz` that is less than 8?

Since `GET_CERTIFICATE_RSP_HDR_SZ` is 8, `rsp_sz` could be less than 8,
causing this subtraction to underflow `usize` and panic.

> +
> +        let mut response_vec: KVec<u8> = KVec::from_elem(0u8, rsp_sz, GFP_KERNEL)?;
> +
> +        let mut request_buf = request.to_bytes()?;
> +        let response = self.get_cert_exchange(request_buf.as_mut_slice(), &mut response_vec)?;
[ ... ]
> +        let mut offset: u16 = portion_length;
> +        let mut remainder_length = rem_length as usize;
> +
> +        while remainder_length > 0 {
> +            request.offset = offset;
> +            request.length = (remainder_length.min(rsp_sz - GET_CERTIFICATE_RSP_HDR_SZ)) as u16;

[Severity: Medium]
Could this subtraction also underflow under the same conditions where `rsp_sz`
is smaller than `GET_CERTIFICATE_RSP_HDR_SZ`?

> +
> +            let mut request_buf = request.to_bytes()?;
[ ... ]
> diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> index c058dde314401..f4e9485179d01 100644
> --- a/lib/rspdm/validator.rs
> +++ b/lib/rspdm/validator.rs
[ ... ]
> @@ -794,3 +795,154 @@ fn validate(unvalidated: &[u8], context: &'c SpdmState<'c>) -> Result<Self, Self
[ ... ]
> +impl<'a, 'c> Validate<'c, Untrusted<&'a [u8]>> for SpdmCertChain {
> +    type Err = Error;
> +
> +    type Context = &'c SpdmState<'c>;
> +
> +    fn validate(unvalidated: &[u8], context: &'c SpdmState<'c>) -> Result<Self, Self::Err> {
[ ... ]
> +        let root_hash_end = SPDM_CERT_CHAIN_HDR_SZ + context.hash_len;
> +        let mut root_hash = KVec::new();
> +        root_hash.extend_from_slice(
> +            unvalidated
> +                .get(SPDM_CERT_CHAIN_HDR_SZ..root_hash_end)
> +                .ok_or(EIO)?,
> +            GFP_KERNEL,
> +        )?;
> +
> +        let cert_chain_end = length as usize - root_hash_end;

[Severity: High]
Does this calculation safely handle payloads where `length` is smaller than
`root_hash_end`?

It appears this subtraction will underflow and trigger a panic if the untrusted
payload provides a short length.

> +        let mut certificates = KVec::new();
> +        certificates.extend_from_slice(
> +            unvalidated.get(root_hash_end..cert_chain_end).ok_or(EIO)?,

[Severity: High]
Is it intentional to use the subtracted value as the exclusive slice bound here?

Because `cert_chain_end` is calculated as a relative length but used as an
absolute upper bound, it seems to unconditionally truncate the valid certificate
stream by `root_hash_end` bytes.

> +            GFP_KERNEL,
> +        )?;
> +
> +        Ok(SpdmCertChain {
> +            length,
> +            root_hash,
> +            certificates,
> +        })
> +    }
> +}

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