Re: [PATCH v5 23/23] lib: rspdm: Support SPDM challenge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] lib: rspdm: unsliced request_buf in spdm_exchange() fails validation
--

commit b875413583bbf6bfc09ae38ff772d19acd98e3d0
Author: Alistair Francis <alistair.francis@wdc.com>

lib: rspdm: Support SPDM challenge

This commit adds support for the SPDM CHALLENGE command, including message
transcript tracking, signature verification against the responder leaf
certificate, and challenge response validation.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index aa36d4ce7b41a..993d19cd60935 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> @@ -197,12 +217,14 @@ pub(crate) fn hash_len(&self) -> usize {
>      /// The data in `request_buf` is sent to the device and the response
>      /// is returned as untrusted data to be validated by the caller.
>      pub(crate) fn spdm_exchange(
> -        &self,
> +        &mut self,
>          request_buf: &[u8],
>          mut response_buf: KVec<u8>,
>      ) -> Result<Untrusted<KVec<u8>>, Error> {
>          let header_size = core::mem::size_of::<SpdmHeader>();
> -        let request: SpdmHeader = Untrusted::new(request_buf).validate(self)?;
> +        let request: SpdmHeader = Untrusted::new(request_buf).validate(&*self)?;

[Severity: High]
Does validating the full request_buf as an SpdmHeader cause SpdmState::challenge()
(and other requests with payloads beyond the 4-byte header) to fail with EPROTO?

In lib/rspdm/state.rs, spdm_authenticate() calls SpdmState::challenge(), which
serializes a 36-byte (or 44-byte for SPDM 1.3+) ChallengeReq via
ChallengeReq::to_bytes() and passes the buffer to SpdmState::spdm_exchange():

lib/rspdm/state.rs:SpdmState::challenge() {
    ...
    let mut request_buf = request.to_bytes()?;
    ...
    let mut response: ChallengeRsp = self
        .spdm_exchange(request_buf.as_mut_slice(), response_vec)?
        .validate(&*self)?;
}

In SpdmState::spdm_exchange(), request_buf is passed unsliced to
Untrusted::new(request_buf).validate(&*self), which invokes
SpdmHeader::validate() in lib/rspdm/validator.rs:

lib/rspdm/validator.rs:SpdmHeader::validate() {
    SpdmHeader::read_from_bytes(unvalidated).or(Err(EPROTO))
}

Because zerocopy::FromBytes::read_from_bytes() requires the input slice length
to exactly equal size_of::<SpdmHeader>() (4 bytes) via Ref::sized_from() in
rust/zerocopy/src/ref.rs, any request buffer larger than 4 bytes is rejected
with EPROTO before the request can be added to the transcript or sent over the
transport.

Should request_buf be sliced to ..header_size before validating the request
header, matching how response_buf.get(..header_size) is validated later in
SpdmState::spdm_exchange()?

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