Re: [PATCH 18/18] lib: rspdm: Support SPDM challenge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Silent security bypass in `spdm_authenticate` when authentication fails.
- [High] Out-of-bounds slice access in `get_version`, `get_digests`, and `get_cert_exchange` due to unvalidated `rsp_sz`.
- [High] Out-of-bounds slice access for `opaque_len` in `challenge()` due to insufficient length validation.
- [High] Undefined Behavior and use of uninitialized memory for `response_buf` in `challenge()`.
- [High] Slice boundary panic when computing `sig_start` in `challenge()`.
- [High] `copy_from_slice` panics if `next_nonce` is not exactly 32 bytes.
- [Medium] Overwriting `leaf_key` during cert validation loop causes valid challenges to fail.
- [Medium] Logic error when no certificates are provisioned causing invalid SPDM command.
--

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

lib: rspdm: Support SPDM challenge

This commit adds support for the CHALLENGE SPDM command in the rspdm
library. It updates the state machine to perform challenges, verify
signatures, and record transcripts.

> diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs
> index d6421b2fab7df..7fcf5a2d3071a 100644
> --- a/lib/rspdm/lib.rs
> +++ b/lib/rspdm/lib.rs
> @@ -135,17 +135,23 @@
>          provisioned_slots &= !(1 << slot);
>      }
>  
> +    let mut verify = true;
>      let mut provisioned_slots = state.provisioned_slots;
>      while (provisioned_slots as usize) > 0 {
>          let slot = provisioned_slots.trailing_zeros() as u8;
>  
>          if let Err(e) = state.validate_cert_chain(slot) {
> -            return e.to_errno() as c_int;
> +            pr_err!("Certificate in slot {slot} failed to verify: {e:?}");
> +            verify = false;
>          }
>  
>          provisioned_slots &= !(1 << slot);
>      }

By continuing the loop and setting verify to false, could this allow a
device with an invalid certificate to bypass authentication? Later in
challenge(), if verify is false, signature verification is skipped and it
returns Ok(()), causing spdm_authenticate() to return 0 (success).

When multiple slots are provisioned, this loop calls validate_cert_chain()
for each slot sequentially, which updates state.leaf_key. Could this cause
authentication to fail? The challenge is issued for the lowest slot (using
trailing_zeros() below), but state.leaf_key will contain the public key of
the highest provisioned slot.

> +    if let Err(e) = state.challenge(state.provisioned_slots.trailing_zeros() as u8, verify) {
> +        return e.to_errno() as c_int;
> +    }
> +
>      0
>  }

