-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(SHA256): Add initial hardware acceleration impl for SHA256 #19
base: main
Are you sure you want to change the base?
Conversation
- Currently, esp-wifi needs to be patched to work with this hal revision
Are the exceptions raised in |
This is a chain of unsafe operations, and I think many of them are causing the issue. esp-mbedtls/esp-mbedtls/src/sha/sha256.rs Line 47 in ab4ec67
If I replace this by a static variable that contains SHA from peripheral, instead of stealing it in _init(), it doesn't overflow there anymore. It then overflows where I try to save the hasher in the context, at this line: esp-mbedtls/esp-mbedtls/src/sha/sha256.rs Line 49 in ab4ec67
|
I had a look and I think I partly know what is going wrong: typedef struct mbedtls_sha256_context {
void* peripheral;
void* hasher;
} mbedtls_sha256_context; is not the same as #[repr(C)]
pub struct mbedtls_sha256_context<'a> {
peripheral: SHA,
hasher: Sha<'a>,
} On the C side there are just two pointers while on the Rust side there is a ZST and something which is definitely not ZST. I tried something and came up with use crate::hal::peripherals::SHA;
use crate::hal::sha::Sha;
use crate::hal::sha::ShaMode;
use esp_mbedtls_sys::c_types::*;
use nb::block;
#[repr(C)]
pub struct mbedtls_sha256_context<'a> {
peripheral: *mut SHA,
hasher: *mut Sha<'a>,
}
#[no_mangle]
pub unsafe extern "C" fn mbedtls_sha256_init(ctx: *mut mbedtls_sha256_context) {
let hasher_mem = crate::calloc(1, core::mem::size_of::<Sha<'_>>() as u32) as *mut Sha<'_>;
(*ctx).hasher = hasher_mem;
}
#[no_mangle]
pub unsafe extern "C" fn mbedtls_sha256_free(ctx: *mut mbedtls_sha256_context) {
if !ctx.is_null() && !(*ctx).hasher.is_null() {
crate::free((*ctx).hasher as *const u8);
(*ctx).hasher = core::ptr::null_mut();
}
}
#[no_mangle]
pub unsafe extern "C" fn mbedtls_sha256_clone<'a>(
dst: *mut mbedtls_sha256_context<'a>,
src: *const mbedtls_sha256_context<'a>,
) {
core::ptr::copy_nonoverlapping(src, dst, 1);
mbedtls_sha256_init(dst);
core::ptr::copy((*src).hasher, (*dst).hasher, 1);
}
#[allow(unused_variables)]
#[no_mangle]
pub unsafe extern "C" fn mbedtls_sha256_starts(
ctx: *mut mbedtls_sha256_context,
is224: c_int,
) -> c_int {
#[cfg(not(feature = "esp32"))]
let mode = if is224 == 1 {
ShaMode::SHA224
} else {
ShaMode::SHA256
};
#[cfg(feature = "esp32")]
let mode = ShaMode::SHA256;
let hasher = Sha::new(SHA::steal(), mode);
core::ptr::copy(&hasher as *const _, (*ctx).hasher, 1);
0
}
#[no_mangle]
pub unsafe extern "C" fn mbedtls_sha256_update(
ctx: *mut mbedtls_sha256_context,
input: *const c_uchar,
ilen: usize,
) -> c_int {
let slice = core::ptr::slice_from_raw_parts(input as *const u8, ilen as usize);
let mut remaining = &*slice;
while remaining.len() > 0 {
remaining = block!((*ctx).hasher.as_mut().unwrap().update(remaining)).unwrap();
}
0
}
#[no_mangle]
pub unsafe extern "C" fn mbedtls_sha256_finish(
ctx: *mut mbedtls_sha256_context,
output: *mut c_uchar,
) -> c_int {
let mut data = [0u8; 32];
block!((*ctx).hasher.as_mut().unwrap().finish(&mut data)).unwrap();
core::ptr::copy_nonoverlapping(data.as_ptr(), output, data.len());
0
} This is not ideal but I tried to avoid changing the C side. You can just ignore With the above code the tests still work and the example runs into Especially I wonder about the e.g. esp-idf does that here: https://github.com/espressif/esp-idf/blob/master/components/mbedtls/port/sha/block/esp_sha256.c#L106 - especially in |
Hi, Thanks for the feedback and help. We can always change the C side headers, as we are providing the alternative context. If there are better ways to represent the context in C, we should do it right that way. Yeah, I'm not sure about the handling of the clone either. I've also looked at what we write in the output in I think the main difference between self_tests and a real operation, is that self_tests don't use clone, while doing a request clones the context a few times. I'm seeing that the C implementation in I've ran
|
That's definitely an improvement then. I guess the general idea of the code here is fine so no need to worry about esp-idf looking quite different. I think the real remaining problem is that the driver needs a way to "resume" an operation and switch between them. Technically I think we just need to restore some state but the way the driver works currently (IIRC) might require some API changes (i.e. there is currently no safe way to create multiple instances of it) We can then make |
Description
This adds an initial support to use the hardware accelerated drivers for sha256 operations.
It is currently in a PoC phase.
It is my first time writing a Rust implementation for FFI bindings, so feel free to provide as much feedback as needed.
Current issue.
I am currently stuck on whether or not this implementation is safe and sound, because it successfully passes the self_tests, but it will fail to run the examples in a real_case scenario, triggering
Store/AMO access fault
errors and such.Building / Bindings
You can re-compile mbedtls and generate new bindings using the xtask to work with the
sha256_alt.
.It's not obligatory since I've edited
mbedtls
and included the compiled libraries in this PR to provide a temporary fix for #13, where it would not print if a test has passed or failedTesting
esp-wifi
to be compatible with the breaking changes from Implementembedded_hal_async::delay::DelayUs
trait forSYSTIMER
alarms esp-hal#812Here's a patch that works for me, for a quick copy paste with the current origin:
crypto_self_test
example to test current and future crypto acceleration implementationsHow to run (esp32c3):
cargo run --release --example crypto_self_test --target riscv32imc-unknown-none-elf -F esp32c3,esp-wifi/wifi-logs