Messages in this thread |  | | From | Alistair Francis <> | | Date | Tue, 31 Mar 2026 13:29:40 +1000 | | Subject | Re: [RFC v3 22/27] lib: rspdm: Support SPDM certificate validation |
| |
On Wed, Mar 4, 2026 at 1:00 AM Jonathan Cameron <jonathan.cameron@huawei.com> wrote: > > On Wed, 11 Feb 2026 13:29:29 +1000 > alistair23@gmail.com wrote: > > > From: Alistair Francis <alistair@alistair23.me> > > > > Support validating the SPDM certificate chain. > > > > Signed-off-by: Alistair Francis <alistair@alistair23.me> > > Only minor thing inline + observation on the code that I think we want to remove > for now wrt to checking the root cert. > > > diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs > > index 1e5656144611..728b920beace 100644 > > --- a/lib/rspdm/state.rs > > +++ b/lib/rspdm/state.rs > > > @@ -743,4 +746,92 @@ 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_chain_buf = &self.certs[slot as usize]; > > + let cert_chain_len = cert_chain_buf.len(); > > + let header_len = 4 + self.hash_len; > > + > > + let mut offset = header_len; > > + let mut prev_cert: Option<*mut bindings::x509_certificate> = None; > > + > > + while offset < cert_chain_len { > > + let cert_len = unsafe { > > + bindings::x509_get_certificate_length( > > + &cert_chain_buf[offset..] as *const _ as *const u8, > > + cert_chain_len - offset, > > + ) > > + }; > > + > > + if cert_len < 0 { > > + pr_err!("Invalid certificate length\n"); > > + to_result(cert_len as i32)?; > > + } > > + > > + let _is_leaf_cert = if offset + cert_len as usize == cert_chain_len { > > + true > > + } else { > > + false > > + }; > Same as the following? > let _is_leaf_cert = offset + cert_len as usize == cert_chain_len;
Yep, same thing. My way is the Rust-y way to do it
Although _is_leaf_cert is unused, so I'll just remove it
> > > > + > > + 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 { > > + to_result(-(bindings::EKEYREJECTED as i32))?; > > + } > > + > > + 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)?; > > + } > > + } else { > > + // Check aginst root keyring > So this is the check that Dan and Jason are proposing we drop (for now). > > Works for me as easy to bring back later and lets us move forward > in the meantime. As long as userspace gets nothing to indicate > that we have in any way checked this root cert we are making no false claims.
Will do!
Alistair
> > > + let key = unsafe { > > + from_err_ptr(bindings::find_asymmetric_key( > > + self.keyring, > > + (*cert.sig).auth_ids[0], > > + (*cert.sig).auth_ids[1], > > + (*cert.sig).auth_ids[2], > > + false, > > + ))? > > + }; > > + > > + let rc = unsafe { bindings::verify_signature(key, cert.sig) }; > > + unsafe { bindings::key_put(key) }; > > + > > + if rc < 0 { > > + pr_err!("Root signature validation error\n"); > > + to_result(rc)?; > > + } > > + } > > + > > + if let Some(prev) = prev_cert { > > + unsafe { bindings::x509_free_certificate(prev) }; > > + } > > + > > + prev_cert = Some(cert_ptr); > > + offset += cert_len as usize; > > + } > > + > > + 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_) }; > > + } > > + > > + Ok(()) > > + } > > } > >
|  |