Re: [PATCH v6 17/25] lib: rspdm: Support SPDM negotiate_algorithms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] lib: rspdm: build failure in update_response_algs() without
  crypto Kconfigs
- [Low] lib: rspdm: unreachable extended algorithm loops in
  NegotiateAlgsRsp::validate()
--

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

lib: rspdm: Support SPDM negotiate_algorithms

This commit adds support for the SPDM NEGOTIATE_ALGORITHMS command,
including request construction, response validation, and algorithm state
selection.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index cb6a351fe6085..169d0d3fee9bd 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> @@ -265,4 +323,113 @@ pub(crate) fn get_capabilities(&mut self) -> Result<(), Error> {
>  
>          Ok(())
>      }
> +
> +    fn update_response_algs(&mut self) -> Result<(), Error> {
> +        match self.base_asym_alg {
> +            #[cfg(CONFIG_CRYPTO_RSA)]
> +            SPDM_ASYM_RSASSA_2048 => {
> +                self.sig_len = 256;
> +                self.base_asym_enc = c"pkcs1";
> +            }
> +            #[cfg(CONFIG_CRYPTO_RSA)]
> +            SPDM_ASYM_RSASSA_3072 => {
> +                self.sig_len = 384;
> +                self.base_asym_enc = c"pkcs1";
> +            }
> +            #[cfg(CONFIG_CRYPTO_RSA)]
> +            SPDM_ASYM_RSASSA_4096 => {
> +                self.sig_len = 512;
> +                self.base_asym_enc = c"pkcs1";
> +            }
> +            #[cfg(CONFIG_CRYPTO_ECDSA)]
> +            SPDM_ASYM_ECDSA_ECC_NIST_P256 => {
> +                self.sig_len = 64;
> +                self.base_asym_enc = c"p1363";
> +            }
> +            #[cfg(CONFIG_CRYPTO_ECDSA)]
> +            SPDM_ASYM_ECDSA_ECC_NIST_P384 => {
> +                self.sig_len = 96;
> +                self.base_asym_enc = c"p1363";
> +            }
> +            #[cfg(CONFIG_CRYPTO_ECDSA)]
> +            SPDM_ASYM_ECDSA_ECC_NIST_P521 => {
> +                self.sig_len = 132;
> +                self.base_asym_enc = c"p1363";
> +            }
> +            _ => {
> +                dev_err!(&self.dev, "Unknown asym algorithm\n");
> +                return Err(EPROTO);
> +            }
> +        }
> +
> +        let hash_name = match self.base_hash_alg {
> +            #[cfg(CONFIG_CRYPTO_SHA256)]
> +            SPDM_HASH_SHA_256 => c"sha256",
> +            #[cfg(CONFIG_CRYPTO_SHA512)]
> +            SPDM_HASH_SHA_384 => c"sha384",
> +            #[cfg(CONFIG_CRYPTO_SHA512)]
> +            SPDM_HASH_SHA_512 => c"sha512",
> +            _ => {
> +                dev_err!(&self.dev, "Unknown hash algorithm\n");
> +                return Err(EPROTO);
> +            }
> +        };
> +
> +        self.crypto = Some(Shash::new(hash_name)?);
> +
> +        Ok(())
> +    }
> +
> +    pub(crate) fn negotiate_algs(&mut self) -> Result<(), Error> {
[ ... ]
> +        self.base_asym_alg = response.base_asym_sel;
> +        self.base_hash_alg = response.base_hash_sel;
> +        self.meas_hash_alg = response.measurement_hash_algo;
> +
> +        if self.base_asym_alg & SPDM_ASYM_ALGOS == 0 || self.base_hash_alg & SPDM_HASH_ALGOS == 0 {
> +            dev_err!(&self.dev, "No common supported algorithms\n");
> +            return Err(EPROTO);
> +        }

[Severity: Medium]
Does this fail to build when CONFIG_RSPDM=y is enabled without any asymmetric
crypto option (CONFIG_CRYPTO_RSA and CONFIG_CRYPTO_ECDSA both disabled) or
without any hash option (CONFIG_CRYPTO_SHA256 and CONFIG_CRYPTO_SHA512 both
disabled)?

When both options in either pair are disabled, the corresponding match in
update_response_algs() collapses to the diverging wildcard arm alone, which
triggers rustc unreachable_code and unused_variables errors as well as
clippy::match_single_binding.  In addition, SPDM_ASYM_ALGOS or
SPDM_HASH_ALGOS evaluates to 0, so the bitmask check in negotiate_algs()
triggers clippy::bad_bit_mask.

> diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
> index 66212f0faf5f1..335f2f43dc07c 100644
> --- a/lib/rspdm/validator.rs
> +++ b/lib/rspdm/validator.rs
[ ... ]
> +        // We currently never request extended algorithms or algorithm structures
> +        if header.param1 != 0 || ext_asym_sel_count != 0 || ext_hash_sel_count != 0 {
> +            dev_err!(
> +                &context.dev,
> +                "Responder selected unsupported extended algorithms\n"
> +            );
> +            return Err(EPROTO);
> +        }
> +
> +        let mut offset = NEGOTIATE_ALGS_RSP_SZ;
> +
> +        let mut ext_asym = KVec::new();
> +        for _ in 0..ext_asym_sel_count {
> +            ext_asym.push(read_le32(offset)?, GFP_KERNEL)?;
> +            offset += mem::size_of::<u32>();
> +        }
> +
> +        let mut ext_hash = KVec::new();
> +        for _ in 0..ext_hash_sel_count {
> +            ext_hash.push(read_le32(offset)?, GFP_KERNEL)?;
> +            offset += mem::size_of::<u32>();
> +        }
> +
> +        let mut resp_alg_struct = KVec::new();
> +        for _ in 0..header.param1 {
> +            let alg_type = *unvalidated.get(offset).ok_or(EPROTO)?;
> +            let alg_count = *unvalidated.get(offset + 1).ok_or(EPROTO)?;
> +            let fixed_alg_count = (alg_count >> 4) as usize;
> +            let ext_alg_count = (alg_count & 0xf) as usize;
> +            offset += 2;
> +
> +            let mut alg_supported = KVec::new();
> +            alg_supported.extend_from_slice(
> +                unvalidated
> +                    .get(offset..offset + fixed_alg_count)
> +                    .ok_or(EPROTO)?,
> +                GFP_KERNEL,
> +            )?;
> +            offset += fixed_alg_count;
> +
> +            let mut alg_external = KVec::new();
> +            for _ in 0..ext_alg_count {
> +                alg_external.push(read_le32(offset)?, GFP_KERNEL)?;
> +                offset += mem::size_of::<u32>();
> +            }
> +
> +            resp_alg_struct.push(
> +                RespAlgStruct {
> +                    alg_type,
> +                    alg_count,
> +                    alg_supported,
> +                    alg_external,
> +                },
> +                GFP_KERNEL,
> +            )?;
> +        }

[Severity: Low]
This isn't a bug, but since NegotiateAlgsRsp::validate() returns Err(EPROTO)
above whenever header.param1, ext_asym_sel_count, or ext_hash_sel_count is
non-zero, the three loops that follow can never execute (and duplicate the
parsing in SupportedAlgorithms::from_bytes()).

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