Skip to content
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

Initial support for the ATECC508A #3696

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alistair23
Copy link
Contributor

@alistair23 alistair23 commented Sep 29, 2023

Pull Request Overview

The ATECC508A is an interesting Cryptographic Co-Processor.

It connects to an MCU via I2C and can then provide crypto features, including

  • Creating and securely storing unique asymmetric key pairs based on Elliptic Curve Cryptography (FIPS186-3).
  • Creating and verifying 64-byte digital signatures (from 32-bytes of message data).
  • Creating a shared secret key on a public channel via Elliptic Curve Diffie-Hellman Algorithm.
  • Creating and verifying 64-byte digital signatures (from 32-bytes of message data).
  • Internal high quality FIPS random number generator.

The ATECC608A (different chip, but backwards compatible) even supports AES encryption/decryption. The ATECC608A is aimed at TLS support and secure booting, both of which are interesting use cases for Tock.

The ATECC508A/ATECC608A is useful for Tock boards as it provides crypto operations and entropy sources for boards that otherwise wouldn't have any. The hope is that co-processors like this can help expand Tock's crypto support and develop generic HILs and capsules. It would also be great to implement a measured boot in Tock.

Testing Strategy

This PR can lock the device and retrieve random numbers from it.

TODO or Help Wanted

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

@alevy
Copy link
Member

alevy commented Oct 20, 2023

Generally, I think it absolutely makes sense to include/use cryptographic peripherals.

First, while having the cipher/plaintext exposed over, e.g. I2C that could be sniffed with physical access is a bummer, but fine under some threat models (e.g. TPMs often have this "flaw" at a higher level). Second, it's really not the place of the Tock, IMO, to dictate which threat models are or are not acceptable.

One caveat would be if there is a significant architectural or performance/overhead cost to support multiple cases, we might want to optimize for the case that is either more common or "better."

In this case, since cryptographic accelerators, whether on or off die, are very likely to be asynchronous anyway (we're not talking about an AESNI instruction or something, but co-processors in both cases), I don't forsee that being an issue here.

scheduler: &'static RoundRobinSched<'static>,
systick: cortexm4::systick::SysTick,
}

fn atecc508a_wakeup() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wakeup function is a little odd. We need to wake the device up before each request and this is the best way I can think of doing it.

Basically the board provides a wakeup function to the capsule. The capsule then calls that function before accessing the device.

It unfortunately contains unsafe and will probably break the I2C mux as well. The capsule can change the SDA line without going through the I2C capsule.

Any thoughts on better ways to do this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a case for a PeripheralManagement wrapper. Put the object to access the device inside of a PM struct, which can handle the wakeup (and poweroff if appropriate) correctly and consistently in one place; also encapsulates the unsafe if-needed.

The sam4l i2c driver has a reasonably advanced example of using PM.

Why would it break the I2C mux? If it's a case of needing to 'own' the bus before starting transaction, I think you can use the before/after_peripheral_management hooks, possibly with a deferred call in the before path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a case for a PeripheralManagement wrapper. Put the object to access the device inside of a PM struct, which can handle the wakeup (and poweroff if appropriate) correctly and consistently in one place; also encapsulates the unsafe if-needed.

I'm not sure that fixes the issue. From what I can tell the PeripheralManager would wrap the I2C chip implementation that bangs on the MMIO registers. That means that every I2C device in Tock needs to be wrapped. In this case it's the actual I2C device (I2CDevice in Tock) that needs to have the I2C bus pulled low before opreations.

Why would it break the I2C mux?

Currently one device could be performing an I2C operation yet this capsule can still pull the SDA line low.

The before_peripheral_access() looks like the right direction, but I need it to apply to specifically this one capsule when it tried to perform an I2C operation. So I really need a before_peripheral_access() callback supported in I2CDevice() that can be set in the board file

@radnvlad
Copy link

First, while having the cipher/plaintext exposed over, e.g. I2C that could be sniffed with physical access is a bummer, but fine under some threat models (e.g. TPMs often have this "flaw" at a higher level).

I know I'm kind of necroing this and just leaving a drive-by reply, but the ATECC608A is capable of having a symmetric key on the MCU and on the secure element and you can encrypt the I2C communication, in order to mitigate this issue.

If we read the interrupt state again during the interrupt handler we can
miss important information, such as a Nack event. So don't handle the
interrupts that occur while processing an interrupt and instead wait for
the interrupt handler to be called again.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
@alistair23 alistair23 force-pushed the alistair/atecc508a branch 2 times, most recently from 0e4e20c to ad8df50 Compare May 1, 2024 05:33
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
…ation Device

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
…on Device

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
@alistair23 alistair23 marked this pull request as ready for review May 1, 2024 05:38
@alistair23
Copy link
Contributor Author

Ready to go now

@alistair23 alistair23 requested a review from ppannuto May 1, 2024 05:38
@alistair23
Copy link
Contributor Author

Ping!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants