Re: [PATCH v3 16/21] lib: rspdm: Support SPDM negotiate_algorithms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Memory provenance truncation when storing a dynamically sized C struct as a statically sized Rust reference.
- [Low] Algorithm match arms become dead code under valid Kconfig combinations.
--

commit 8baafb6a67bc3beb2ad7bb9b93fd0f34551c04c9
Author: Alistair Francis <alistair@alistair23.me>

lib: rspdm: Support SPDM negotiate_algorithms

This commit adds support for the SPDM NEGOTIATE_ALGORITHMS command.
It extends SpdmState to negotiate and store cryptographic algorithms,
allocating the corresponding synchronous hash handle and context.

> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs
> index 5ef14c8ed2375..b78086c753702 100644
> --- a/lib/rspdm/state.rs
> +++ b/lib/rspdm/state.rs
[ ... ]
> @@ -373,4 +452,164 @@ pub(crate) fn get_capabilities(&mut self) -> Result<(), Error> {
>  
>          Ok(())
>      }
> +
> +    fn update_response_algs(&mut self) -> Result<(), Error> {
> +        match self.base_asym_alg {
> +            SPDM_ASYM_RSASSA_2048 => {
> +                self.sig_len = 256;
> +                self.base_asym_enc = CStr::from_bytes_with_nul(b"pkcs1\0")?;
> +            }

[Severity: Low]
Does this match arm become dead code when CONFIG_CRYPTO_RSA is disabled?

If CONFIG_CRYPTO_RSA is disabled, the SPDM_ASYM_RSA constant evaluates to 0.
Consequently, negotiate_algs() will reject RSA algorithms because they are not
present in SPDM_ASYM_ALGOS, making this match arm structurally unreachable.

Should these match arms be conditionally compiled so they don't produce dead
code under valid Kconfig combinations?

[ ... ]
> +        let desc_vec: KVec<u8> = KVec::from_elem(0u8, desc_len, GFP_KERNEL)?;
> +        // Consume the desc_vec to make sure it isn't dropped, untill we
> +        // manually drop it later
> +        let (desc_buf, _length, _capacity) = desc_vec.into_raw_parts();
> +
> +        // SAFETY: We are casting the allocation to be a shash_desc
> +        let desc = unsafe {
> +            core::mem::transmute::<*mut c_void, &mut bindings::shash_desc>(desc_buf as *mut c_void)
> +        };
> +        desc.tfm = self.shash;
> +
> +        self.desc = Some(desc);
> +
> +        /* Used frequently to compute offsets, so cache H */
> +        self.hash_len = unsafe { bindings::crypto_shash_digestsize(self.shash) as usize };
> +
> +        if let Some(desc) = &mut self.desc {
> +            // SAFETY: `self.desc` is a valid and initalised `shash_desc` sized buffer
> +            unsafe { to_result(bindings::crypto_shash_init(*desc)) }

[Severity: High]
Does transmuting the dynamically sized buffer to a static Rust reference here
cause memory provenance truncation?

When the dynamically sized buffer (which includes space for both the header
and a flexible array context) is transmuted into a statically sized Rust
reference &mut bindings::shash_desc, the Rust compiler strictly limits the
memory provenance of this reference to the header's size.

When this restricted reference is coerced back to a pointer and passed to C
functions like crypto_shash_init(), C writes into the trailing context array.
This access is out-of-bounds with respect to the Rust reference's provenance,
which triggers Undefined Behavior.

A similar issue occurs during Drop when the restricted reference is transmuted
back to a pointer to free the full capacity. Checking the end of the patch
series confirms this remains unfixed.

Would it be better to retain a raw pointer (*mut bindings::shash_desc)
instead of materializing a provenance-restricted Rust reference?

> +        } else {
> +            Err(ENOMEM)
> +        }
> +    }

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