On Tue Mar 31, 2026 at 3:37 AM BST, Alistair Francis wrote:
> On Wed, Mar 4, 2026 at 12:51 AM Jonathan Cameron
> <jonathan.cameron@huawei.com> wrote:
>>
>> On Wed, 11 Feb 2026 13:29:23 +1000
>> alistair23@gmail.com wrote:
>>
>> > From: Alistair Francis <alistair@alistair23.me>
>> >
>> > Support the GET_CERTIFICATE SPDM command.
>> >
>> > Signed-off-by: Alistair Francis <alistair@alistair23.me>
>>
>> Minor things inline. The endian handling in general needs
>> some care + possibly some tests.
>>
>> > diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
>> > index 2606b825c494..1e5656144611 100644
>> > --- a/lib/rspdm/state.rs
>> > +++ b/lib/rspdm/state.rs
>> > @@ -26,8 +26,9 @@
>> >      SPDM_REQ, SPDM_RSP_MIN_CAPS, SPDM_SLOTS, SPDM_VER_10, SPDM_VER_11, SPDM_VER_12,
>> >  };
>>
>> >  impl SpdmState {
>> >      pub(crate) fn new(
>> >          dev: *mut bindings::device,
>> > @@ -620,4 +629,118 @@ pub(crate) fn get_digests(&mut self) -> Result<(), Error> {
>> >
>> >          Ok(())
>> >      }
>> > +
>> > +    fn get_cert_exchange(
>> > +        &mut self,
>> > +        request_buf: &mut [u8],
>> > +        response_vec: &mut KVec<u8>,
>> > +        rsp_sz: usize,
>> > +    ) -> Result<&mut GetCertificateRsp, Error> {
>> > +        // SAFETY: `request` is repr(C) and packed, so we can convert it to a slice
>> > +        let response_buf = unsafe { from_raw_parts_mut(response_vec.as_mut_ptr(), rsp_sz) };
>> > +
>> > +        let rc = self.spdm_exchange(request_buf, response_buf)?;
>> > +
>> > +        if rc < (core::mem::size_of::<GetCertificateReq>() as i32) {
>> > +            pr_err!("Truncated certificate response\n");
>> > +            to_result(-(bindings::EIO as i32))?;
>> > +        }
>> > +
>> > +        // SAFETY: `rc` is the length of data read, which will be smaller
>> > +        // then the capacity of the vector
>> > +        unsafe { response_vec.inc_len(rc as usize) };
>> > +
>> > +        let response: &mut GetCertificateRsp = Untrusted::new_mut(response_vec).validate_mut()?;
>> > +
>> > +        if rc
>> > +            < (core::mem::size_of::<GetCertificateRsp>() + response.portion_length as usize) as i32
>>
>> As below, I'd keep the type matching the spec and have the little endian to cpu conversion out here.
>>
>>
>> > +        {
>> > +            pr_err!("Truncated certificate response\n");
>> > +            to_result(-(bindings::EIO as i32))?;
>> > +        }
>> > +
>> > +        Ok(response)
>> > +    }
>> > +
>> > +    pub(crate) fn get_certificate(&mut self, slot: u8) -> Result<(), Error> {
>> > +        let mut request = GetCertificateReq::default();
>> > +        request.version = self.version;
>> > +        request.param1 = slot;
>> > +
>> > +        let req_sz = core::mem::size_of::<GetCertificateReq>();
>> > +        let rsp_sz = ((core::mem::size_of::<GetCertificateRsp>() + 0xffff) as u32)
>>
>> Similar to earlier comment, do we have U16_MAX or similar available?
>>
>> > +            .min(self.transport_sz) as usize;
>> > +
>> > +        request.offset = 0;
>>
>> That's the default, so worth setting here?
>
> I like being explicit :)
>
>>
>> > +        request.length = (rsp_sz - core::mem::size_of::<GetCertificateRsp>()).to_le() as u16;
>>
>> Why store it in a u16 if it is le16?
>
> core::mem::size_of gives us a usize. So this ensures it's little
> endian then casts it to a u16.

This is broken code then.

In BE architectures, `.to_le()` flips the bytes around, and you do the truncation
*after* the byteswap, which would give you 0.

You'd need ((...) as u16).to_le()

Best,
Gary

>
> We could cast it to a __le16. u16 is a standard Rust type (compared to
> __le16 which is a internal kernel type), so I prefer u16 as it matches
> other Rust implementations.
>
>>
>> > +
>> > +        // SAFETY: `request` is repr(C) and packed, so we can convert it to a slice
>> > +        let request_buf = unsafe { from_raw_parts_mut(&mut request as *mut _ as *mut u8, req_sz) };
>> > +
>> > +        let mut response_vec: KVec<u8> = KVec::with_capacity(rsp_sz, GFP_KERNEL)?;
>> > +
>> > +        let response = self.get_cert_exchange(request_buf, &mut response_vec, rsp_sz)?;
>> > +
>> > +        let total_cert_len =
>> > +            ((response.portion_length + response.remainder_length) & 0xFFFF) as usize;
>> > +
>> > +        let mut certs_buf: KVec<u8> = KVec::new();
>> > +
>> > +        certs_buf.extend_from_slice(
>> > +            &response_vec[8..(8 + response.portion_length as usize)],
>> > +            GFP_KERNEL,
>> > +        )?;
>> > +
>> > +        let mut offset: usize = response.portion_length as usize;
>> > +        let mut remainder_length = response.remainder_length as usize;
>> > +
>> > +        while remainder_length > 0 {
>> > +            request.offset = offset.to_le() as u16;
>>
>> Similar to other places, why not just make the type __le16
>> and avoid need to cast.
>
> Same as above
>
>>
>> > +            request.length = (remainder_length
>> > +                .min(rsp_sz - core::mem::size_of::<GetCertificateRsp>()))
>> > +            .to_le() as u16;
>>
>> Likewise.
>
> and above
>
>>
>> > +
>> > +            let request_buf =
>> > +                unsafe { from_raw_parts_mut(&mut request as *mut _ as *mut u8, req_sz) };
>> > +
>> > +            let response = self.get_cert_exchange(request_buf, &mut response_vec, rsp_sz)?;
>> > +
>> > +            if response.portion_length == 0
>> > +                || (response.param1 & 0xF) != slot
>> > +                || offset as u16 + response.portion_length + response.remainder_length
>> > +                    != total_cert_len as u16
>> > +            {
>> > +                pr_err!("Malformed certificate response\n");
>> > +                to_result(-(bindings::EPROTO as i32))?;
>> > +            }
>> > +
>> > +            certs_buf.extend_from_slice(
>> > +                &response_vec[8..(8 + response.portion_length as usize)],
>> > +                GFP_KERNEL,
>> > +            )?;
>> > +            offset += response.portion_length as usize;
>> > +            remainder_length = response.remainder_length as usize;
>> > +        }
>> > +
>> > +        let header_length = core::mem::size_of::<SpdmCertChain>() + self.hash_len;
>> > +
>> > +        let ptr = certs_buf.as_mut_ptr();
>> > +        // SAFETY: `SpdmCertChain` is repr(C) and packed, so we can convert it from a slice
>> > +        let ptr = ptr.cast::<SpdmCertChain>();
>> > +        // SAFETY: `ptr` came from a reference and the cast above is valid.
>> > +        let certs: &mut SpdmCertChain = unsafe { &mut *ptr };
>> > +
>> > +        if total_cert_len < header_length
>> > +            || total_cert_len != usize::from_le(certs.length as usize)
>>
>> That's a confusing bit of casting as you are interpretting an __le16 as a usize
>> before doing the endian conversion?  Seems unlikely to get what you want
>> on a big endian machine.
>
> Yeah, I think this is wrong
>
>>
>> > +            || total_cert_len != certs_buf.len()
>> > +        {
>> > +            pr_err!("Malformed certificate chain in slot {slot}\n");
>> > +            to_result(-(bindings::EPROTO as i32))?;
>> > +        }
>> > +
>> > +        self.certs[slot as usize].clear();
>> > +        self.certs[slot as usize].extend_from_slice(&certs_buf, GFP_KERNEL)?;
>> > +
>> > +        Ok(())
>> > +    }
>> >  }
>> > diff --git a/lib/rspdm/validator.rs b/lib/rspdm/validator.rs
>> > index 2150a23997db..a8bc3378676f 100644
>> > --- a/lib/rspdm/validator.rs
>> > +++ b/lib/rspdm/validator.rs
>> > @@ -17,8 +17,9 @@
>> >  };
>>
>> >  #[repr(C, packed)]
>> > @@ -364,3 +365,62 @@ fn validate(unvalidated: &mut Unvalidated<KVec<u8>>) -> Result<Self, Self::Err>
>> >          Ok(rsp)
>> >      }
>> >  }
>>
>> > +#[repr(C, packed)]
>> > +pub(crate) struct GetCertificateRsp {
>> > +    pub(crate) version: u8,
>> > +    pub(crate) code: u8,
>> > +    pub(crate) param1: u8,
>> > +    pub(crate) param2: u8,
>> > +
>> > +    pub(crate) portion_length: u16,
>> > +    pub(crate) remainder_length: u16,
>> > +
>> > +    pub(crate) cert_chain: __IncompleteArrayField<u8>,
>> > +}
>> > +
>> > +impl Validate<&mut Unvalidated<KVec<u8>>> for &mut GetCertificateRsp {
>> > +    type Err = Error;
>> > +
>> > +    fn validate(unvalidated: &mut Unvalidated<KVec<u8>>) -> Result<Self, Self::Err> {
>> > +        let raw = unvalidated.raw_mut();
>> > +        if raw.len() < mem::size_of::<GetCertificateRsp>() {
>> > +            return Err(EINVAL);
>> > +        }
>> > +
>> > +        let ptr = raw.as_mut_ptr();
>> > +        // CAST: `GetCertificateRsp` only contains integers and has `repr(C)`.
>> > +        let ptr = ptr.cast::<GetCertificateRsp>();
>> > +        // SAFETY: `ptr` came from a reference and the cast above is valid.
>> > +        let rsp: &mut GetCertificateRsp = unsafe { &mut *ptr };
>> > +
>> > +        rsp.portion_length = rsp.portion_length.to_le();
>> > +        rsp.remainder_length = rsp.remainder_length.to_le();
>>
>> Why to_le()?  I can understand from_le() but then I'm a bit dubious about the
>> types. My gut feeling is that the validate code should leave these in little
>> endian and we should convert them only at time of use.
>
> Fixed
>
> Alistair
>
>>
>> > +
>> > +        Ok(rsp)
>> > +    }
>> > +}
>>