Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

Hi, thanks a lot for reviewing!

On Mon, 19 Jun 2023 11:40:40 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

>> diff --git a/rust/kernel/crypto/hash.rs b/rust/kernel/crypto/hash.rs
>> new file mode 100644
>> index 000000000000..53f4a311b3b2
>> --- /dev/null
>> +++ b/rust/kernel/crypto/hash.rs
>> @@ -0,0 +1,118 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Cryptographic Hash operations.
>> +//!
>> +//! C headers: [`include/crypto/hash.h`](../../../../include/crypto/hash.h)
>> +
>> +use crate::{
>> +    error::{code::ENOMEM, from_err_ptr, to_result, Result},
>> +    str::CStr,
>> +};
>> +use alloc::alloc::{alloc, dealloc};
>> +use core::alloc::Layout;
>> +
>> +/// Corresponds to the kernel's `struct crypto_shash`.
>> +///
>> +/// # Invariants
>> +///
>> +/// The pointer is valid.
>> +pub struct Shash(*mut bindings::crypto_shash);
>> +
>> +impl Drop for Shash {
>> +    fn drop(&mut self) {
>> +        // SAFETY: The type invariant guarantees that the pointer is valid.
>> +        unsafe { bindings::crypto_free_shash(self.0) }
>> +    }
>> +}
>> +
>> +impl Shash {
>> +    /// Creates a [`Shash`] object for a message digest handle.
>> +    pub fn new(name: &CStr, t: u32, mask: u32) -> Result<Shash> {
>> +        // SAFETY: FFI call.
> 
> If there are no requirements then say so.

Understood. Looks like we have variation. Might be better to add the
standard to Documentation/rust/coding-guidelines.rst or such?

ubuntu@ip-172-30-47-114:~/git/linux$ grep -r FFI rust/kernel|grep SAFE
rust/kernel/task.rs:        // SAFETY: Just an FFI call with no additional safety requirements.
rust/kernel/lib.rs:    // SAFETY: FFI call.
rust/kernel/sync/arc.rs:            // SAFETY: There are no safety requirements for this FFI call.
rust/kernel/sync/arc.rs:            // SAFETY: There are no safety requirements for this FFI call.
rust/kernel/error.rs:        // SAFETY: Just an FFI call, there are no extra safety requirements.
rust/kernel/error.rs:///     // SAFETY: FFI call.
rust/kernel/error.rs:    // SAFETY: The FFI function does not deref the pointer.
rust/kernel/error.rs:        // SAFETY: The FFI function does not deref the pointer.

>> +        let ptr =
>> +            unsafe { from_err_ptr(bindings::crypto_alloc_shash(name.as_char_ptr(), t, mask)) }?;
>> +        // INVARIANT: `ptr` is valid and non-null since `crypto_alloc_shash`
>> +        // returned a valid pointer which was null-checked.
>> +        Ok(Self(ptr))
>> +    }
>> +
>> +    /// Sets optional key used by the hashing algorithm.
>> +    pub fn setkey(&mut self, data: &[u8]) -> Result {
> 
> This should be called `set_key`.

I thought that using C function names is a recommended way because
it's easier for subsystem maintainers to review.


>> +        // SAFETY: The type invariant guarantees that the pointer is valid.
>> +        to_result(unsafe {
>> +            bindings::crypto_shash_setkey(self.0, data.as_ptr(), data.len() as u32)
>> +        })
>> +    }
>> +
>> +    /// Returns the size of the result of the transformation.
>> +    pub fn digestsize(&self) -> u32 {
> 
> This should be called `digest_size`.

Ditto.

>> +        // SAFETY: The type invariant guarantees that the pointer is valid.
>> +        unsafe { bindings::crypto_shash_digestsize(self.0) }
>> +    }
>> +}
>> +
>> +/// Corresponds to the kernel's `struct shash_desc`.
>> +///
>> +/// # Invariants
>> +///
>> +/// The field `ptr` is valid.
>> +pub struct ShashDesc<'a> {
>> +    ptr: *mut bindings::shash_desc,
>> +    tfm: &'a Shash,
>> +    size: usize,
>> +}
>> +
>> +impl Drop for ShashDesc<'_> {
>> +    fn drop(&mut self) {
>> +        // SAFETY: The type invariant guarantees that the pointer is valid.
>> +        unsafe {
>> +            dealloc(
>> +                self.ptr.cast(),
>> +                Layout::from_size_align(self.size, 2).unwrap(),
>> +            );
> 
> Why do we own the pointer (i.e. why can we deallocate the memory)? Add as
> a TI (type invariant). Why are you using `dealloc`? Is there no C
> function that allocates a `struct shash_desc`? Why is the alignment 2?

No C function that allocates `struct shash_desc`. kmalloc() family is
used in the C side (or stack is used).

IIUC, the alignment isn't used in the kernel but dealloc() still
requires, right? I'm not sure what number should be used here.

>> +        }
>> +    }
>> +}
>> +
>> +impl<'a> ShashDesc<'a> {
>> +    /// Creates a [`ShashDesc`] object for a request data structure for message digest.
>> +    pub fn new(tfm: &'a Shash) -> Result<Self> {
>> +        // SAFETY: The type invariant guarantees that `tfm.0` pointer is valid.
>> +        let size = core::mem::size_of::<bindings::shash_desc>()
>> +            + unsafe { bindings::crypto_shash_descsize(tfm.0) } as usize;
>> +        let layout = Layout::from_size_align(size, 2)?;
>> +        let ptr = unsafe { alloc(layout) } as *mut bindings::shash_desc;
> 
> Several things:
> - The `SAFETY` comment for `crypto_shash_descsize` should be directly above
>   the `unsafe` block,maybe factor that out into its own variable.

Ok.

> - Why is 2 the right alignment?

As long as the size is larger than alignment, alignment arugment is
meaningless. Like dealloc, not sure what should be used.


> - Missing `SAFETY` comment for `alloc`.

Will be fixed.

> - Why are you manually creating this layout from size and alignment? Is it
>   not possible to do it via the `Layout` API?

What function should be used?

>> +        if ptr.is_null() {
>> +            return Err(ENOMEM);
>> +        }
>> +        // INVARIANT: `ptr` is valid and non-null since `alloc`
>> +        // returned a valid pointer which was null-checked.
>> +        let mut desc = ShashDesc { ptr, tfm, size };
>> +        // SAFETY: `desc.ptr` is valid and non-null since `alloc`
>> +        // returned a valid pointer which was null-checked.
>> +        // Additionally, The type invariant guarantees that `tfm.0` is valid.
>> +        unsafe { (*desc.ptr).tfm = desc.tfm.0 };
>> +        desc.reset()?;
>> +        Ok(desc)
>> +    }
>> +
>> +    /// Re-initializes message digest.
>> +    pub fn reset(&mut self) -> Result {
>> +        // SAFETY: The type invariant guarantees that the pointer is valid.
>> +        to_result(unsafe { bindings::crypto_shash_init(self.ptr) })
>> +    }
>> +
>> +    /// Adds data to message digest for processing.
>> +    pub fn update(&mut self, data: &[u8]) -> Result {
>> +        // SAFETY: The type invariant guarantees that the pointer is valid.
>> +        to_result(unsafe {
>> +            bindings::crypto_shash_update(self.ptr, data.as_ptr(), data.len() as u32)
>> +        })
> 
> What if `data.len() > u32::MAX`?

The buffer might not be updated properly, I guess. Should check the case?

>> +    }
>> +
>> +    /// Calculates message digest.
>> +    pub fn finalize(&mut self, output: &mut [u8]) -> Result {
>> +        // SAFETY: The type invariant guarantees that the pointer is valid.
>> +        to_result(unsafe { bindings::crypto_shash_final(self.ptr, output.as_mut_ptr()) })
> 
> As already mentioned by Alex, this needs a size check.

Will be fixed.


thanks,