Re: [PATCH v3 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:

New issues:
- [Medium] `ChallengeRsp` struct definition misrepresents the SPDM packet format by declaring variable-sized fields in the middle of a `repr(C, packed)` struct.
- [Medium] Missing upper bound check on transport response length (`rc`) in `challenge` function.

Pre-existing issues:
- [High] Stale `leaf_key` retention across multiple SPDM authentications causes persistent authentication failures.
--

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

lib: rspdm: Support SPDM challenge
    
This commit adds support for the CHALLENGE SPDM command in the SPDM responder
implementation.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index fc7df9dd7b973..1fd691037fa57 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> +    fn verify_signature(&mut self, signature: &mut [u8]) -> Result<(), Error> {
[ ... ]
> +        if let Some(leaf_key) = self.leaf_key {
> +            unsafe { to_result(bindings::public_key_verify_signature(leaf_key, &sig)) }
> +        } else {
> +            return Err(EPROTO);
> +        }
> +    }

[Severity: High]
This is a pre-existing issue, but this code in verify_signature() relies on
the cached leaf_key which can lead to persistent authentication failures.

If a device is re-authenticated and its certificates have changed,
spdm_authenticate() will fetch the new certificates, but the existing code in
validate_cert_chain() only sets leaf_key if it is missing.

Will verify_signature() then attempt to verify the new signature using the
stale, previously cached leaf_key, resulting in an inevitable verification
failure during the challenge sequence?

[ ... ]
> +    pub(crate) fn challenge(&mut self, slot: u8) -> 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 < core::mem::size_of::<ChallengeRsp>() {
> +            pr_err!("Truncated challenge response\n");
> +            return Err(EIO);
> +        }
> +        response_vec.truncate(rc);

[Severity: Medium]
Does SpdmState::challenge() miss the upper bound check on the transport
response length?

The comment notes that the transport must report a length within the buffer
provided. If a buggy transport driver returns an rc greater than
response_vec.len(), this check will pass and response_vec.truncate(rc) will
not bound the vector correctly, allowing the function to process potentially
uninitialized or out-of-bounds data later in the parsing process.

Should this also ensure that rc is less than or equal to response_vec.len()?

> diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> index 2584bed739792..f623ac1d28606 100644
> --- a/lib/rspdm/validator.rs
> +++ b/lib/rspdm/validator.rs
[ ... ]
> +#[repr(C, packed)]
> +pub(crate) struct ChallengeRsp {
> +    pub(crate) version: u8,
> +    pub(crate) code: u8,
> +    pub(crate) param1: u8,
> +    pub(crate) param2: u8,
> +
> +    pub(crate) cert_chain_hash: __IncompleteArrayField<u8>,
> +    pub(crate) nonce: [u8; 32],

[Severity: Medium]
Does the ChallengeRsp struct definition in lib/rspdm/validator.rs misrepresent
the actual wire format of the SPDM protocol by placing a variable-sized field
before a fixed-size field?

Because __IncompleteArrayField<u8> acts as a zero-sized type, nonce and all
subsequent fields will be assigned static offsets that do not account for the
actual length of cert_chain_hash. Any future code attempting to access these
fields directly from the struct would read from the wrong offsets.

Does the struct layout need to be restructured to accurately reflect the
variable length nature of the SPDM challenge response?

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