Re: [PATCH v2 17/21] lib: rspdm: Support SPDM get_digests

[sashiko-bot@kernel.org]

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

New issues:
- [High] Buffer allocation for `GetDigestsRsp` does not account for SPDM 1.3 additional fields, causing legitimate responses to exceed the buffer size.
- [High] The returned digest payload is ignored, failing to invalidate stale cached certificates if a device updates its certificate.

Pre-existing issues:
- [High] Rebuilding `KVec` from a raw pointer infers the wrong generic type parameter, causing the global allocator to deallocate a massively oversized layout.
--

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

lib: rspdm: Support SPDM get_digests

Support the GET_DIGESTS SPDM command.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index e4eb009a977c3..055f37289c8b5 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
> @@ -608,4 +626,64 @@ pub(crate) fn negotiate_algs(&mut self) -> Result<(), Error> {
>  
>          Ok(())
>      }
> +
> +    pub(crate) fn get_digests(&mut self) -> Result<(), Error> {
> +        let mut request = GetDigestsReq::default();
> +        request.version = self.version;
> +
> +        let req_sz = core::mem::size_of::<GetDigestsReq>();
> +        let rsp_sz = core::mem::size_of::<GetDigestsRsp>() + SPDM_SLOTS * self.hash_len;
> +
> +        // 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::from_elem(0u8, rsp_sz, GFP_KERNEL)?;

[Severity: High]
Does this correctly calculate the required buffer size for an SPDM 1.3
responder?

If the responder negotiates SPDM 1.3+, the GET_DIGESTS response appends extra
fields (like KeyPairIDs, CertificateInfo, or KeyUsageMask) after the digests.
Because rsp_sz strictly allocates only enough for the headers and the hash
lengths, the transport may truncate the response or fail the bounds check
when it returns a length larger than response_vec.len().

> +
> +        let len = self.spdm_exchange(request_buf, response_vec.as_mut_slice())? as usize;
> +
> +        // The transport must report a length within the buffer we provided.
> +        if len > response_vec.len() {
> +            pr_err!("Overflowed digests response\n");
> +            return Err(EIO);
> +        }

[ ... ]

> +        let mut deprovisioned_slots = self.provisioned_slots & !response.param2;
> +        while (deprovisioned_slots.trailing_zeros() as usize) < SPDM_SLOTS {
> +            let slot = deprovisioned_slots.trailing_zeros() as usize;
> +            self.certs[slot].clear();
> +            deprovisioned_slots &= !(1 << slot);
> +        }

[Severity: High]
Does this logic correctly invalidate stale cached certificates if a device
updates its certificate in an existing provisioned slot?

While this clears completely deprovisioned slots, it appears to ignore the
returned digest payload itself. If the responder rotated its certificate in
an existing slot, the digest would change. By not comparing the returned
digests against the cached certificates and clearing mismatches, later
validation of CHALLENGE_AUTH could fail cryptographically because it
verifies against the stale cached certificate.

[ ... ]

>             let desc_len = core::mem::size_of::<bindings::shash_desc>()
>                 + unsafe { bindings::crypto_shash_descsize(self.shash) } as usize;
> 
>             // SAFETY: `desc` was allocated and converted to a raw pointer with
>             // into_raw_parts()
>             let desc_vec = unsafe { KVec::from_raw_parts(desc, desc_len, desc_len) };
>             drop(desc_vec);

[Severity: High]
This isn't a bug introduced by this patch, but can this type inference
deallocate a massively oversized layout?

When rebuilding KVec from the raw pointer desc in SpdmState::drop() and
update_response_algs(), the generic type T is implicitly inferred as
bindings::shash_desc instead of u8.

Since desc_len represents the original allocation length in bytes, the
reconstructed KVec<bindings::shash_desc> incorrectly assumes it owns desc_len
elements of type bindings::shash_desc. When this vector is dropped, it
passes a layout to the global allocator with an extremely large size of
(desc_len * size_of::<bindings::shash_desc>()) bytes instead of desc_len
bytes.

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