Re: [PATCH v5 16/23] rust: kernel: Add crypto hash bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] rust: kernel: hash module gated on CONFIG_CRYPTO breaks m/n builds
- [Low] rust: kernel: unchecked usize to u32 cast in Shash::digest()
- [Low] rust: kernel: wrong doc comment and missing #[inline] in Shash
--

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

rust: kernel: Add crypto hash bindings

This commit adds initial Rust abstractions in the kernel crate for synchronous
crypto hash transformations (Shash) wrapping the C crypto_shash APIs.

> diff --git a/rust/kernel/hash.rs b/rust/kernel/hash.rs
> new file mode 100644
> index 0000000000000..1800065d98767
> --- /dev/null
> +++ b/rust/kernel/hash.rs

[ ... ]

> +    /// Return the name of the hash
> +    pub fn name(&self) -> &'static CStr {
> +        self.name
> +    }
> +
> +    /// Return the name of the hash output
> +    pub fn hash_len(&self) -> usize {
> +        self.hash_len
> +    }

[Severity: Low]
This isn't a bug, but the doc comment on Shash::hash_len() says "Return the
name of the hash output" instead of the length of the hash output. Also,
should Shash::name(), Shash::hash_len(), and Shash::digest() in
rust/kernel/hash.rs be annotated with #[inline] as small kernel crate
abstraction functions?

> +    /// Computes a one-shot digest into `out`, which must be exactly the digest
> +    /// size of the selected hash algorithm.
> +    pub fn digest(&self, data: &[u8], out: &mut [u8]) -> Result {
> +        if out.len() != self.hash_len {
> +            return Err(EINVAL);
> +        }
> +
> +        // SAFETY: `self.desc` was created by `Shash::new()` and points to a
> +        // valid `shash_desc` with a valid `tfm`. `out` is checked above to be
> +        // exactly `crypto_shash_digestsize()` bytes.
> +        unsafe {
> +            to_result(bindings::crypto_shash_digest(
> +                self.desc.as_ptr(),
> +                data.as_ptr(),
> +                data.len() as u32,
> +                out.as_mut_ptr(),
> +            ))
> +        }
> +    }

[Severity: Low]
In Shash::digest(), could passing a slice with data.len() > u32::MAX on a
64-bit platform silently truncate the input length via data.len() as u32 and
compute a digest over only a prefix of data? Would u32::try_from(data.len())?
avoid the unchecked narrowing cast?

[ ... ]

> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index a3c54971e9dea..f7db5975e7621 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -82,6 +82,8 @@
>  pub mod fwctl;
>  #[cfg(CONFIG_GPU_BUDDY = "y")]
>  pub mod gpu;
> +#[cfg(CONFIG_CRYPTO)]
> +pub mod hash;

[Severity: Medium]
Will gating the built-in hash module in rust/kernel/lib.rs on
#[cfg(CONFIG_CRYPTO)] break the build when CONFIG_CRYPTO=m, or when
CONFIG_CRYPTO=y and CONFIG_CRYPTO_HASH2 is m or n?

Because CONFIG_CRYPTO is a tristate symbol, Kconfig emits --cfg=CONFIG_CRYPTO
for both CONFIG_CRYPTO=y and CONFIG_CRYPTO=m, whereas in C, CONFIG_CRYPTO is
only defined for built-in builds:

rust/helpers/hash.c:
    #ifdef CONFIG_CRYPTO

    __rust_helper unsigned int rust_helper_crypto_shash_descsize(struct crypto_shash *tfm)

When CONFIG_CRYPTO=m, Shash::new() and Shash::drop() in rust/kernel/hash.rs
are compiled into built-in rust/kernel.o, but bindgen does not generate
bindings::crypto_shash_descsize, bindings::crypto_shash_digestsize, or
bindings::crypto_free_shash from rust/helpers/hash.c.

In addition, Shash::new() and Shash::digest() call crypto_alloc_shash(),
crypto_shash_init(), and crypto_shash_digest(), which are compiled under
CONFIG_CRYPTO_HASH2 rather than CONFIG_CRYPTO:

crypto/Makefile:
    crypto_hash-y += ahash.o
    crypto_hash-y += shash.o
    obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o

If CONFIG_CRYPTO=y and CONFIG_CRYPTO_HASH2 is n or m, vmlinux fails to link
with undefined references to those shash symbols from rust/kernel.o.

Should rust/kernel/lib.rs use #[cfg(CONFIG_CRYPTO_HASH2 = "y")] (and
rust/helpers/hash.c guard on CONFIG_CRYPTO_HASH2) to match the other built-in
subsystem modules in rust/kernel/lib.rs?

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