Re: [PATCH v6 19/25] lib: rspdm: Support SPDM get_certificate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] lib: rspdm: u16 length truncation in get_certificate() on 64K+
  page kernels
- [Low] lib: rspdm: duplicated initial exchange and missing check in
  get_certificate()
--

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

lib: rspdm: Support SPDM get_certificate

This commit adds support for the SPDM GET_CERTIFICATE command to retrieve
and store certificate chains for each provisioned slot from the responder.

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

[ ... ]

> @@ -500,4 +505,97 @@ pub(crate) fn get_digests(&mut self) -> Result<(), Error> {
>  
>          Ok(())
>      }
> +
> +    pub(crate) fn get_certificate(&mut self, slot: u8) -> Result<(), Error> {
> +        let mut request = GetCertificateReq::default();
> +        request.header.version = self.version;
> +        request.header.param1 = slot;
> +
> +        // Bound the per-chunk allocation to one page
> +        let rsp_sz = (GET_CERTIFICATE_RSP_HDR_SZ + PAGE_SIZE).min(self.data_transfer_sz as usize);
> +
> +        request.offset = 0;
> +        request.length = (rsp_sz - GET_CERTIFICATE_RSP_HDR_SZ) as u16;

[Severity: High]
Does casting rsp_sz - GET_CERTIFICATE_RSP_HDR_SZ to u16 truncate
request.length to 0 on 64K and 256K page kernels?

When self.data_transfer_sz is at least GET_CERTIFICATE_RSP_HDR_SZ +
PAGE_SIZE (such as with PCI_DOE_MAX_PAYLOAD), rsp_sz -
GET_CERTIFICATE_RSP_HDR_SZ equals PAGE_SIZE (0x10000 on 64K-page kernels),
which wraps to 0 when cast to u16 and causes certificate retrieval and
device authentication to fail.  Adding GET_CERTIFICATE_RSP_HDR_SZ to
PAGE_SIZE also makes rsp_sz equal PAGE_SIZE + 8, so KVec::from_elem()
allocates two pages instead of one.

Bounding rsp_sz to PAGE_SIZE.min(self.data_transfer_sz as usize) and
clamping the requested length to u16::MAX avoids the truncation and keeps
the buffer within a single page.

> +
> +        let response_vec: KVec<u8> = KVec::from_elem(0u8, rsp_sz, GFP_KERNEL)?;
> +
> +        let mut request_buf = request.to_bytes()?;
> +        let response: GetCertificateRsp = self
> +            .spdm_exchange(request_buf.as_mut_slice(), response_vec)?
> +            .validate(&*self)?;
> +
> +        if (response.header.param1 & 0xF) != slot {
> +            dev_err!(&self.dev, "Invalid slot response\n");
> +            return Err(EPROTO);
> +        }
> +
> +        let portion_length = response.portion_length;
> +        let rem_length = response.remainder_length;
> +
> +        let total_cert_len = portion_length as usize + rem_length as usize;
> +
> +        let mut certs_buf: KVec<u8> = KVec::new();
> +
> +        certs_buf.extend_from_slice(&response.cert_chain, GFP_KERNEL)?;
> +
> +        let mut offset: u16 = portion_length;
> +        let mut remainder_length = rem_length as usize;
> +
> +        while remainder_length > 0 {
> +            request.offset = offset;
> +            request.length = (remainder_length.min(rsp_sz - GET_CERTIFICATE_RSP_HDR_SZ)) as u16;
> +
> +            let mut request_buf = request.to_bytes()?;
> +
> +            let response_vec: KVec<u8> = KVec::from_elem(
> +                0u8,
> +                request.length as usize + GET_CERTIFICATE_RSP_HDR_SZ,
> +                GFP_KERNEL,
> +            )?;
> +
> +            let response: GetCertificateRsp = self
> +                .spdm_exchange(request_buf.as_mut_slice(), response_vec)?
> +                .validate(&*self)?;
> +
> +            let portion_length = response.portion_length;
> +            let rem_length = response.remainder_length;
> +
> +            if portion_length == 0
> +                || (response.header.param1 & 0xF) != slot
> +                || offset as usize + portion_length as usize + rem_length as usize != total_cert_len
> +            {
> +                dev_err!(&self.dev, "Malformed certificate response\n");
> +                return Err(EPROTO);
> +            }

[Severity: Low]
Can the initial exchange before the while loop be folded into the loop so
that the first response also checks portion_length == 0 and avoids
duplicating the request/response sequence?  Right now, if a responder
returns portion_length == 0 with a non-zero remainder_length on the first
chunk, or a total_cert_len exceeding u16::MAX, get_certificate() performs
an extra exchange before failing.

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