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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] A truncated SPDM capability response causes a kernel panic due to an out-of-bounds slice access.
- [High] Incorrect zero-padding placement in the SPDM 1.2+ combined prefix causes signature verification failure.
- [High] Incorrect memory layout passed to allocator when dropping `KVec` constructed via `from_raw_parts`.
--

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

lib: rspdm: Support SPDM challenge

Support the CHALLENGE SPDM command.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index e381cf3f75f9e..7341d9d8a9310 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> @@ -463,12 +498,15 @@ pub(crate) fn get_capabilities(&mut self) -> Result<(), Error> {
>          let rc = self.spdm_exchange(request_buf, response_vec.as_mut_slice())? as usize;
>  
>          // The transport must report a length within the buffer we provided.
> -        if rc > response_vec.len() {
> +        if rc > response_vec.len() || rc > rsp_sz {
>              pr_err!("Overflowed capabilities response\n");
>              return Err(EIO);
>          }
>          response_vec.truncate(rc);
>  
> +        self.transcript
> +            .extend_from_slice(&response_vec[..rsp_sz], GFP_KERNEL)?;

[Severity: High]
If spdm_exchange() returns a smaller rc than rsp_sz, response_vec will be
truncated to rc elements. Will the slice &response_vec[..rsp_sz] then panic
due to being out of bounds?

[ ... ]
> @@ -971,4 +1028,175 @@ pub(crate) fn validate_cert_chain(&mut self, slot: u8) -> Result<(), Error> {
[ ... ]
> +            let major = self.version >> 4;
> +            let minor = self.version & 0xF;
> +
> +            let output = CString::try_from_fmt(fmt!("dmtf-spdm-v{major:x}.{minor:x}.*dmtf-spdm-v{major:x}.{minor:x}.*dmtf-spdm-v{major:x}.{minor:x}.*dmtf-spdm-v{major:x}.{minor:x}.*"))?;
> +            let mut buf = output.into_vec();
> +            let zero_pad_len = SPDM_COMBINED_PREFIX_SZ - SPDM_PREFIX_SZ - SPDM_CONTEXT.len() - 1;
> +
> +            buf.extend_with(zero_pad_len, 0, GFP_KERNEL)?;
> +            buf.extend_from_slice(SPDM_CONTEXT.as_bytes(), GFP_KERNEL)?;

[Severity: High]
Does this code append the zero padding before the context string?

According to the SPDM 1.2+ specification (DSP0274), the combined prefix
format should be VersionPrefix || 0x00 || ContextString || ZeroPadding.
Could placing the zero padding before SPDM_CONTEXT cause the transcript hash
to mismatch the signature provided by the device?

[Severity: High]
Additionally, there appears to be a memory layout mismatch regression when
dropping the shash_desc context in SpdmState::drop() and
update_response_algs():

lib/rspdm/state.rs:SpdmState::drop() {
    let desc_vec = unsafe { KVec::from_raw_parts(desc, desc_len, desc_len) };
    drop(desc_vec);
}

Is the generic parameter T incorrectly inferred as bindings::shash_desc
instead of u8? Since desc_len is a byte count, would this pass the wrong
layout to the allocator during drop, assuming desc_len elements of type
shash_desc instead of desc_len bytes?

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