Re: [PATCH v6 23/25] lib: rspdm: Support SPDM certificate validation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] rspdm: premature take_public_key() before validate in
  validate_cert_chain()
- [Medium] rspdm: missing leaf key algorithm and size check in
  validate_cert_chain()
--

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

lib: rspdm: Support SPDM certificate validation

This commit adds certificate chain digest verification, X.509 chain
signature validation, and leaf public key extraction to the Rust SPDM
implementation.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index f1a55da1f1cfa..dfe2f8becfb74 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs

[ ... ]

> @@ -593,9 +604,164 @@ pub(crate) fn get_certificate(&mut self, slot: u8) -> Result<(), Error> {

[ ... ]

> +        let Some(mut leaf) = prev_cert.take() else {
> +            return Err(EPROTO);
> +        };
> +
> +        let flags = leaf.key_ext_flags()?;
> +        if flags.is_ca() || !flags.digital_signature() {
> +            dev_err!(&self.dev, "Invalid leaf certificate constraints\n");
> +            return Err(EKEYREJECTED);
> +        }

[Severity: Medium]
Should validate_cert_chain() verify that leaf.pkey_algo() and
leaf.pkey_len() match the session's negotiated self.base_asym_alg?

Without checking the leaf key's algorithm and size against
self.base_asym_alg, a responder that negotiated a strong algorithm in
NEGOTIATE_ALGORITHMS (for example ECDSA P-384 or RSA-4096) can supply a
leaf certificate with a different or weaker key accepted by the X.509
parser, and validate_cert_chain() will accept the chain and store the
mismatched key in self.leaf_key.

> +
> +        // The leaf key is the same for all slots (MULT_KEY_CAP == 0),
> +        // so just store the first one.
> +        if self.leaf_key.is_none() {
> +            self.leaf_key = leaf.take_public_key();
> +        }
> +
> +        if let Some(validate) = self.validate {
> +            // SAFETY: Call the `validate` function provided with a valid
> +            // certificate pointer, which the callback must not modify or
> +            // free.
> +            let rc = unsafe { validate(self.dev.as_raw(), slot, leaf.as_raw()) };
> +            to_result(rc)?;
> +        }
> +
> +        Ok(())
> +    }
>  }

[Severity: Medium]
Does calling leaf.take_public_key() before invoking self.validate pass a
struct x509_certificate with a NULL pub pointer to the callback on the
first slot?

Certificate::take_public_key() transfers ownership of the key by clearing
cert->pub to NULL in place:

rust/kernel/x509.rs:Certificate::take_public_key() {
    ...
        unsafe {
            (*self.cert.as_ptr()).pub_ = ptr::null_mut();
        }
    ...
}

As a result, on the first validated slot a validate callback that inspects
leaf_cert->pub will dereference a NULL pointer, and if the callback
rejects the certificate, self.leaf_key remains populated with the rejected
certificate's key.  Moving the self.leaf_key assignment after the
self.validate check avoids both issues.

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