Message-ID: <sashiko-outbox-206245@kernel.org>
From: sashiko-bot@kernel.org
Subject: Re: [PATCH v6 23/25] lib: rspdm: Support SPDM certificate
 validation
Reply-To: sashiko-reviews@lists.linux.dev
To: alistair23@gmail.com, linux-pci@vger.kernel.org, linux-cxl@vger.kernel.org
Cc: ojeda@kernel.org, gary@garyguo.net
In-Reply-To: <20261110041832.1730924-24-alistair.francis@wdc.com>
References: <20261110041832.1730924-1-alistair.francis@wdc.com>
 <20261110041832.1730924-24-alistair.francis@wdc.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Tue, 10 Nov 2026 04:51:30 +0000
Precedence: list

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) =3D prev_cert.take() else {
> +            return Err(EPROTO);
> +        };
> +
> +        let flags =3D 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 =3D=3D 0=
),
> +        // so just store the first one.
> +        if self.leaf_key.is_none() {
> +            self.leaf_key =3D leaf.take_public_key();
> +        }
> +
> +        if let Some(validate) =3D self.validate {
> +            // SAFETY: Call the `validate` function provided with a valid
> +            // certificate pointer, which the callback must not modify or
> +            // free.
> +            let rc =3D unsafe { validate(self.dev.as_raw(), slot, leaf.a=
s_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_ =3D 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.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20261110041832.1730=
924-1-alistair.francis@wdc.com?part=3D23