What happens if state.provisioned_slots is 0? The trailing_zeros() call
would return 32, which is an invalid slot number for SPDM, potentially
sending a malformed request to the device.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index 4c8ee553bb699..3cf7236af7b24 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> @@ -367,6 +395,12 @@ pub(crate) fn get_version(&mut self) -> Result<(), Error> {
>          unsafe { response_vec.inc_len(rc as usize) };
>  
>          let response: &mut GetVersionRsp = Untrusted::new_mut(&mut response_vec).validate_mut()?;
> +        let rsp_sz = core::mem::size_of::<SpdmHeader>()
> +            + 2
> +            + response.version_number_entry_count as usize * 2;
> +
> +        self.transcript
> +            .extend_from_slice(&response_vec[..rsp_sz], GFP_KERNEL)?;

Can this cause an out-of-bounds slice access? We calculate rsp_sz based on
the attacker-controlled entry count, but we don't appear to verify that the
actually received length (rc) is at least rsp_sz before creating the slice.

[ ... ]
> @@ -637,6 +677,10 @@ pub(crate) fn get_digests(&mut self) -> Result<(), Error> {
>          unsafe { response_vec.inc_len(len as usize) };
>  
>          let response: &mut GetDigestsRsp = Untrusted::new_mut(&mut response_vec).validate_mut()?;
> +        let rsp_sz = core::mem::size_of::<SpdmHeader>() + response.param2 as usize * self.hash_len;
> +
> +        self.transcript
> +            .extend_from_slice(&response_vec[..rsp_sz], GFP_KERNEL)?;

Similar to get_version(), is it possible for response.param2 to cause an
out-of-bounds read if the device returns a short packet?

[ ... ]
> @@ -697,6 +741,10 @@ fn get_cert_exchange(
>          unsafe { response_vec.inc_len(rc as usize) };
>  
>          let response: &mut GetCertificateRsp = Untrusted::new_mut(response_vec).validate_mut()?;
> +        let rsp_sz = core::mem::size_of::<SpdmHeader>() + 4 + response.portion_length as usize;
> +
> +        self.transcript
> +            .extend_from_slice(&response_vec[..rsp_sz], GFP_KERNEL)?;

Could an untrusted portion_length lead to an out-of-bounds slice access
here if the actual received length is smaller than rsp_sz?

[ ... ]
> +    pub(crate) fn challenge(&mut self, slot: u8, verify: bool) -> Result<(), Error> {
> +        let mut request = ChallengeReq::default();
> +        request.version = self.version;
> +        request.param1 = slot;
> +
> +        let nonce_len = request.nonce.len();
> +
> +        if self.next_nonce.len() > 0 {
> +            request.nonce.copy_from_slice(&self.next_nonce);

Can this panic if next_nonce is not exactly 32 bytes? Since next_nonce is
populated from user space sysfs and request.nonce is a fixed 32-byte array,
an incorrect length might cause copy_from_slice() to panic.

> +            self.next_nonce.clear();
> +        } else {
> +            unsafe {
> +                bindings::get_random_bytes(&mut request.nonce as *mut _ as *mut c_void, nonce_len)
> +            };
> +        }
> +
> +        let req_sz = if self.version <= 0x12 {
> +            offset_of!(ChallengeReq, context)
> +        } else {
> +            core::mem::size_of::<ChallengeReq>()
> +        };
> +
> +        let rsp_sz = self.challenge_rsp_len(nonce_len, SPDM_MAX_OPAQUE_DATA);
> +
> +        // 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::with_capacity(rsp_sz, GFP_KERNEL)?;
> +        // SAFETY: `response_vec` is rsp_sz length, initialised, aligned
> +        // and won't be mutated
> +        let response_buf = unsafe { from_raw_parts_mut(response_vec.as_mut_ptr(), rsp_sz) };

Does KVec::with_capacity() initialize the memory? If it only allocates
capacity without initializing, creating a mutable slice over this memory
could be undefined behavior. Also, if spdm_exchange() validates the buffer
before passing it to the transport, wouldn't it be reading uninitialized
memory?

> +        let rc = self.spdm_exchange(request_buf, response_buf)?;
> +
> +        if rc < (core::mem::size_of::<ChallengeRsp>() as i32) {
> +            pr_err!("Truncated challenge response\n");
> +            to_result(-(bindings::EIO as i32))?;
> +        }
> +
> +        // SAFETY: `rc` is the length of data read, which will be smaller
> +        // then the capacity of the vector
> +        unsafe { response_vec.inc_len(rc as usize) };
> +
> +        let _response: &mut ChallengeRsp = Untrusted::new_mut(&mut response_vec).validate_mut()?;
> +
> +        // MSHLength is 0 as no measurement summary hash requested
> +        let opaque_len_offset = core::mem::size_of::<SpdmHeader>() + self.hash_len + nonce_len;
> +        let opaque_len = u16::from_le_bytes(
> +            response_vec[opaque_len_offset..(opaque_len_offset + 2)]
> +                .try_into()
> +                .unwrap_or([0, 0]),
> +        );

Does the initial truncation check guarantee that response_vec is large enough
to read opaque_len? The check only verifies it is at least the size of
ChallengeRsp (which might only be 46 bytes), but opaque_len_offset includes
hash_len and nonce_len (which could be 68 bytes or more). Could a device
send a response that passes the truncation check but causes a panic when
slicing opaque_len_offset..(opaque_len_offset + 2)?

> +        let rsp_sz = self.challenge_rsp_len(nonce_len, opaque_len as usize);
> +
> +        if rc < rsp_sz as i32 {
> +            pr_err!("Truncated challenge response\n");
> +            to_result(-(bindings::EIO as i32))?;
> +        }
> +
> +        self.transcript
> +            .extend_from_slice(&response_vec[..rsp_sz], GFP_KERNEL)?;
> +
> +        if verify {
> +            /* Verify signature at end of transcript against leaf key */
> +            let sig_start = response_vec.len() - self.sig_len;
> +            let signature = &mut response_vec[sig_start..rsp_sz];

If the device sends more data than expected (where rc is greater than
rsp_sz), could sig_start become larger than rsp_sz? If response_vec.len()
was set to rc earlier via inc_len(), then sig_start could shift forward,
causing a panic when attempting to create the signature slice since the
start index would exceed the end index.

> +            match self.verify_signature(signature) {
> +                Ok(()) => {
> +                    pr_info!("Authenticated with certificate slot {slot}");
> +                    self.authenticated = true;
> +                }
> +                Err(e) => {
> +                    pr_err!("Cannot verify challenge_auth signature: {e:?}");
> +                    self.authenticated = false;
> +                }
> +            };
> +        }
> +
> +        Ok(())
> +    }
>  }

If verify_signature() fails and we take the Err path, setting authenticated
to false, the function still returns Ok(()). Should this return an error
instead? As currently written, it appears spdm_authenticate() will return
success to the C caller even when a device provides an invalid signature.

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