Improve SHA driver API

[Dominaezzz]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• My changes were added to the CHANGELOG.md in the proper section.
• My changes are in accordance to the esp-rs API guidelines

Extra:

• I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

Fixes #2041 .

Improvements:

• Bring back the peripheral ref pattern to track exclusive access to the shared registers.
• Saving and restore the SHA state is now an explicit operation, which reduces the number of memcpys required to sha something.
• Fixes the issue linked above.
• Removed the save/restore feature from the base ESP32. It doesn't support it.

Regressions:

• Removed the implementation of the Digest trait. Unfortunately the Digest trait has a Digest::new() method, which can't be trivially implemented in a safe manner as you need global access to the peripheral. The answer here is likely some kind of global mutex, but I'm not solving that here as there's no one right implementation.

Future potential improvements:

• Make ShaDigest::finish truly non-blocking. Doing so requires an enum/state machine to track the finishing process.
• Start writing the next message whilst the previous one is being processed, increasing throughput. This also requires a state machine. It's unclear from the TRM whether the base ESP32 supports it but I've confirmed that S2, S3 and C6 support this.
• Make size of Context object depend on algorithm used, to save memory.
• Add async support. (Unsure if this is possible without using DMA)
• Add DMA support.
• The TRMs say that SHA cannot be accessed whilst DS or HMAC is working so... some research needs to go into how to handle this in the hal.

Questions:

• I've opt-ed to save and restore the Context by mutable reference. I'm wondering if this should be done by taking ownership instead.

Testing

Ran the HIL tests on an ESP32-S3.

[bugadani]

Since the generic is only required because of the Digest output type, and we already need an adapter type for the digest support, I'd suggest a) removing the digest feature and b) removing the generic and turning the algorithm selection into an enum. There's no reason to lug that type information around, at least I don't see any good reason to do so.

[Dominaezzz]

I haven't added it in this PR but the generic would be used for this enhancement.

Make size of Context object depend on algorithm used, to save memory.

It's also handy for the register calls on the ESP32 but I suppose that can be solved with some branches. Besides those I don't see any good reasons either.

a) removing the digest feature

You mean remove the traits completely or to hard depend on the digest crate?
If it's the former I'm on board but I feel a bit bad removing the whole thing after @AnthonyGrondin just added it.

[AnthonyGrondin]

I think he means removing the feature as a dependency flag and always pull the digest crate. In any cases, support for digest::Digest will be needed in one way or another for using with crates that implement RustCrypto traits, and for the incoming rustls support, if we want to use HW acceleration.

My 2 cents; When I implemented the RSA support for esp-mbedtls, the espressif fork we use provides a way to still fallback on the software implementation if needed. Hence why it was possible to optionally use hardware accelerated RSA at runtime. For SHA we don't get this luxury. We'd have to either use HW or SW. That means forcing the user to pass &mut SHA to use TLS. Hence why creating hashers out of thin-air was needed, to not make the user carry peripherals all around the application. But I do understand the safety issues with this approach, and using a global aquire mecanism is another layer of complexity, that can come in a subsequent PR later.

[bugadani]

I meant removing digest support - not sure if it's useful at all if we're not actually implementing Digest. Hard depending on digest wouldn't be a problem, we're moving away from optional trait impls anyway as they are just a headache for users.

Digest::new certainly makes things more annoying and I get why Sha conjured the peripheral so eagerly. As the options seem tradeoffy, maybe we don't need to limit ourselves to a single API. We can keep a PeripheralRef based implementation for normal use, and an extras/esp-digest crate implementation that may have some hackery, more context save/restore and some global locks, whatever we need to get digest working soundly.

[AnthonyGrondin]

I'd like to test this with esp-mbedtls to confirm everything can still work for an HW impl, even if we have to take the &mut SHA peripheral. So far I've made it compile but I'm getting a index out of bounds: the len is 4 but the index is 12 error, probably due to user error.

As for the global mutex / locking mecanism, we can use something like the lock introduced in #2051 to ensure safe multi_core concurrency without a PeripheralRef. esp-idf uses an acquire / release mecanism for its SHA implementation. I know its a bit of a far reach from the original scope of this PR, but that would prevent any regression :)

