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

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

[Benno Lossin]

Dear crypto maintainers,

Fujita Tomonori has created some Rust bindings for the crypto API seen in
this thread. Here is a fragment of my review of said code:

On 25.06.23 12:08, Benno Lossin wrote:
>>>> +    /// 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?
> 
> Not sure what we should do in that case, will bring it up at the next
> team meeting. In Rust, `write` and `read` functions often output the
> number of bytes that were actually read/written. So maybe we should also
> do that here? Then you could just return `u32::MAX` and the user would
> have to call again. We could also call the C side multiple times until
> the entire buffer has been processed. But as the C side only supports
> u32 anyway, I think it would be a rare occurrence for `data` to be large.

I noted that in the code segment above that the length of the data
that is to be hashed is cast from a `usize` to a `u32`. Since
`usize = uintptr_t` this might be a problem for very large arguments.

Since the C side only accepts an `unsigned int`, it seems as if large inputs
are never the case. On the Rust side we are forced to use `usize`, since that
is the length of slices (the input type `&[u8]`).

We came up with the following solutions, but could not come to a consensus on any
particular one, could you please assist us in making this decision?

1. create a loop that calls the C API multiple times if the input is large
2. panic
3. truncate
4. return an error

Thanks a lot!

--
Cheers,
Benno

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

[Greg KH]

On Fri, Jun 30, 2023 at 02:48:37PM +0000, Benno Lossin wrote:
> We came up with the following solutions, but could not come to a consensus on any
> particular one, could you please assist us in making this decision?
> 
> 2. panic

Never an option in kernel code, sorry, please don't even suggest it.

thanks,

greg k-h

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

[Herbert Xu]

On Fri, Jun 30, 2023 at 02:48:37PM +0000, Benno Lossin wrote:
>
> 4. return an error

This would seem to make the most sense.

If there is ever a need to hash more than 4G of data, we would
be adding this to C first.

At this point I can't see why we would need to do that so an
error would be the appropriate response.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Benno Lossin]

------- Original Message -------
On Tuesday, July 4th, 2023 at 01:19, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Fri, Jun 30, 2023 at 02:48:37PM +0000, Benno Lossin wrote:
> 
> > 4. return an error
> 
> 
> This would seem to make the most sense.
> 
> If there is ever a need to hash more than 4G of data, we would
> be adding this to C first.
> 
> At this point I can't see why we would need to do that so an
> error would be the appropriate response.
> 
> Thanks,
> --
> Email: Herbert Xu herbert@gondor.apana.org.au
> 
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Thanks a lot for taking a look!

--
Cheers,
Benno