Re: [PATCH v6 25/25] lib: rspdm: Expose public Rust API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] lib: rspdm: unsound safe SpdmState::new() and authenticate()
  with C callback
- [Medium] lib: rspdm: missing transport_sz check in SpdmState::new()
  and authenticate()
- [Medium] lib: rspdm: public Rust API not built as an importable crate
  in Kbuild
- [Low] lib: rspdm: Transport not implemented for &T in SpdmState::new()
  example
--

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

lib: rspdm: Expose public Rust API

This commit moves the SPDM authentication sequence into a public
SpdmState::authenticate() method, makes SpdmState and Transport public,
and adds a crate-level usage example.

> diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs
> index 6bfe1eedc4121..6b1f864fe7266 100644
> --- a/lib/rspdm/lib.rs
> +++ b/lib/rspdm/lib.rs
> @@ -9,6 +9,26 @@
>  //!
>  //! Top level library, including C compatible public functions to be called
>  //! from other subsytems.
> +//!
> +//! ```rust,ignore
> +//! struct MyTransport { }
> +//!
> +//! impl Transport for MyTransport {
> +//!     fn exchange(
> +//!         &self,
> +//!         dev: &Device,
> +//!         request: &[u8],
> +//!         response: &mut [u8],
> +//!     ) -> Result<usize, Error> {
> +//!         todo!()
> +//!     }
> +//! }
> +//!
> +//! fn authenticate_device(dev: &Device, transport: &MyTransport) -> Result {
> +//!     let mut state = SpdmState::new(ARef::from(dev), transport, 512, None);
> +//!     state.authenticate()
> +//! }
> +//! ```

[Severity: Low]
Does Transport need a blanket implementation for shared references (&T)
so that passing transport: &MyTransport to SpdmState::new() satisfies the
T: Transport bound?

[ ... ]

> @@ -30,7 +50,7 @@
>  };
>  
>  use crate::consts::SPDM_MIN_DATA_TRANSFER_SIZE;
> -use crate::state::{
> +pub use crate::state::{
>      SpdmState,
>      Transport, //
>  };

[Severity: Medium]
Can other Rust crates in the kernel import SpdmState and Transport with
the current Kbuild setup?

lib/rspdm/Makefile builds spdm-y := lib.o as a plain object file without
emitting crate metadata (.rmeta), exporting Rust symbols, or passing
--extern flags to downstream Rust crates in scripts/Makefile.build, so no
other Rust crate can reference or link against this public API.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index 059398e95b92d..0fe99caf27958 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs

[ ... ]

> @@ -194,7 +194,7 @@ impl<T: Transport> SpdmState<T> {
>      ///
>      /// `transport_sz` is the maximum message size the transport can carry,
>      /// and must be at least the SPDM minimum data transfer size (42 bytes).
> -    pub(crate) fn new(
> +    pub fn new(
>          dev: ARef<Device>,
>          transport: T,
>          transport_sz: u32,

[Severity: Medium]
Can a Rust caller pass transport_sz < SPDM_MIN_DATA_TRANSFER_SIZE (42) to
SpdmState::new() and underflow rsp_sz in SpdmState::get_certificate()?

The SPDM_MIN_DATA_TRANSFER_SIZE check remains only in the C entry point
spdm_create().  When Rust code calls SpdmState::new() and
SpdmState::authenticate() with a smaller transport_sz, data_transfer_sz
stays below the minimum; if transport_sz < GET_CERTIFICATE_RSP_HDR_SZ (8),
computing request.length in get_certificate() underflows usize:

lib/rspdm/state.rs:SpdmState::get_certificate() {
    ...
        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;
    ...
}

> @@ -232,6 +232,43 @@ pub(crate) fn hash_len(&self) -> usize {
>              .unwrap_or(0)
>      }
>  
> +    /// Authenticates the responder through a sequence of GET_VERSION,
> +    /// GET_CAPABILITIES, NEGOTIATE_ALGORITHMS, GET_DIGESTS, GET_CERTIFICATE
> +    /// and CHALLENGE exchanges.
> +    pub fn authenticate(&mut self) -> Result {
> +        self.get_version()?;
> +        self.get_capabilities()?;
> +        self.negotiate_algs()?;
> +        self.get_digests()?;
> +
> +        if self.provisioned_slots == 0 {
> +            return Err(EIO);
> +        }
> +
> +        let mut provisioned_slots = self.provisioned_slots;
> +        while (provisioned_slots as usize) > 0 {
> +            let slot = provisioned_slots.trailing_zeros() as u8;
> +
> +            self.get_certificate(slot)?;
> +
> +            provisioned_slots &= !(1 << slot);
> +        }
> +
> +        let mut provisioned_slots = self.provisioned_slots;
> +        while (provisioned_slots as usize) > 0 {
> +            let slot = provisioned_slots.trailing_zeros() as u8;
> +
> +            self.validate_cert_chain(slot)?;

[Severity: Medium]
Is it sound for SpdmState::new() and SpdmState::authenticate() to be safe
public functions while SpdmState::new() accepts a raw
bindings::spdm_validate callback?

bindings::spdm_validate is an Option<unsafe extern "C" fn(...)>, which
safe Rust code can construct and pass to SpdmState::new().  Calling
state.authenticate() then invokes that unsafe function pointer inside
SpdmState::validate_cert_chain() without an unsafe block at the call site,
allowing safe Rust callers to bypass callback preconditions (such as
device type assumptions) while also leaking raw kernel::bindings types
into the public Rust API:

lib/rspdm/state.rs:SpdmState::validate_cert_chain() {
    ...
        if let Some(validate) = self.validate {
            // SAFETY: Call the `validate` function provided with a valid
            // certificate pointer, which the callback must not modify or
            // free.
            let rc = unsafe { validate(self.dev.as_raw(), slot, leaf.as_raw()) };
            to_result(rc)?;
        }
    ...
}

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