Re: [PATCH 16/18] lib: rspdm: Support SPDM certificate validation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] validate_cert_chain overwrites `leaf_key` when validating multiple provisioned slots, corrupting state and breaking authentication.
- [High] validate_cert_chain leaks dynamically allocated `x509_certificate` structures on all error paths.
- [High] validate_cert_chain leaks the leaf `x509_certificate` structure upon successful validation.
--

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

    lib: rspdm: Support SPDM certificate validation

This commit adds support for validating the SPDM certificate chain. It performs
basic sanity checks to ensure the chain is correct, uses supported signatures,
and is not blacklisted in the kernel.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index 69b6f67a6ef52..4c8ee553bb699 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs

[ ... ]

> @@ -786,4 +789,68 @@ pub(crate) fn get_certificate(&mut self, slot: u8) -> Result<(), Error> {
>  
>          Ok(())
>      }
> +
> +    pub(crate) fn validate_cert_chain(&mut self, slot: u8) -> Result<(), Error> {

[ ... ]

> +            let cert_ptr = unsafe {
> +                from_err_ptr(bindings::x509_cert_parse(
> +                    &cert_chain_buf[offset..] as *const _ as *const c_void,
> +                    cert_len as usize,
> +                ))?
> +            };
> +            let cert = unsafe { *cert_ptr };
> +
> +            if cert.unsupported_sig || cert.blacklisted {
> +                pr_err!("Certificate was rejected\n");
> +                to_result(-(bindings::EKEYREJECTED as i32))?;
> +            }

Is it possible this code leaks the x509_certificate structure on error paths?

If any validation step fails (such as cert.unsupported_sig or an error in
public_key_verify_signature), returning early with the ? operator will bypass
the x509_free_certificate cleanup for both the currently parsing certificate
(cert_ptr) and the previously parsed certificate (prev_cert).

Since they are stored as raw pointers, the drop semantics do not free them.

> +
> +            if let Some(prev) = prev_cert {
> +                // Check against previous certificate
> +                let rc = unsafe { bindings::public_key_verify_signature((*prev).pub_, cert.sig) };
> +
> +                if rc < 0 {
> +                    pr_err!("Signature validation error\n");
> +                    to_result(rc)?;
> +                }
> +            }

[ ... ]

> +        if let Some(prev) = prev_cert {
> +            if let Some(validate) = self.validate {
> +                let rc = unsafe { validate(self.dev, slot, prev) };
> +                to_result(rc)?;
> +            }
> +
> +            self.leaf_key = unsafe { Some((*prev).pub_) };

Does this overwrite self.leaf_key when validating multiple provisioned slots?

The caller in lib.rs iterates over all provisioned slots, calling
validate_cert_chain() for each:

    let mut provisioned_slots = state.provisioned_slots;
    while (provisioned_slots as usize) > 0 {
        let slot = provisioned_slots.trailing_zeros() as u8;

        if let Err(e) = state.validate_cert_chain(slot) {
            return e.to_errno() as c_int;
        }

        provisioned_slots &= !(1 << slot);
    }

Since self.leaf_key is a single Option, this will overwrite the previous public
key pointer without freeing it.

If a device has multiple provisioned slots, could signature verification later
in challenge() fail because self.leaf_key holds the public key of the last
provisioned slot rather than the first?

Also, does this leak the leaf x509_certificate structure on successful
validation?

At the successful completion of the certificate chain loop, the final
certificate remains in prev_cert. The public key is extracted into
self.leaf_key, but bindings::x509_free_certificate(prev) is never called.

While spdm_destroy() later frees the public key via public_key_free(), the
x509_certificate structure itself and its internal fields appear to be leaked
permanently.

> +        }
> +
> +        Ok(())
> +    }

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