Re: [PATCH v4 16/22] lib: rspdm: Support SPDM get_capabilities

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] rspdm: spec violation and connection failure parsing SPDM 1.3 capabilities on SPDM 1.2
- [Low] rspdm: unreachable data_transfer_size boundary error path
--

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

lib: rspdm: Support SPDM get_capabilities

This commit adds support for the GET_CAPABILITIES SPDM command.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index 3bbe55d965f7a..fb43260f48004 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> @@ -290,4 +301,59 @@ pub(crate) fn get_version(&mut self) -> Result<(), Error> {
[ ... ]
> +    pub(crate) fn get_capabilities(&mut self) -> Result<(), Error> {
[ ... ]
> +        let response: GetCapabilitiesRsp =
> +            Untrusted::new(response_vec.as_slice()).validate(&*self)?;
> +
> +        self.rsp_caps = response.flags;
> +        if (self.rsp_caps & SPDM_RSP_MIN_CAPS) != SPDM_RSP_MIN_CAPS {
> +            pr_err!(
> +                "{:#x} capabilities are supported, which don't meet required {:#x}\n",
> +                self.rsp_caps,
> +                SPDM_RSP_MIN_CAPS
> +            );
> +            self.rsp_caps = 0;
> +            return Err(EPROTONOSUPPORT);
> +        }
> +
> +        if self.version >= SPDM_VER_12 {
> +            if response.data_transfer_size < 42 {
> +                pr_err!(
> +                    "Invalid minimum transport size {}, must be at least 42\n",
> +                    response.data_transfer_size
> +                );
> +                return Err(EPROTONOSUPPORT);
> +            }

[Severity: Low]
Is this error path reachable?

It looks like the earlier validation in GetCapabilitiesRsp::validate already
enforces data_transfer_size >= SPDM_MIN_DATA_TRANSFER_SIZE (which is 42)
and returns EPROTO. 

This seems to make the EPROTONOSUPPORT path in SpdmState::get_capabilities
dead code.

> +
> +            self.transport_sz = self.transport_sz.min(response.data_transfer_size);
> +        }
> +
> +        Ok(())
> +    }
>  }
> diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> index d0b6691365c0a..ca837bd9cfcc7 100644
> --- a/lib/rspdm/validator.rs
> +++ b/lib/rspdm/validator.rs
[ ... ]
> @@ -175,3 +183,302 @@ fn validate(unvalidated: &[u8], context: &'c SpdmState) -> Result<Self, Self::Er
[ ... ]
> +impl<'a, 'c> Validate<'c, Untrusted<&'a [u8]>> for GetCapabilitiesRsp {
> +    type Err = Error;
> +
> +    type Context = &'c SpdmState;
> +
> +    fn validate(unvalidated: &[u8], context: &'c SpdmState) -> Result<Self, Self::Err> {
[ ... ]
> +        let (data_transfer_size, max_spdm_msg_size, supported_algorithms, length) =
> +            if context.version <= SPDM_VER_11 {
[ ... ]
> +            } else {
[ ... ]
> +                let supported_algorithms = if header.param1 & SPDM_CAP_SUPPORTED_ALGORITHMS != 0 {
> +                    Some(SupportedAlgorithms::from_bytes(
> +                        unvalidated.get(GET_CAPABILITIES_RSP_SZ..).ok_or(EIO)?,
> +                    )?)
> +                } else {
> +                    None
> +                };

[Severity: Medium]
Can this logic incorrectly parse an SPDM 1.2 payload?

If an SPDM 1.2 responder has bit 0 of Param1 set (either by mistake or due to
uninitialized reserved fields), wouldn't this lack of a
context.version >= SPDM_VER_13 guard cause the kernel to incorrectly interpret
this SPDM 1.3 field?

It seems this would attempt to parse SupportedAlgorithms from the end of the
20-byte packet, read an empty slice, and abort the handshake with EIO.

> +
> +                let length = GET_CAPABILITIES_RSP_SZ
> +                    + supported_algorithms
> +                        .as_ref()
> +                        .map_or(0, |s| s.length as usize);
> +
> +                (
> +                    data_transfer_size,
> +                    max_spdm_msg_size,
> +                    supported_algorithms,
> +                    length,
> +                )
> +            };

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