[Dominaezzz]

So far I've made it compile but I'm getting a index out of bounds: the len is 4 but the index is 12 error, probably due to user error.

Got a stack? I'm curious.

[AnthonyGrondin]

None, Just an assert error.

====================== PANIC ======================
panicked at ~/.cargo/git/checkouts/esp-hal-28618c44cb435972/dd2aafa/esp-hal/src/reg_access.rs:194:25:
index out of bounds: the len is 4 but the index is 12

Backtrace:

Error:   × flush failed

[Dominaezzz]

Considering the location it's either an existing bug or perhaps user error like you said. Though user error should only cause this if you're using unsafe I think, like MaybeUninit or something.

[AnthonyGrondin]

It's most likely a user error. I tried to get something quick and dirty to test.

Can I ask you to provide a migration guide for the existing SHA impl PR in esp-mbedtls, since you're more familiar with this API? Doesn't need to be for every algorithm, just for Sha1 and I'll figure out the rest and the passing of the SHA PeripheralRef.

What I've tried is to put ShaDigest::<Sha1, Sha<'a>>::new(sha) in hasher_mem and calling the update method from the digest crate directly with: Update::update((*ctx).hasher.as_mut().unwrap(), &*slice);

[Dominaezzz]

The diff in the driver doc can function as a migration guide but I can take a look at the PR sure

[Dominaezzz]

The PR is a bit too big for me to look at right now but here some pseudo code.

static SHARED_SHA: Mutex<RefCell<Option<Sha<'static>>>> = Mutex::new(RefCell::new(None));

fn main() {
    let sha = Sha::new(peripherals.SHA);
    critical_section::with(|cs| SHARED_SHA.borrow_ref_mut(cs).replace(Some(sha)));

    // do other things
}

// Your global functions will look like this.
fn sha_something(mut data: &[u8], output: &mut [0u8; 32]) {
    critical_section::with(|cs| {
        let hasher = SHARED_SHA.borrow_ref_mut(cs).as_mut().unwrap().start::<Sha1>();
        while !data.is_empty() {
           data = block!(hasher.update(data)).unwrap();
        }
        block!(hasher.finish(output)).unwrap();
    });
}

[AnthonyGrondin]

Good news 🎉

I've locally refactored the SHA PR for esp-mbedtls to use the new driver API. It's now even faster, for the self-tests

Hash Algorithm	Software (cycles)	Hardware (cycles)	HW New driver (cycles)
SHA-1	3,390,785	2,171,981	896,889
SHA-224	8,251,799	2,151,948	898,344
SHA-256	8,237,932	2,149,413	901,709
SHA-384	13,605,806	1,298,537	799,532
SHA-512	13,588,104	1,296,381	801,556

[Dominaezzz]

Woah! I didn't think the memcpy's would make that much of a difference.
I'm curious how much faster it'll be with the 2nd improvement in the PR description.
Which chip is that on?

[AnthonyGrondin]

I've achieved those results on the esp32s3.

I'll update the PR in a few hours to reflect the changes.

[AnthonyGrondin]

• Removed the save/restore feature from the base ESP32. It doesn't support it.

Can you expand on this point? The esp32 uses different register for each hashing algorithm, but I remember the old implementation supporting interleaving operations, albeit in an unsafe manner. After updating esp-rs/esp-mbedtls#46 esp32 is no longer supported due to not having the possibility to restore a context.

[Dominaezzz]

Sure! Like other chips the esp32 allows you to save the message and digest. However it doesn't let you restore the digest. Once you start hashing something else, the digest is lost and you have to start again. The TRM also doesn't mention any interleaving support fwiw.

I don't think the old driver was working on the esp32, hence the failing hil tests in #1977 .

The previous driver was using the hash of the previous step as the message of the next step, as it was accidently overriding the message with the digest at restoration time.

[AnthonyGrondin]

Gotcha! I assumed it was working after some minimal local testing.

LGTM for this new implementation on my side. It's now safer and faster. Even if we lose the Digest::new() impl, but we can figure that out later with a global lock state like esp-idf does it.

[bjoernQ]

Thanks

[MabezDev]

LGTM, thanks!