Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 16 May 2023 08:25:19 +0000
Message-ID: <0101018823a9f0f2-a4620a25-6974-4464-a7ed-c997e9579243-000000@us-west-2.amazonses.com>
To:     ebiggers@kernel.org
Cc:     tomo@exabit.dev, rust-for-linux@vger.kernel.org,
        netdev@vger.kernel.org, linux-crypto@vger.kernel.org,
        fujita.tomonori@gmail.com
Subject: Re: [PATCH 1/2] rust: add synchronous message digest support
From:   FUJITA Tomonori <tomo@exabit.dev>
In-Reply-To: <20230516055219.GC2704@sol.localdomain>
References: <20230515043353.2324288-1-tomo@exabit.dev>
        <010101881db037b4-c8c941a9-c482-4759-9c07-b8bf645d96ed-000000@us-west-2.amazonses.com>
        <20230516055219.GC2704@sol.localdomain>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Feedback-ID: 1.us-west-2.j0GTvY5MHQQ5Spu+i4ZGzzYI1gDE7m7iuMEacWMZbe8=:AmazonSES
Precedence: bulk

Hi,

On Mon, 15 May 2023 22:52:19 -0700
Eric Biggers <ebiggers@kernel.org> wrote:

>> +#include <crypto/hash.h>
>>  #include <linux/bug.h>
>>  #include <linux/build_bug.h>
>>  #include <linux/err.h>
>> @@ -27,6 +28,29 @@
>>  #include <linux/sched/signal.h>
>>  #include <linux/wait.h>
>>  
>> +void rust_helper_crypto_free_shash(struct crypto_shash *tfm)
>> +{
>> +	crypto_free_shash(tfm);
>> +}
>> +EXPORT_SYMBOL_GPL(rust_helper_crypto_free_shash);
> 
> Shouldn't this code be compiled only when the crypto API is available?

Oops, I'll add #ifdef CONFIG_CRYPTO


>> +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 the 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;
>> +        let mut desc = ShashDesc { ptr, tfm, size };
>> +        // SAFETY: The `desc.tfm` is non-null and valid for the lifetime of this object.
>> +        unsafe { (*desc.ptr).tfm = desc.tfm.0 };
>> +        Ok(desc)
>> +    }
>> +
>> +    /// (Re)initializes message digest.
>> +    pub fn init(&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)
>> +        })
>> +    }
>> +
>> +    /// 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()) })
>> +    }
> 
> This doesn't enforce that init() is called before update() or finalize().  I
> think that needs to be checked in the Rust code, since the C code doesn't have
> defined behavior in that case.

Surely, Rust side should handle the case.

If the new() function internally calls init() before returning, it
works? The new() returns an initialized ShaDesc object.


Thanks for reviewing!