* [RFC PATCH v2 0/2] Rust abstractions for Crypto API
@ 2023-06-15 14:23 FUJITA Tomonori
2023-06-15 14:23 ` [RFC PATCH v2 1/2] rust: add synchronous message digest support FUJITA Tomonori
2023-06-15 14:23 ` [RFC PATCH v2 2/2] rust: add Random Number Generator algorithms support FUJITA Tomonori
0 siblings, 2 replies; 17+ messages in thread
From: FUJITA Tomonori @ 2023-06-15 14:23 UTC (permalink / raw)
To: rust-for-linux
Hi,
Before sending v2 of my crypto patch [1] to linux-crypto ml and
checking the chance of Rust bindings for crypto being accepted, I'd
like to iron out Rust issues. I'd appreciate any feedback.
Once reviewing on rust-for-linux ml is done, I'll send only crypto
support to linux-crypto (drops the socket support patch to netdev).
Changes since v1:
- addressed Eric's comments on hash support [2].
- add rng support
- split files
[1] https://lore.kernel.org/netdev/010101881db036fb-2fb6981d-e0ef-4ad1-83c3-54d64b6d93b3-000000@us-west-2.amazonses.com/T/
[2] https://www.spinics.net/lists/netdev/msg904380.html
FUJITA Tomonori (2):
rust: add synchronous message digest support
rust: add Random Number Generator algorithms support
rust/bindings/bindings_helper.h | 2 +
rust/helpers.c | 38 ++++++++++
rust/kernel/crypto.rs | 6 ++
rust/kernel/crypto/hash.rs | 118 ++++++++++++++++++++++++++++++++
rust/kernel/crypto/rng.rs | 72 +++++++++++++++++++
rust/kernel/lib.rs | 2 +
6 files changed, 238 insertions(+)
create mode 100644 rust/kernel/crypto.rs
create mode 100644 rust/kernel/crypto/hash.rs
create mode 100644 rust/kernel/crypto/rng.rs
base-commit: d2e3115d717197cb2bc020dd1f06b06538474ac3
--
2.34.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-06-15 14:23 [RFC PATCH v2 0/2] Rust abstractions for Crypto API FUJITA Tomonori
@ 2023-06-15 14:23 ` FUJITA Tomonori
2023-06-15 15:01 ` Greg KH
` (2 more replies)
2023-06-15 14:23 ` [RFC PATCH v2 2/2] rust: add Random Number Generator algorithms support FUJITA Tomonori
1 sibling, 3 replies; 17+ messages in thread
From: FUJITA Tomonori @ 2023-06-15 14:23 UTC (permalink / raw)
To: rust-for-linux
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
rust/bindings/bindings_helper.h | 1 +
rust/helpers.c | 26 +++++++
rust/kernel/crypto.rs | 5 ++
rust/kernel/crypto/hash.rs | 118 ++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
5 files changed, 152 insertions(+)
create mode 100644 rust/kernel/crypto.rs
create mode 100644 rust/kernel/crypto/hash.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 3e601ce2548d..2f198c6d5de5 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -6,6 +6,7 @@
* Sorted alphabetically.
*/
+#include <crypto/hash.h>
#include <linux/errname.h>
#include <linux/slab.h>
#include <linux/refcount.h>
diff --git a/rust/helpers.c b/rust/helpers.c
index bb594da56137..7966902ed8eb 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -18,6 +18,7 @@
* accidentally exposed.
*/
+#include <crypto/hash.h>
#include <linux/bug.h>
#include <linux/build_bug.h>
#include <linux/err.h>
@@ -28,6 +29,31 @@
#include <linux/sched/signal.h>
#include <linux/wait.h>
+#ifdef CONFIG_CRYPTO
+void rust_helper_crypto_free_shash(struct crypto_shash *tfm)
+{
+ crypto_free_shash(tfm);
+}
+EXPORT_SYMBOL_GPL(rust_helper_crypto_free_shash);
+
+unsigned int rust_helper_crypto_shash_digestsize(struct crypto_shash *tfm)
+{
+ return crypto_shash_digestsize(tfm);
+}
+EXPORT_SYMBOL_GPL(rust_helper_crypto_shash_digestsize);
+
+unsigned int rust_helper_crypto_shash_descsize(struct crypto_shash *tfm)
+{
+ return crypto_shash_descsize(tfm);
+}
+EXPORT_SYMBOL_GPL(rust_helper_crypto_shash_descsize);
+
+int rust_helper_crypto_shash_init(struct shash_desc *desc) {
+ return crypto_shash_init(desc);
+}
+EXPORT_SYMBOL_GPL(rust_helper_crypto_shash_init);
+#endif
+
__noreturn void rust_helper_BUG(void)
{
BUG();
diff --git a/rust/kernel/crypto.rs b/rust/kernel/crypto.rs
new file mode 100644
index 000000000000..f80dd7bd3381
--- /dev/null
+++ b/rust/kernel/crypto.rs
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Cryptography.
+
+pub mod hash;
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.
+ 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 {
+ // 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 {
+ // 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(),
+ );
+ }
+ }
+}
+
+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;
+ 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)
+ })
+ }
+
+ /// 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()) })
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 85b261209977..3cb8bd8a17d9 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -31,6 +31,8 @@
#[cfg(not(testlib))]
mod allocator;
mod build_assert;
+#[cfg(CONFIG_CRYPTO)]
+pub mod crypto;
pub mod error;
pub mod init;
pub mod ioctl;
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [RFC PATCH v2 2/2] rust: add Random Number Generator algorithms support
2023-06-15 14:23 [RFC PATCH v2 0/2] Rust abstractions for Crypto API FUJITA Tomonori
2023-06-15 14:23 ` [RFC PATCH v2 1/2] rust: add synchronous message digest support FUJITA Tomonori
@ 2023-06-15 14:23 ` FUJITA Tomonori
2023-06-19 11:41 ` Benno Lossin
1 sibling, 1 reply; 17+ messages in thread
From: FUJITA Tomonori @ 2023-06-15 14:23 UTC (permalink / raw)
To: rust-for-linux
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
rust/bindings/bindings_helper.h | 1 +
rust/helpers.c | 12 ++++++
rust/kernel/crypto.rs | 1 +
rust/kernel/crypto/rng.rs | 72 +++++++++++++++++++++++++++++++++
4 files changed, 86 insertions(+)
create mode 100644 rust/kernel/crypto/rng.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 2f198c6d5de5..089ac38c6461 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -7,6 +7,7 @@
*/
#include <crypto/hash.h>
+#include <crypto/rng.h>
#include <linux/errname.h>
#include <linux/slab.h>
#include <linux/refcount.h>
diff --git a/rust/helpers.c b/rust/helpers.c
index 7966902ed8eb..e4dcd611738f 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -19,6 +19,7 @@
*/
#include <crypto/hash.h>
+#include <crypto/rng.h>
#include <linux/bug.h>
#include <linux/build_bug.h>
#include <linux/err.h>
@@ -52,6 +53,17 @@ int rust_helper_crypto_shash_init(struct shash_desc *desc) {
return crypto_shash_init(desc);
}
EXPORT_SYMBOL_GPL(rust_helper_crypto_shash_init);
+
+void rust_helper_crypto_free_rng(struct crypto_rng *tfm) {
+ crypto_free_rng(tfm);
+}
+EXPORT_SYMBOL_GPL(rust_helper_crypto_free_rng);
+
+int rust_helper_crypto_rng_generate(struct crypto_rng *tfm, const u8 *src,
+ unsigned int slen, u8 *dst, unsigned int dlen) {
+ return crypto_rng_generate(tfm, src, slen, dst, dlen);
+}
+EXPORT_SYMBOL_GPL(rust_helper_crypto_rng_generate);
#endif
__noreturn void rust_helper_BUG(void)
diff --git a/rust/kernel/crypto.rs b/rust/kernel/crypto.rs
index f80dd7bd3381..a1995e6c85d4 100644
--- a/rust/kernel/crypto.rs
+++ b/rust/kernel/crypto.rs
@@ -3,3 +3,4 @@
//! Cryptography.
pub mod hash;
+pub mod rng;
diff --git a/rust/kernel/crypto/rng.rs b/rust/kernel/crypto/rng.rs
new file mode 100644
index 000000000000..2215a8344669
--- /dev/null
+++ b/rust/kernel/crypto/rng.rs
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Random number generator.
+//!
+//! C headers: [`include/crypto/rng.h`](../../../../include/crypto/rng.h)
+
+use crate::{
+ error::{from_err_ptr, to_result, Result},
+ str::CStr,
+};
+
+/// Corresponds to the kernel's `struct crypto_rng`.
+///
+/// # Invariants
+///
+/// The pointer is valid.
+pub struct Rng(*mut bindings::crypto_rng);
+
+impl Drop for Rng {
+ fn drop(&mut self) {
+ // SAFETY: The type invariant guarantees that `self.0` is valid.
+ if unsafe { bindings::crypto_default_rng } == self.0 {
+ // SAFETY: FFI call.
+ unsafe {
+ bindings::crypto_put_default_rng();
+ }
+ } else {
+ // SAFETY: The type invariant guarantees that `self.0` is valid.
+ unsafe { bindings::crypto_free_rng(self.0) };
+ }
+ }
+}
+
+impl Rng {
+ /// Creates a [`Rng`] instance.
+ pub fn new(name: &CStr, t: u32, mask: u32) -> Result<Self> {
+ // SAFETY: FFI call.
+ let ptr = unsafe { from_err_ptr(bindings::crypto_alloc_rng(name.as_char_ptr(), t, mask)) }?;
+ // INVARIANT: `ptr` is valid and non-null since `crypto_alloc_rng`
+ // returned a valid pointer which was null-checked.
+ Ok(Self(ptr))
+ }
+
+ /// Creates a [`Rng`] instance with a default algorithm.
+ pub fn new_with_default() -> Result<Self> {
+ // SAFETY: FFI call.
+ to_result(unsafe { bindings::crypto_get_default_rng() })?;
+ // SAFETY: The C API guarantees that `crypto_default_rng` is valid until
+ // `crypto_put_default_rng` is called.
+ Ok(Self(unsafe { bindings::crypto_default_rng }))
+ }
+
+ /// Get a random number.
+ pub fn generate(&mut self, src: &[u8], dst: &mut [u8]) -> Result {
+ // SAFETY: The type invariant guarantees that the pointer is valid.
+ to_result(unsafe {
+ bindings::crypto_rng_generate(
+ self.0,
+ src.as_ptr(),
+ src.len() as u32,
+ dst.as_mut_ptr(),
+ dst.len() as u32,
+ )
+ })
+ }
+
+ /// Re-initializes the [`Rng`] instance.
+ pub fn reset(&mut self, seed: &[u8]) -> Result {
+ // SAFETY: The type invariant guarantees that the pointer is valid.
+ to_result(unsafe { bindings::crypto_rng_reset(self.0, seed.as_ptr(), seed.len() as u32) })
+ }
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-06-15 14:23 ` [RFC PATCH v2 1/2] rust: add synchronous message digest support FUJITA Tomonori
@ 2023-06-15 15:01 ` Greg KH
2023-06-15 15:33 ` FUJITA Tomonori
2023-06-15 15:02 ` Alex Gaynor
2023-06-19 11:40 ` Benno Lossin
2 siblings, 1 reply; 17+ messages in thread
From: Greg KH @ 2023-06-15 15:01 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: rust-for-linux
On Thu, Jun 15, 2023 at 11:23:10PM +0900, FUJITA Tomonori wrote:
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
For obvious reasons, we can't take commits with no changelog text at
all, nor would you want us to :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-06-15 14:23 ` [RFC PATCH v2 1/2] rust: add synchronous message digest support FUJITA Tomonori
2023-06-15 15:01 ` Greg KH
@ 2023-06-15 15:02 ` Alex Gaynor
2023-06-15 15:24 ` FUJITA Tomonori
2023-06-19 11:40 ` Benno Lossin
2 siblings, 1 reply; 17+ messages in thread
From: Alex Gaynor @ 2023-06-15 15:02 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: rust-for-linux
One note inline
On Thu, Jun 15, 2023 at 10:50 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers.c | 26 +++++++
> rust/kernel/crypto.rs | 5 ++
> rust/kernel/crypto/hash.rs | 118 ++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 2 +
> 5 files changed, 152 insertions(+)
> create mode 100644 rust/kernel/crypto.rs
> create mode 100644 rust/kernel/crypto/hash.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 3e601ce2548d..2f198c6d5de5 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -6,6 +6,7 @@
> * Sorted alphabetically.
> */
>
> +#include <crypto/hash.h>
> #include <linux/errname.h>
> #include <linux/slab.h>
> #include <linux/refcount.h>
> diff --git a/rust/helpers.c b/rust/helpers.c
> index bb594da56137..7966902ed8eb 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -18,6 +18,7 @@
> * accidentally exposed.
> */
>
> +#include <crypto/hash.h>
> #include <linux/bug.h>
> #include <linux/build_bug.h>
> #include <linux/err.h>
> @@ -28,6 +29,31 @@
> #include <linux/sched/signal.h>
> #include <linux/wait.h>
>
> +#ifdef CONFIG_CRYPTO
> +void rust_helper_crypto_free_shash(struct crypto_shash *tfm)
> +{
> + crypto_free_shash(tfm);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_crypto_free_shash);
> +
> +unsigned int rust_helper_crypto_shash_digestsize(struct crypto_shash *tfm)
> +{
> + return crypto_shash_digestsize(tfm);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_crypto_shash_digestsize);
> +
> +unsigned int rust_helper_crypto_shash_descsize(struct crypto_shash *tfm)
> +{
> + return crypto_shash_descsize(tfm);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_crypto_shash_descsize);
> +
> +int rust_helper_crypto_shash_init(struct shash_desc *desc) {
> + return crypto_shash_init(desc);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_crypto_shash_init);
> +#endif
> +
> __noreturn void rust_helper_BUG(void)
> {
> BUG();
> diff --git a/rust/kernel/crypto.rs b/rust/kernel/crypto.rs
> new file mode 100644
> index 000000000000..f80dd7bd3381
> --- /dev/null
> +++ b/rust/kernel/crypto.rs
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Cryptography.
> +
> +pub mod hash;
> 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.
> + 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 {
> + // 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 {
> + // 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(),
> + );
> + }
> + }
> +}
> +
> +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;
> + 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)
> + })
> + }
> +
> + /// 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()) })
Is this sound? What happens if output.len() < the digest size?
> + }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 85b261209977..3cb8bd8a17d9 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -31,6 +31,8 @@
> #[cfg(not(testlib))]
> mod allocator;
> mod build_assert;
> +#[cfg(CONFIG_CRYPTO)]
> +pub mod crypto;
> pub mod error;
> pub mod init;
> pub mod ioctl;
> --
> 2.34.1
>
--
All that is necessary for evil to succeed is for good people to do nothing.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-06-15 15:02 ` Alex Gaynor
@ 2023-06-15 15:24 ` FUJITA Tomonori
0 siblings, 0 replies; 17+ messages in thread
From: FUJITA Tomonori @ 2023-06-15 15:24 UTC (permalink / raw)
To: alex.gaynor; +Cc: fujita.tomonori, rust-for-linux
Hi,
On Thu, 15 Jun 2023 11:02:31 -0400
Alex Gaynor <alex.gaynor@gmail.com> wrote:
>> + /// 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()) })
>
> Is this sound? What happens if output.len() < the digest size?
Good point, surely we need that checking before calling crypto_shash_final().
It will be fixed in the next version.
Thanks a lot!
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-06-15 15:01 ` Greg KH
@ 2023-06-15 15:33 ` FUJITA Tomonori
0 siblings, 0 replies; 17+ messages in thread
From: FUJITA Tomonori @ 2023-06-15 15:33 UTC (permalink / raw)
To: gregkh; +Cc: fujita.tomonori, rust-for-linux
Hi,
On Thu, 15 Jun 2023 17:01:06 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Jun 15, 2023 at 11:23:10PM +0900, FUJITA Tomonori wrote:
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>
> For obvious reasons, we can't take commits with no changelog text at
> all, nor would you want us to :)
Oops, definitely I'll write nice changelog in the next version.
thanks,
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-06-15 14:23 ` [RFC PATCH v2 1/2] rust: add synchronous message digest support FUJITA Tomonori
2023-06-15 15:01 ` Greg KH
2023-06-15 15:02 ` Alex Gaynor
@ 2023-06-19 11:40 ` Benno Lossin
2023-06-22 2:14 ` FUJITA Tomonori
2 siblings, 1 reply; 17+ messages in thread
From: Benno Lossin @ 2023-06-19 11:40 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: rust-for-linux
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers.c | 26 +++++++
> rust/kernel/crypto.rs | 5 ++
> rust/kernel/crypto/hash.rs | 118 ++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 2 +
> 5 files changed, 152 insertions(+)
> create mode 100644 rust/kernel/crypto.rs
> create mode 100644 rust/kernel/crypto/hash.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 3e601ce2548d..2f198c6d5de5 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -6,6 +6,7 @@
> * Sorted alphabetically.
> */
>
> +#include <crypto/hash.h>
> #include <linux/errname.h>
> #include <linux/slab.h>
> #include <linux/refcount.h>
> diff --git a/rust/helpers.c b/rust/helpers.c
> index bb594da56137..7966902ed8eb 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -18,6 +18,7 @@
> * accidentally exposed.
> */
>
> +#include <crypto/hash.h>
> #include <linux/bug.h>
> #include <linux/build_bug.h>
> #include <linux/err.h>
> @@ -28,6 +29,31 @@
> #include <linux/sched/signal.h>
> #include <linux/wait.h>
>
> +#ifdef CONFIG_CRYPTO
> +void rust_helper_crypto_free_shash(struct crypto_shash *tfm)
> +{
> + crypto_free_shash(tfm);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_crypto_free_shash);
> +
> +unsigned int rust_helper_crypto_shash_digestsize(struct crypto_shash *tfm)
> +{
> + return crypto_shash_digestsize(tfm);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_crypto_shash_digestsize);
> +
> +unsigned int rust_helper_crypto_shash_descsize(struct crypto_shash *tfm)
> +{
> + return crypto_shash_descsize(tfm);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_crypto_shash_descsize);
> +
> +int rust_helper_crypto_shash_init(struct shash_desc *desc) {
> + return crypto_shash_init(desc);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_crypto_shash_init);
> +#endif
> +
> __noreturn void rust_helper_BUG(void)
> {
> BUG();
> diff --git a/rust/kernel/crypto.rs b/rust/kernel/crypto.rs
> new file mode 100644
> index 000000000000..f80dd7bd3381
> --- /dev/null
> +++ b/rust/kernel/crypto.rs
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Cryptography.
> +
> +pub mod hash;
> 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.
> + 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`.
> + // 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`.
> + // 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?
> + }
> + }
> +}
> +
> +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.
- Why is 2 the right alignment?
- Missing `SAFETY` comment for `alloc`.
- Why are you manually creating this layout from size and alignment? Is it
not possible to do it via the `Layout` API?
> + 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`?
> + }
> +
> + /// 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.
--
Cheers,
Benno
> + }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 85b261209977..3cb8bd8a17d9 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -31,6 +31,8 @@
> #[cfg(not(testlib))]
> mod allocator;
> mod build_assert;
> +#[cfg(CONFIG_CRYPTO)]
> +pub mod crypto;
> pub mod error;
> pub mod init;
> pub mod ioctl;
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 2/2] rust: add Random Number Generator algorithms support
2023-06-15 14:23 ` [RFC PATCH v2 2/2] rust: add Random Number Generator algorithms support FUJITA Tomonori
@ 2023-06-19 11:41 ` Benno Lossin
2023-07-06 4:05 ` FUJITA Tomonori
0 siblings, 1 reply; 17+ messages in thread
From: Benno Lossin @ 2023-06-19 11:41 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: rust-for-linux
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers.c | 12 ++++++
> rust/kernel/crypto.rs | 1 +
> rust/kernel/crypto/rng.rs | 72 +++++++++++++++++++++++++++++++++
> 4 files changed, 86 insertions(+)
> create mode 100644 rust/kernel/crypto/rng.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 2f198c6d5de5..089ac38c6461 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -7,6 +7,7 @@
> */
>
> #include <crypto/hash.h>
> +#include <crypto/rng.h>
> #include <linux/errname.h>
> #include <linux/slab.h>
> #include <linux/refcount.h>
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 7966902ed8eb..e4dcd611738f 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -19,6 +19,7 @@
> */
>
> #include <crypto/hash.h>
> +#include <crypto/rng.h>
> #include <linux/bug.h>
> #include <linux/build_bug.h>
> #include <linux/err.h>
> @@ -52,6 +53,17 @@ int rust_helper_crypto_shash_init(struct shash_desc *desc) {
> return crypto_shash_init(desc);
> }
> EXPORT_SYMBOL_GPL(rust_helper_crypto_shash_init);
> +
> +void rust_helper_crypto_free_rng(struct crypto_rng *tfm) {
> + crypto_free_rng(tfm);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_crypto_free_rng);
> +
> +int rust_helper_crypto_rng_generate(struct crypto_rng *tfm, const u8 *src,
> + unsigned int slen, u8 *dst, unsigned int dlen) {
> + return crypto_rng_generate(tfm, src, slen, dst, dlen);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_crypto_rng_generate);
> #endif
>
> __noreturn void rust_helper_BUG(void)
> diff --git a/rust/kernel/crypto.rs b/rust/kernel/crypto.rs
> index f80dd7bd3381..a1995e6c85d4 100644
> --- a/rust/kernel/crypto.rs
> +++ b/rust/kernel/crypto.rs
> @@ -3,3 +3,4 @@
> //! Cryptography.
>
> pub mod hash;
> +pub mod rng;
> diff --git a/rust/kernel/crypto/rng.rs b/rust/kernel/crypto/rng.rs
> new file mode 100644
> index 000000000000..2215a8344669
> --- /dev/null
> +++ b/rust/kernel/crypto/rng.rs
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Random number generator.
> +//!
> +//! C headers: [`include/crypto/rng.h`](../../../../include/crypto/rng.h)
> +
> +use crate::{
> + error::{from_err_ptr, to_result, Result},
> + str::CStr,
> +};
> +
> +/// Corresponds to the kernel's `struct crypto_rng`.
> +///
> +/// # Invariants
> +///
> +/// The pointer is valid.
> +pub struct Rng(*mut bindings::crypto_rng);
> +
> +impl Drop for Rng {
> + fn drop(&mut self) {
> + // SAFETY: The type invariant guarantees that `self.0` is valid.
This safety comment should explain why it is safe to access this mutable
static variable.
> + if unsafe { bindings::crypto_default_rng } == self.0 {
> + // SAFETY: FFI call.
> + unsafe {
> + bindings::crypto_put_default_rng();
> + }
> + } else {
> + // SAFETY: The type invariant guarantees that `self.0` is valid.
> + unsafe { bindings::crypto_free_rng(self.0) };
> + }
> + }
> +}
> +
> +impl Rng {
> + /// Creates a [`Rng`] instance.
> + pub fn new(name: &CStr, t: u32, mask: u32) -> Result<Self> {
> + // SAFETY: FFI call.
If there are no requirements then say so.
> + let ptr = unsafe { from_err_ptr(bindings::crypto_alloc_rng(name.as_char_ptr(), t, mask)) }?;
> + // INVARIANT: `ptr` is valid and non-null since `crypto_alloc_rng`
> + // returned a valid pointer which was null-checked.
> + Ok(Self(ptr))
> + }
> +
> + /// Creates a [`Rng`] instance with a default algorithm.
> + pub fn new_with_default() -> Result<Self> {
> + // SAFETY: FFI call.
> + to_result(unsafe { bindings::crypto_get_default_rng() })?;
> + // SAFETY: The C API guarantees that `crypto_default_rng` is valid until
> + // `crypto_put_default_rng` is called.
> + Ok(Self(unsafe { bindings::crypto_default_rng }))
> + }
> +
> + /// Get a random number.
> + pub fn generate(&mut self, src: &[u8], dst: &mut [u8]) -> Result {
> + // SAFETY: The type invariant guarantees that the pointer is valid.
> + to_result(unsafe {
> + bindings::crypto_rng_generate(
> + self.0,
> + src.as_ptr(),
> + src.len() as u32,
What happens when `src.len() > u32::MAX`? Then it will overflow here and
write less than `len` random bytes. Not sure what we want to do in this
case though, maybe call it in a loop? Return an error? Also below.
> + dst.as_mut_ptr(),
> + dst.len() as u32,
> + )
> + })
> + }
> +
> + /// Re-initializes the [`Rng`] instance.
> + pub fn reset(&mut self, seed: &[u8]) -> Result {
> + // SAFETY: The type invariant guarantees that the pointer is valid.
> + to_result(unsafe { bindings::crypto_rng_reset(self.0, seed.as_ptr(), seed.len() as u32) })
Same problem with the as u32 cast here.
--
Cheers,
Benno
> + }
> +}
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-06-19 11:40 ` Benno Lossin
@ 2023-06-22 2:14 ` FUJITA Tomonori
2023-06-25 10:08 ` Benno Lossin
0 siblings, 1 reply; 17+ messages in thread
From: FUJITA Tomonori @ 2023-06-22 2:14 UTC (permalink / raw)
To: benno.lossin; +Cc: fujita.tomonori, rust-for-linux
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,
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-06-22 2:14 ` FUJITA Tomonori
@ 2023-06-25 10:08 ` Benno Lossin
2023-06-25 11:55 ` FUJITA Tomonori
2023-06-30 14:48 ` Benno Lossin
0 siblings, 2 replies; 17+ messages in thread
From: Benno Lossin @ 2023-06-25 10:08 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: rust-for-linux, Gary Guo
On 6/22/23 04:14, FUJITA Tomonori wrote:
> 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.
I am currently working on more standardized safety comments. Until we
agree on how it should be, you can just choose a wording that you think
is OK.
>
>>> + 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.
IMO having a `_` that separates words helps a lot with readability.
Especially with `digestsize`. I also think that adding an `_` will not
confuse the subsystem maintainers, so we should just do it.
>
>
>>> + // 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.
CC'ing Gary, since I am not familiar with `dealloc` in the kernel.
I think the value of the alignment should still be correct if at some
point in the future `dealloc` starts to use it again.
>
>>> + }
>>> + }
>>> +}
>>> +
>>> +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?
Maybe `Layout::new()`, `Layout::extend` and `Layout::repeat` might be
enough?
>
>>> + 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?
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.
--
Cheers,
Benno
>
>>> + }
>>> +
>>> + /// 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,
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-06-25 10:08 ` Benno Lossin
@ 2023-06-25 11:55 ` FUJITA Tomonori
2023-06-30 14:48 ` Benno Lossin
1 sibling, 0 replies; 17+ messages in thread
From: FUJITA Tomonori @ 2023-06-25 11:55 UTC (permalink / raw)
To: benno.lossin; +Cc: fujita.tomonori, rust-for-linux, gary
Hi,
On Sun, 25 Jun 2023 10:08:29 +0000
Benno Lossin <benno.lossin@proton.me> wrote:
(snip)
>>>> + 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.
>
> IMO having a `_` that separates words helps a lot with readability.
> Especially with `digestsize`. I also think that adding an `_` will not
> confuse the subsystem maintainers, so we should just do it.
Looks like `digestsize` is more popular in the tree so let's wait for
reviewing from the crypto maintainers:
ubuntu@ip-172-30-47-114:~/git/linux$ grep -or digestsize crypto/|wc -l
112
ubuntu@ip-172-30-47-114:~/git/linux$ grep -or digest_size crypto/|wc -l
37
>>>> + // 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.
>
> CC'ing Gary, since I am not familiar with `dealloc` in the kernel.
> I think the value of the alignment should still be correct if at some
> point in the future `dealloc` starts to use it again.
>
>>
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> +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?
>
> Maybe `Layout::new()`, `Layout::extend` and `Layout::repeat` might be
> enough?
new() needs type and extend() and repeat() need self; both is
irrelevant here.
>>>> + 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?
>
> 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'll change the code to return an error in this case. I prefer not to
extend C logic (like calling a C function multiple times) but if there
is an official policy for Rust bindings, I'll change the code to
follow the policy.
thanks,
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-06-25 10:08 ` Benno Lossin
2023-06-25 11:55 ` FUJITA Tomonori
@ 2023-06-30 14:48 ` Benno Lossin
2023-06-30 19:50 ` Greg KH
2023-07-03 23:19 ` Herbert Xu
1 sibling, 2 replies; 17+ messages in thread
From: Benno Lossin @ 2023-06-30 14:48 UTC (permalink / raw)
To: Herbert Xu, David S. Miller
Cc: FUJITA Tomonori, rust-for-linux, Gary Guo, linux-crypto
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-06-30 14:48 ` Benno Lossin
@ 2023-06-30 19:50 ` Greg KH
2023-07-03 23:19 ` Herbert Xu
1 sibling, 0 replies; 17+ messages in thread
From: Greg KH @ 2023-06-30 19:50 UTC (permalink / raw)
To: Benno Lossin
Cc: Herbert Xu, David S. Miller, FUJITA Tomonori, rust-for-linux,
Gary Guo, linux-crypto
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-06-30 14:48 ` Benno Lossin
2023-06-30 19:50 ` Greg KH
@ 2023-07-03 23:19 ` Herbert Xu
2023-07-10 19:59 ` Benno Lossin
1 sibling, 1 reply; 17+ messages in thread
From: Herbert Xu @ 2023-07-03 23:19 UTC (permalink / raw)
To: Benno Lossin
Cc: David S. Miller, FUJITA Tomonori, rust-for-linux, Gary Guo, linux-crypto
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 2/2] rust: add Random Number Generator algorithms support
2023-06-19 11:41 ` Benno Lossin
@ 2023-07-06 4:05 ` FUJITA Tomonori
0 siblings, 0 replies; 17+ messages in thread
From: FUJITA Tomonori @ 2023-07-06 4:05 UTC (permalink / raw)
To: benno.lossin; +Cc: fujita.tomonori, rust-for-linux
Hi,
On Mon, 19 Jun 2023 11:41:29 +0000
Benno Lossin <benno.lossin@proton.me> wrote:
>> diff --git a/rust/kernel/crypto/rng.rs b/rust/kernel/crypto/rng.rs
>> new file mode 100644
>> index 000000000000..2215a8344669
>> --- /dev/null
>> +++ b/rust/kernel/crypto/rng.rs
>> @@ -0,0 +1,72 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Random number generator.
>> +//!
>> +//! C headers: [`include/crypto/rng.h`](../../../../include/crypto/rng.h)
>> +
>> +use crate::{
>> + error::{from_err_ptr, to_result, Result},
>> + str::CStr,
>> +};
>> +
>> +/// Corresponds to the kernel's `struct crypto_rng`.
>> +///
>> +/// # Invariants
>> +///
>> +/// The pointer is valid.
>> +pub struct Rng(*mut bindings::crypto_rng);
>> +
>> +impl Drop for Rng {
>> + fn drop(&mut self) {
>> + // SAFETY: The type invariant guarantees that `self.0` is valid.
>
> This safety comment should explain why it is safe to access this mutable
> static variable.
>
>> + if unsafe { bindings::crypto_default_rng } == self.0 {
>> + // SAFETY: FFI call.
>> + unsafe {
>> + bindings::crypto_put_default_rng();
>> + }
>> + } else {
>> + // SAFETY: The type invariant guarantees that `self.0` is valid.
>> + unsafe { bindings::crypto_free_rng(self.0) };
>> + }
>> + }
>> +}
crypto_get_default_rng() is called during the initialization so it's
safe to access to bindings::crypto_default_rng. However, it's cleaner
to use an internal type like the following?
diff --git a/rust/kernel/crypto/rng.rs b/rust/kernel/crypto/rng.rs
new file mode 100644
index 000000000000..1d5d16577f3e
--- /dev/null
+++ b/rust/kernel/crypto/rng.rs
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Random number generator.
+//!
+//! C headers: [`include/crypto/rng.h`](../../../../include/crypto/rng.h)
+
+use crate::{
+ error::{code::EINVAL, from_err_ptr, to_result, Result},
+ str::CStr,
+};
+
+/// Type of Random number generator.
+///
+/// # Invariants
+///
+/// The pointer is valid.
+enum RngType {
+ /// Uses `crypto_default_rng`
+ Default(*mut bindings::crypto_rng),
+
+ /// Allocated via `crypto_alloc_rng.
+ Allocated(*mut bindings::crypto_rng),
+}
+
+/// Corresponds to the kernel's `struct crypto_rng`.
+pub struct Rng(RngType);
+
+impl Drop for Rng {
+ fn drop(&mut self) {
+ match self.0 {
+ RngType::Default(_) => {
+ // SAFETY: it's safe because `crypto_get_default_rng()` was called during
+ // the initialization.
+ unsafe {
+ bindings::crypto_put_default_rng();
+ }
+ }
+ RngType::Allocated(ptr) => {
+ // SAFETY: The type invariants of `RngType` guarantees that the pointer is valid.
+ unsafe { bindings::crypto_free_rng(ptr) };
+ }
+ }
+ }
+}
+
+impl Rng {
+ /// Creates a [`Rng`] instance.
+ pub fn new(name: &CStr, t: u32, mask: u32) -> Result<Self> {
+ // SAFETY: There are no safety requirements for this FFI call.
+ let ptr = unsafe { from_err_ptr(bindings::crypto_alloc_rng(name.as_char_ptr(), t, mask)) }?;
+ // INVARIANT: `ptr` is valid and non-null since `crypto_alloc_rng`
+ // returned a valid pointer which was null-checked.
+ Ok(Self(RngType::Allocated(ptr)))
+ }
+
+ /// Creates a [`Rng`] instance with a default algorithm.
+ pub fn new_with_default() -> Result<Self> {
+ // SAFETY: There are no safety requirements for this FFI call.
+ to_result(unsafe { bindings::crypto_get_default_rng() })?;
+ // INVARIANT: The C API guarantees that `crypto_default_rng` is valid until
+ // `crypto_put_default_rng` is called.
+ Ok(Self(RngType::Default(unsafe {
+ bindings::crypto_default_rng
+ })))
+ }
+
+ /// Get a random number.
+ pub fn generate(&mut self, src: &[u8], dst: &mut [u8]) -> Result {
+ if src.len() > u32::MAX as usize || dst.len() > u32::MAX as usize {
+ return Err(EINVAL);
+ }
+ let ptr = match self.0 {
+ RngType::Default(ptr) => ptr,
+ RngType::Allocated(ptr) => ptr,
+ };
+ // SAFETY: The type invariants of `RngType' guarantees that the pointer is valid.
+ to_result(unsafe {
+ bindings::crypto_rng_generate(
+ ptr,
+ src.as_ptr(),
+ src.len() as u32,
+ dst.as_mut_ptr(),
+ dst.len() as u32,
+ )
+ })
+ }
+
+ /// Re-initializes the [`Rng`] instance.
+ pub fn reset(&mut self, seed: &[u8]) -> Result {
+ if seed.len() > u32::MAX as usize {
+ return Err(EINVAL);
+ }
+ let ptr = match self.0 {
+ RngType::Default(ptr) => ptr,
+ RngType::Allocated(ptr) => ptr,
+ };
+ // SAFETY: The type invariants of `RngType' guarantees that the pointer is valid.
+ to_result(unsafe { bindings::crypto_rng_reset(ptr, seed.as_ptr(), seed.len() as u32) })
+ }
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [RFC PATCH v2 1/2] rust: add synchronous message digest support
2023-07-03 23:19 ` Herbert Xu
@ 2023-07-10 19:59 ` Benno Lossin
0 siblings, 0 replies; 17+ messages in thread
From: Benno Lossin @ 2023-07-10 19:59 UTC (permalink / raw)
To: Herbert Xu
Cc: David S. Miller, FUJITA Tomonori, rust-for-linux, Gary Guo, linux-crypto
------- 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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-07-10 19:59 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-15 14:23 [RFC PATCH v2 0/2] Rust abstractions for Crypto API FUJITA Tomonori
2023-06-15 14:23 ` [RFC PATCH v2 1/2] rust: add synchronous message digest support FUJITA Tomonori
2023-06-15 15:01 ` Greg KH
2023-06-15 15:33 ` FUJITA Tomonori
2023-06-15 15:02 ` Alex Gaynor
2023-06-15 15:24 ` FUJITA Tomonori
2023-06-19 11:40 ` Benno Lossin
2023-06-22 2:14 ` FUJITA Tomonori
2023-06-25 10:08 ` Benno Lossin
2023-06-25 11:55 ` FUJITA Tomonori
2023-06-30 14:48 ` Benno Lossin
2023-06-30 19:50 ` Greg KH
2023-07-03 23:19 ` Herbert Xu
2023-07-10 19:59 ` Benno Lossin
2023-06-15 14:23 ` [RFC PATCH v2 2/2] rust: add Random Number Generator algorithms support FUJITA Tomonori
2023-06-19 11:41 ` Benno Lossin
2023-07-06 4:05 ` FUJITA Tomonori
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.